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.

Answer 2

Your synchronization has some issues as you want to get numbers from 0 to 4 in random order.

The problem is that the variable i is write outside the lock and every time the addQuery method get called by the execution of a thread, it get the modified version of variable i. That why you may see 5 as the value at the output for all.

So, here is my fix for this scenario. Instead of pass the address of variable i in parameters of the function addQuery, you should pass it's value. Hope it helps:

#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();
}