Is this code robust

[Tornation]

I have made multiple threads (no pun intended) regarding multi threading in the past. I believe it has been established that I'm confused and don't know what I'm doing. But today I come today with a bit of code for you guys to look at and judge.

 

Basically my question is could this system fail. (eg. A thread gets stuck in sleep)

 

Spoiler

#include <iostream>
#include <thread>
#include <mutex>

class ThreadController {
	std::mutex mtx;
	std::condition_variable shouldStart;
	std::condition_variable hasEnded;
	bool isRunning;
	bool shouldExit;
public:
	void SignalStart() {
		isRunning = true;
		shouldStart.notify_one();
	}

	void WaitForStart() {
		if (!isRunning) {
			std::unique_lock<std::mutex> lock(mtx);
			shouldStart.wait(lock);
			lock.unlock();
		}
	}

	void SignalEnd() {
		isRunning = false;
		hasEnded.notify_one();
	}

	void WaitForEnd() {
		if (isRunning) {
			std::unique_lock<std::mutex> lock(mtx);
			hasEnded.wait(lock);
			lock.unlock();
		}
	}

	void SignalExit() {
		shouldExit = true;
	}

	bool ShouldExit() const {
		return shouldExit;
	}
};

void func1(ThreadController * threadController) {
	do {
		threadController->WaitForStart();
		std::cout << "started" << std::endl;
		threadController->SignalEnd();
	} while (!threadController->ShouldExit());
}

void func2(ThreadController * threadController) {
	int i = 0;
	do {
		threadController->SignalStart();
		++i; //Obviously the following isn't actually what would be here
		if (i > 100) {
			threadController->SignalExit();
		}
		threadController->WaitForEnd();
		std::cout << "ended" << std::endl;
	} while (!threadController->ShouldExit());
}

int main() {
	ThreadController * tc = new ThreadController();
	std::thread t1(func1, tc);
	std::thread t2(func2, tc);
	t1.join();
	t2.join();
	delete tc;
}

 

[fizzlesticks]

There are a few race conditions and deadlocking issues. The main one being that shouldStart may be in a waiting state when the second thread exits which will cause it to sit and wait forever. In the simple case of this code that can be solved by adding a call to SignalStart after exiting the loop in the second thread, if the first thread is actually doing work you'd need to add some checks to see if it should exit early. Others involve data read/write races which can be fixed by adding more locking.

 

Condition variables have a problem called spurious wakeup. This means the wait() function may return before the cv has been notified. To fix this, the wait() function always needs to be done in a loop or by using the other overload which includes a predicate to check if the cv should actually have woken up.

 

The lock.unlock()s you have aren't necessary as the mutex will be unlocked automatically when the lock is destroyed. (you may want to keep them if you're doing work after waiting for the cv and the lock is no longer needed)

 

Here's some updated code that I believe won't have any issues. It may being doing extra locking that isn't necessary but that's better than deadlocks. Lines with //f were added or changed.

Spoiler

#include <iostream>
#include <thread>
#include <mutex>

class ThreadController {
    mutable std::mutex mtx;//f
    std::condition_variable shouldStart;
    std::condition_variable hasEnded;
    bool isRunning;
    bool shouldExit;
public:
    void SignalStart() {
        std::unique_lock<std::mutex> lock(mtx);//f
        isRunning = true;
        shouldStart.notify_one();
    }

    void WaitForStart() {
        std::unique_lock<std::mutex> lock(mtx);//f
        while (!isRunning && !shouldExit) {//f
            shouldStart.wait(lock);
        }
    }

    void SignalEnd() {
        std::unique_lock<std::mutex> lock(mtx);//f
        isRunning = false;
        hasEnded.notify_one();
    }

    void WaitForEnd() {
        std::unique_lock<std::mutex> lock(mtx);//f
        while (isRunning && !shouldExit) {//f
            hasEnded.wait(lock);
        }
    }

    void SignalExit() {
        std::unique_lock<std::mutex> lock(mtx);//f
        shouldExit = true;
    }

    bool ShouldExit() const {
        std::unique_lock<std::mutex> lock(mtx);//f
        return shouldExit;
    }
};

void func1(ThreadController * threadController) {
    do {
        threadController->WaitForStart();
        std::cout << "started" << std::endl;
        threadController->SignalEnd();
    } while (!threadController->ShouldExit());
}

void func2(ThreadController * threadController) {
    int i = 0;
    do {
        threadController->SignalStart();
        ++i; //Obviously the following isn't actually what would be here
        if (i > 100) {
            threadController->SignalExit();
        }
        threadController->WaitForEnd();
        std::cout << "ended" << std::endl;
    } while (!threadController->ShouldExit());
    threadController->SignalStart(); //f
}

int main() {
    ThreadController * tc = new ThreadController();
    std::thread t1(func1, tc);
    std::thread t2(func2, tc);
    t1.join();
    t2.join();
    delete tc;
}

 

Edit: Those unique_locks that aren't being used for condition_variables should be lock_guards, they're slightly faster if you don't need unlocking and relocking capabilities.

[Tornation]

4 minutes ago, fizzlesticks said:

Here's some updated code that I believe won't have any issues. It may being doing extra locking that isn't necessary but that's better than deadlocks. Lines with //f were added or changed.

<3