• Create Account

Need scary sound effects or creepy audio loops for your next horror-themed game? Check out Highscore Vol.3 - The Horror Edition in our marketplace. 50 sounds and 10 loops for only \$9.99!

### #Actuale‍dd

Posted 10 September 2012 - 02:01 PM

void ThreadPool::TerminateAllWorkers()
{
mMutex->Lock();
mMutex->Unlock();


Here, you're accessing mNumThreads without having locked the associated mutex, which presumably is mMutex.

Also, consider using the scoped-locking pattern. You'll find it in pretty much any C++ threads library (for good reason).

  {
mCondVar_WorkDoneOrWorkerKilled->Wait();


Waiting on a condition variable should internally unlock a mutex and ensure it is locked when the wait call returns. Your implementation doesn't seem to be doing that, here.

...

// terminate if requested, otherwise wait for new task
if (running)
{


And again here.

I would look at the use of condition variables closer, but the Windows implementation below doesn't look at all correct

void ConditionVariable::Wait()
{
mCondVarState = ConditionVariable::WAITING;
#if defined _WIN32 || _WIN64
WaitForSingleObject(mCondVarHandle, INFINITE);
#else
mMutex.Lock();
mMutex.Unlock();
#endif
}
void ConditionVariable::Signal()
{
#if defined _WIN32 || _WIN64
SetEvent(mCondVarHandle);
ResetEvent(mCondVarHandle);


For example, here SetEvent could wake a waiting thread, which could then immediately wait on the condition variable again and be instantly let through, before ResetEvent is called. This is not the correct behaviour. Upon waiting again, the thread should block until a subsequent signal or broadcast awakens it.

If you are targeting Windows Vista and later, it's probably worth looking at Windows' CONDITION_VARIABLE structure and the associated functions. If you need to implement a condition variable from scratch, this informal paper (PDF) is pleasantly readable and points out the pitfalls in some other attempts.

Also, for the mutex you'll almost certainly be better served by Windows' CRITICAL_SECTION structure rather than a full mutex HANDLE, as locking and unlocking CRITICAL_SECTIONS require dipping in to the kernel far less often. You could also look at the slim reader/writer locks, but they are again Vista and onwards.

You might also be well-served by separating-out the part of your Worker() function that needs to do synchronization from the part that's just churning through tasks. I don't know the rest of your code at all, but it seems more complicated than it might otherwise need to be.

// Roughly...

// Returns null if the ThreadPool wants us to stop processing
{
scoped_lock lock(mtx);

while (tasks.empty() && state == running)
condvar.wait(lock);

if (state != running)
return 0;

}

void worker()
{
while (true)
{
if (!next)
return;

next->run();
}
}

void stop_processing()
{
{
scoped_lock lock(mtx);
state = stopped;
}

}


### #1e‍dd

Posted 10 September 2012 - 01:45 PM

void ThreadPool::TerminateAllWorkers()
{
mMutex->Lock();
mMutex->Unlock();


Here, you're accessing mNumThreads without having locked the associated mutex, which presumably is mMutex.

Also, consider using the scoped-locking pattern. You'll find it in pretty much any C++ threads library (for good reason).

  {
mCondVar_WorkDoneOrWorkerKilled->Wait();


Waiting on a condition variable should internally unlock a mutex and ensure it is locked when the wait call returns. Your implementation doesn't seem to be doing that, here.

...

// terminate if requested, otherwise wait for new task
if (running)
{


And again here.

I would look at the use of condition variables closer, but the Windows implementation below doesn't look at all correct

void ConditionVariable::Wait()
{
mCondVarState = ConditionVariable::WAITING;
#if defined _WIN32 || _WIN64
WaitForSingleObject(mCondVarHandle, INFINITE);
#else
mMutex.Lock();
mMutex.Unlock();
#endif
}
void ConditionVariable::Signal()
{
#if defined _WIN32 || _WIN64
SetEvent(mCondVarHandle);
ResetEvent(mCondVarHandle);


For example, here SetEvent could wake a waiting thread, which could then immediately wait on the condition variable again and be instantly let through, before ResetEvent is called. This is not the correct behaviour. Upon waiting again, the thread should block until a subsequent signal or broadcast awakens it.

If you are targeting Windows Vista and later, it's probably worth looking at Windows' CONDITION_VARIABLE structure and the associated functions. If you need to implement a condition variable from scratch, this informal paper (PDF) is pleasantly readable and points out the pitfalls in some other attempts.

Also, for the mutex you'll almost certainly be better served by Windows' CRITICAL_SECTION structure rather than a full mutex HANDLE, as locking and unlocking CRITICAL_SECTIONS require dipping in to the kernel far less often. You could also look at the slim reader/writer locks, but they are again Vista and onwards.

PARTNERS