How to make it run sync?

Question

Hello I want to sync two threads one incrementing a variable and other decrementing it. The result that I want looks like:

Thread #0 j = 1

Thread #1 j = 0

Thread #0 j = 1

Thread #1 j = 0

And so on.. but my code sometimes works like that in some cases it print really weird values. I supose that I have some undefined behavior in somewhere but I can't figured out what is really happen.

My code consist in a HANDLE ghMutex that containg the handler of my mutex:

My main function:

int main(void)
{
    HANDLE aThread[THREADCOUNT];

    ghMutex = CreateMutex(NULL, FALSE, NULL);             

    aThread[0] = (HANDLE)_beginthreadex(NULL, 0, &inc, NULL, CREATE_SUSPENDED, 0);
    aThread[1] = (HANDLE)_beginthreadex(NULL, 0, &dec, NULL, CREATE_SUSPENDED, 0);

    ResumeThread(aThread[0]);
    ResumeThread(aThread[1]);

    WaitForMultipleObjects(THREADCOUNT, aThread, TRUE, INFINITE);

    printf("j = %d\n", j);

    for (int i = 0; i < THREADCOUNT; i++)
        CloseHandle(aThread[i]);

    CloseHandle(ghMutex);

    return 0;
}

Inc function:

unsigned int __stdcall inc(LPVOID)
{
    for (volatile int i = 0; i < MAX; ++i)
    {
        WaitForSingleObject(
            ghMutex,    // handle to mutex
            INFINITE);  // no time-out interval

            j++;
            printf("Thread %d j = %d\n", GetCurrentThreadId(), j);
            ReleaseMutex(ghMutex);

    }
    _endthread();

    return TRUE;
}

Dec function:

unsigned int __stdcall dec(void*)
{
    for (volatile int i = 0; i < MAX; ++i)
    {
        WaitForSingleObject(
            ghMutex,    // handle to mutex
            INFINITE);  // no time-out interval

        j--;
        printf("Thread %d j = %d\n", GetCurrentThreadId(), j);
        ReleaseMutex(ghMutex);
    }
    _endthread();

    return TRUE;
}

I need a win api solution in std c++98.

Answer 1

windows mutex object guarantees exclusive ownership, but does not care about the ownership order. so that the same thread can capture several times in a row while others will wait.

for your task you need signal to another thread, when your task is done, and then wait for signal from another thread. for this task can be used event pair for example. thread (i) signal event (1-i) and wait on event (i). for optimize instead 2 calls -

SetEvent(e[1-i]); WaitForSingleObject(e[i], INFINITE);

we can use single call SignalObjectAndWait

SignalObjectAndWait(e[1-i], e[i], INFINITE, FALSE)

of course start and end of loop require special care. for inc

    HANDLE hObjectToSignal  = _hEvent[1], hObjectToWaitOn  = _hEvent[0];

    for (;;)
    {
        _shared_value++;

        if (!--n)
        {
            SetEvent(hObjectToSignal);
            break;
        }

        SignalObjectAndWait(hObjectToSignal, hObjectToWaitOn, INFINITE, FALSE);
    }

and for dec

    HANDLE hObjectToSignal  = _hEvent[0], hObjectToWaitOn  = _hEvent[1];

    WaitForSingleObject(hObjectToWaitOn, INFINITE);
    for (;;)
    {
        --_shared_value;

        if (!--n)
        {
            break;
        }

        SignalObjectAndWait(hObjectToSignal, hObjectToWaitOn, INFINITE, FALSE);
    }

if write full test, with error checking

struct Task 
{
    HANDLE _hEvent[4];
    ULONG _n;
    LONG _iTasks;
    LONG _shared_value;

    Task()
    {
        RtlZeroMemory(this, sizeof(*this));
    }

    ~Task()
    {
        ULONG n = RTL_NUMBER_OF(_hEvent);
        do 
        {
            if (HANDLE hEvent = _hEvent[--n]) CloseHandle(hEvent);
        } while (n);
    }

    ULONG WaitTaskEnd()
    {
        return WaitForSingleObject(_hEvent[2], INFINITE);
    }

    ULONG WaitTaskReady()
    {
        return WaitForSingleObject(_hEvent[3], INFINITE);
    }

    void SetTaskReady()
    {
        SetEvent(_hEvent[3]);
    }

    void End()
    {
        if (!InterlockedDecrement(&_iTasks)) SetEvent(_hEvent[2]);
    }

    void Begin()
    {
        InterlockedIncrementNoFence(&_iTasks);
    }

    static ULONG WINAPI IncThread(PVOID p)
    {
        return reinterpret_cast<Task*>(p)->Inc(), 0;
    }

    void Inc()
    {
        if (WaitTaskReady() == WAIT_OBJECT_0)
        {
            if (ULONG n = _n)
            {
                HANDLE hObjectToSignal  = _hEvent[1], hObjectToWaitOn  = _hEvent[0];

                for (;;)
                {
                    if (_shared_value) __debugbreak();

                    if (n < 17) DbgPrint("Inc(%u)\n", n);

                    _shared_value++;

                    if (!--n)
                    {
                        SetEvent(hObjectToSignal);
                        break;
                    }

                    if (SignalObjectAndWait(hObjectToSignal, hObjectToWaitOn, INFINITE, FALSE) != WAIT_OBJECT_0)
                    {
                        break;
                    }
                }
            }
        }

        End();
    }

    static ULONG WINAPI DecThread(PVOID p)
    {
        return reinterpret_cast<Task*>(p)->Dec(), 0;
    }

    void Dec()
    {
        if (WaitTaskReady() == WAIT_OBJECT_0)
        {
            if (ULONG n = _n)
            {
                HANDLE hObjectToSignal  = _hEvent[0], hObjectToWaitOn  = _hEvent[1];

                if (WaitForSingleObject(hObjectToWaitOn, INFINITE) == WAIT_OBJECT_0)
                {
                    for (;;)
                    {
                        --_shared_value;

                        if (_shared_value) __debugbreak();

                        if (n < 17) DbgPrint("Dec(%u)\n", n);

                        if (!--n)
                        {
                            break;
                        }

                        if (SignalObjectAndWait(hObjectToSignal, hObjectToWaitOn, INFINITE, FALSE) != WAIT_OBJECT_0)
                        {
                            break;
                        }
                    }
                }
            }
        }

        End();
    }

    ULONG Create()
    {
        ULONG n = RTL_NUMBER_OF(_hEvent);
        do 
        {
            if (HANDLE hEvent = CreateEventW(0, n > 2, 0, 0)) _hEvent[--n] = hEvent; 
            else return GetLastError();
        } while (n);

        return NOERROR;
    }

    ULONG Start()
    {
        static PTHREAD_START_ROUTINE aa[] = { IncThread, DecThread };

        ULONG n = RTL_NUMBER_OF(aa);

        do 
        {
            Begin();
            if (HANDLE hThread = CreateThread(0, 0, aa[--n], this, 0, 0))
            {
                CloseHandle(hThread);
            }
            else
            {
                n = GetLastError();
                End();
                return n;
            }
        } while (n);

        return NOERROR;
    }

    ULONG Start(ULONG n)
    {
        _iTasks = 1;

        ULONG dwError = Start();

        _n = dwError ? 0 : n;

        SetTaskReady();

        End();

        return dwError;
    }
};

void TaskTest(ULONG n)
{
    Task task;
    if (task.Create() == NOERROR)
    {
        task.Start(n);
        task.WaitTaskEnd();
    }
}

note, that no any sense declare local variable (which will be accessed only from single thread and not accessed by any interrupts, etc) as volatile

also when we write code, like:

// thread #1
write_shared_data();
SetEvent(hEvent);

// thread #2
WaitForSingleObject(hEvent, INFINITE);
read_shared_data();

inside SetEvent(hEvent); was atomic write to event state with release semantic (really stronger of course) and inside wait for event function - atomic read it state with more than acquire semantic. as result all what thread #1 write to memory before SetEvent - will be visible to thread #2 after Wait for event (if wait finished as result of call Set from thread #1)

Answer 2

A mutex is not the right tool to synchronize two threads, it is there to protect a resource. You do have a resource j which is protected by your mutex, however the sequence of which thread gets the lock is undefined, so you can have the case where dec gets called several times before inc has the chance to run.

If you want to synchronize the order of the threads you will have to use another synchronization primitive, for example a semaphore. You could, for example, increment the semaphore in inc and decrement it in dec. This would be the classic producer - consumer relationship where the producer will be stalled when the semaphore reaches its maximum value and the consumer will wait for items to consume.

Sorry, no WinAPI C++98 solution from me because that would be silly, but I hope I pointed you to the right direction.