Why is this synchronization not working?

Question

I have this code:

    #include <windows.h>
#include <tchar.h>
#include <stdio.h>
#include <stdlib.h>

#include <time.h>

#define ITERATIONS 10

typedef struct NUMERE
{
    DWORD a;
    DWORD b;
} *PNUMERE;

HANDLE ghThreadHandle[2];
HANDLE ghEvents[2];
//HANDLE hEvent;

NUMERE nr;

DWORD WINAPI GenerateNumbers(PNUMERE nr)
{
    //PNUMERE nr = ((PNUMERE)param);
    if(nr == NULL)
        return -1;

    nr->a = rand() % 100;
    nr->b = (nr->a) * 2;

    _tprintf(TEXT("Generated\n"));

    //Sleep(10);

    return 0;
}

DWORD WINAPI DisplayNumbers(PNUMERE nr)
{
    //NUMERE nr = *((PNUMERE)param);

    _tprintf(TEXT("Displayed: %d %d\n"),nr->a,nr->b);

    return 0;
}

DWORD WINAPI DoStuff(PVOID param)
{
    int index = *((int*)param);

    for(unsigned int i = 0 ; i < ITERATIONS ; i++)
    {
        if(index == 0)
        {
            WaitForSingleObject(ghEvents[1],INFINITE);
            ResetEvent(ghEvents[0]);

            if(GenerateNumbers(&nr) == -1)
                _tprintf(TEXT("GenerateNumbers error!\n"));

            SetEvent(ghEvents[0]);
            ResetEvent(ghEvents[1]);
        }
        else
        {
            WaitForSingleObject(ghEvents[0],INFINITE);
            ResetEvent(ghEvents[1]);

            DisplayNumbers(&nr);

            SetEvent(ghEvents[1]);
            ResetEvent(ghEvents[0]);
        }
    }

    return 0;
}

DWORD GenerateThreads()
{
    int temp0 = 0, temp1 = 1;
    ghThreadHandle[0] = CreateThread(NULL
        ,0
        ,(LPTHREAD_START_ROUTINE)DoStuff
        ,(LPVOID)&temp0
        ,0
        ,NULL);

    if(NULL == ghThreadHandle[0])
        return -1;

    ghThreadHandle[1] = CreateThread(NULL
        ,0
        ,(LPTHREAD_START_ROUTINE)DoStuff
        ,(LPVOID)&temp1
        ,0
        ,NULL);

    if(NULL == ghThreadHandle[1])
    {
        CloseHandle(ghThreadHandle[0]);
        return -1;
    }

    return 0;
}

int main()
{   
    srand(time(NULL));

    ghEvents[0] = CreateEvent(NULL,TRUE,TRUE,TEXT("MyEvent0"));
    ghEvents[1] = CreateEvent(NULL,TRUE,TRUE,TEXT("MyEvent1"));

    if(NULL == ghEvents[0] || NULL == ghEvents[1])
    {
        _tprintf("Error creating events\n");
        return -1;
    }

    if(GenerateThreads() == -1)
    {
        _tprintf("Error GenerateThreads\n");
        return -1;
    }

    WaitForMultipleObjects(2,ghThreadHandle,TRUE,INFINITE);

    //getchar();

    CloseHandle(ghThreadHandle[0]);
    CloseHandle(ghThreadHandle[1]);

    CloseHandle(ghEvents[0]);
    CloseHandle(ghEvents[1]);

    return 0;
}

I want the two functions (GenerateNumbers and DisplayNumbers) to be called alternatively. However, upon launch, GenerateNumbers is called twice and then it just waits. The DisplayNumbers method never gets called. Can someone explain the cause of this deadlock?

Your example is not self-contained. It is not clear what `param` you are passing or how many threads you are creating. Note also that you created both events initially signaled, so you have a race condition right off the bat. — Raymond Chen, Apr 01 '13 at 21:53
I create two threads. Both of them receive an int as the parameter (the first is initialized with 0, the second one with 1). This is not the issue. I've tested this. — conectionist, Apr 01 '13 at 21:58
Since both events are created with initial state signaled, the two threads both run their first iteration concurrently, at which point things go downhill fast. Also, the synchronization code releases the other thread before finishing its cleanup (resetting the wake-up event) which can create other race conditions. Insert some print statements to see what's going on. — Raymond Chen, Apr 01 '13 at 22:07
@RaymondChen It just prints: Generated \n Generated \n ...and then just waits. Oddly, when I debug, it seems to work... — conectionist, Apr 01 '13 at 22:11
Add more print commands, like between every line. You are trying to debug the synchronization, so that's where the print commands go. — Raymond Chen, Apr 01 '13 at 22:18
@RaymondChen You said that the synchronization code releases the other thread before finishing its cleanup (resetting the wake-up event). I don't understand what you mean. If I've understood this mechanism correctly, the following should happen: 1. Wait for the signal from the other thread. 2. Block the other thread. 3. Do the stuff. 4. Give the other thread the signal. 5. Block the current thread. And then back to step 1. Can anyone please tell me where/shat I've misunderstood? — conectionist, Apr 01 '13 at 22:18
I've found the problem. Apparently the thread parameter is 0 in both cases (inside the function, not when it's passed). But I don't understand why... — conectionist, Apr 01 '13 at 22:34

score 3 · Accepted Answer · answered Apr 01 '13 at 22:37

The GenerateThreads function passes the address of local variables to the other thread (temp0 and temp1). The function then returns immediately after starting the threads. This means that the other threads are now accessing memory that has been freed. It appears that by the time the threads read their param, the memory changed value to zero, so both threads think that they are the GenerateNumbers thread.

Adding additional debug print statements would have identified this problem sooner.

Note that you still have a race condition at the end of each block, because you signal the other thread to start before resetting the wake-up event. The other thread may wake up, do its work, then set the wake-up event, all before the first thread returns from SetEvent. The first thread then resets its event and waits on it, causing it to lose the wake-up event and consequently hang.

Why is this synchronization not working?

1 Answers1