Simple multithreading mutex example is incorrect

Question

I expect to get numbers from 0 to 4 in random order, but instead, I have some unsynchronized mess

What i do wrong?

#include <iostream>
#include <windows.h>
#include <process.h>

using namespace std;

void addQuery(void *v );

HANDLE ghMutex;

int main()
{
    HANDLE hs[5];
    ghMutex = CreateMutex( NULL, FALSE, NULL);         
    for(int i=0; i<5; ++i)
    {
        hs[i] = (HANDLE)_beginthread(addQuery, 0, (void *)&i);
        if (hs[i] == NULL) 
        {
            printf("error\n"); return -1;
        }
    }

    printf("WaitForMultipleObjects return: %d error: %d\n",
         (DWORD)WaitForMultipleObjects(5, hs, TRUE, INFINITE), GetLastError());


    return 0;
}

void addQuery(void *v )
{
    int t = *((int*)v);
    WaitForSingleObject(ghMutex, INFINITE);

    cout << t << endl;

    ReleaseMutex(ghMutex);
    _endthread();
}

Answer 1

You have to read and write the shared variable inside the lock. You are reading it outside of the lock and thus rendering the lock irrelevant.

But even that's not enough since your shared variable is a loop variable that you are writing to without protection of the lock. A much better example would run like this:

#include <iostream>
#include <windows.h>
#include <process.h>

using namespace std;

void addQuery(void *v );

HANDLE ghMutex;
int counter = 0;

int main()
{
    HANDLE hs[5];
    ghMutex = CreateMutex( NULL, FALSE, NULL);         
    for(int i=0; i<5; ++i)
    {
        hs[i] = (HANDLE)_beginthread(addQuery, 0, NULL);
        if (hs[i] == NULL) 
        {
            printf("error\n"); return -1;
        }
    }

    printf("WaitForMultipleObjects return: %d error: %d\n",
         (DWORD)WaitForMultipleObjects(5, hs, TRUE, INFINITE), GetLastError());


    return 0;
}

void addQuery(void *v)
{
    WaitForSingleObject(ghMutex, INFINITE);

    cout << counter << endl;
    counter++;

    ReleaseMutex(ghMutex);
    _endthread();
}

If you can, use a critical section rather than a mutex because they are simpler to use and more efficient. But they have the same semantics in that they only protect code inside the locking block.

Note: Jerry has pointer out some other problems, but I've concentrated on the high level trheading and serialization concerns.