Re: [Threads-devel] Mutexes

From: Roland Schwarz (roland.schwarz_at_[hidden])
Date: 2006-03-03 03:01:33

Roland Schwarz schrieb:
> Altough what follows might seem like a small 
> bug, I think it indeedis part of a design problem.
>   
I did some further investigations, and I think the "bug" really isn't a 
small one!

It appears that Anhony is correct about his statement that in Windows it is
legal that a locked mutex may be destroyed.
Unfortunately the current implementation of boost mutex on windows does
not coope nicely with the windows semantics.

I will demonstrate this with the following code snippet:

#include <windows.h>

HANDLE m;
HANDLE th1;
HANDLE th2;

DWORD WINAPI thread1_main(LPVOID)
{
    DWORD dw;
    dw = WaitForSingleObject(m, INFINITE);
    if (dw == WAIT_FAILED) {
        return -1;
    }
    else
        // the thread1 succesfully acquires the lock and loops
        while(true) {};    dw = WaitForSingleObject(m, INFINITE);
    return 0;
}

DWORD WINAPI thread2_main(LPVOID)
{
    DWORD dw;
    dw = WaitForSingleObject(m, INFINITE);
    if (dw == WAIT_FAILED) {
        // thread2 goes here after the CloseHandle succeeded
        return -1;
    }
    else
        while(true) {};
    return 0;
}

int main(int argc, char* argv[])
{
DWORD error;

    m = CreateMutex(NULL, FALSE, NULL);
    th1 = CreateThread(NULL,0,thread1_main, NULL, 0, NULL);
    Sleep(1000);
    th2 = CreateThread(NULL,0,thread2_main, NULL, 0, NULL);
    Sleep(1000);

    // At this point thread1_main still holds the lock
    // and thread2_main waits for it
    if (!CloseHandle(m)) {
        // ... but CloseHandle does not return with an error!!
        // and this is really bad, since it means we cannot
        // detect that the lock still is owned.
        // Even worse is that thread1 still holds the lock
        // and if we are about to destroy the object that
        // mutex is supposed to protect thread1 will access
        // nomans land!
        error = GetLastError();
    }
    while(true){};
    return 0;
}

Then I looked into the current implementation, first the locking part:

void try_mutex::do_lock()
{
    if (m_critical_section)
        wait_critical_section_infinite(m_mutex);
    else
        wait_mutex(m_mutex, INFINITE);
}

Please note that wait_mutex does not return any result information,
nor does it throw exceptions. Consequently the thread will get the
impression that he has succesfully acquired the mutex.

Then the implementation of wait_mutex:

inline int wait_mutex(void* mutex, int time)
{
    unsigned int res = 0;
    res = WaitForSingleObject(mutex_cast(mutex), time);
//:xxx    assert(res != WAIT_FAILED && res != WAIT_ABANDONED);
    return res;
}

And this is interesting: The original writer obviously realized that 
this was an area
where some work still was needed (I conclude this from the comment).
Unfortunately the CVS does not give any hints when this comment has been 
added.

Please note that the overall boost mutex still appears correct, because 
of the
precondition that mutex is required to be unlocked when about to be 
destroyed.
But this requirement put the whole burden on the user, and the library 
gives no
support in the case of error. This can lead to very hard to debug code.
Please also note that behaviour on windows and unix will also be 
different in the
error case.

I think we should aim for something better!

Roland