CODEWITHSUNDEEP
c++multithreadingwinapieventsmutex
Ilya
Ilya

Reputation: 79

Problem with multi-threading and waiting on events

I have a problem with my code:

#define _CRT_SECURE_NO_WARNINGS
#include <iostream>
#include <windows.h>
#include <string.h>
#include <math.h>

HANDLE event;
HANDLE mutex;

int runner = 0;

DWORD WINAPI thread_fun(LPVOID lpParam) {
    int* data = (int*)lpParam;
    for (int j = 0; j < 4; j++) { //this loop necessary in order to reproduce the issue
        if ((data[2] + 1) == data[0]) { // if it is last thread 
            while (1) {
                WaitForSingleObject(mutex, INFINITE);
                if (runner == data[0] - 1) { // if all other thread reach event break
                    ReleaseMutex(mutex);
                    break;
                }
                printf("Run:%d\n", runner);
                ReleaseMutex(mutex);
                Sleep(10);
            }
            printf("Check Done:<<%d>>\n", data[2]);
            runner = 0;
            PulseEvent(event); // let all other threads continue
        }
        else { // if it is not last thread
            WaitForSingleObject(mutex, INFINITE);
            runner++;
            ReleaseMutex(mutex);
            printf("Wait:<<%d>>\n", data[2]);
            WaitForSingleObject(event, INFINITE); // wait till all other threads reach this stage
            printf("Exit:<<%d>>\n", data[2]);
        }
    }
    return 0;
}


int main()
{
    event = CreateEvent(NULL, TRUE, FALSE, NULL);
    mutex = CreateMutex(NULL, FALSE, NULL);
    SetEvent(event);
    int data[3] = {2,8}; //0 amount of threads //1 amount of numbers
    HANDLE t[10000];
    int ThreadData[1000][3];
    for (int i = 0; i < data[0]; i++) {
        memcpy(ThreadData[i], data, sizeof(int) * 2); // copy amount of threads and amount of numbers to the threads data
        ThreadData[i][2] = i; // creat threads id
        LPVOID ThreadsData = (LPVOID)(&ThreadData[i]);
        t[i] = CreateThread(0, 0, thread_fun, ThreadsData, 0, NULL);
        if (t[i] == NULL)return 0;
    }
    while (1) {
        DWORD res = WaitForMultipleObjects(data[0], t, true, 1000);
        if (res != WAIT_TIMEOUT) break;
    }
    for (int i = 0; i < data[0]; i++)CloseHandle(t[i]); // close all threads
    CloseHandle(event); // close event
    CloseHandle(mutex); //close mutex
    printf("Done");
}

The main idea is to wait until all threads except one reach the event and wait there, meanwhile the last thread must release them from waiting.

But the code doesn't work reliably. 1 in 10 times, it ends correctly, and 9 times just gets stuck in while(1). In different tries, printf in while (printf("Run:%d\n", runner);) prints different numbers of runners (0 and 3).

What can be the problem?

Upvotes: 1

Views: 559

Answers (1)

Andreas Wenzel
Andreas Wenzel

Reputation: 25386

As we found out in the comments section, the problem was that although the event was created in the initial state of being non-signalled

event = CreateEvent(NULL, TRUE, FALSE, NULL);

it was being set to the signalled state immediately afterwards:

SetEvent(event);

Due to this, at least on the first iteration of the loop, when j == 0, the first worker thread wouldn't wait for the second worker thread, which caused a race condition.

Also, the following issues with your code are worth mentioning (although these issues were not the reason for your problem):

  1. According to the Microsoft documentation on PulseEvent, that function should not be used, as it can be unreliable and is mainly provided for backward-compatibility. According to the documentation, you should use condition variables instead.
  2. In your function thread_fun, the last thread is locking and releasing the mutex in a loop. This can be bad, because mutexes are not guaranteed to be fair and it is possible that this will cause other threads to never be able to acquire the mutex. Although this possibility is mitigated by you calling Sleep(10); once in every loop iteration, it is still not the ideal solution. A better solution would be to use a condition variable, so that the thread only checks for changes of the variable runner when another thread actually signals a possible change. Such a solution would also be better for performance reasons.

Upvotes: 1

Related Questions