Jump to content

  • Log In with Google      Sign In   
  • Create Account


#Actuale‍dd

Posted 10 September 2012 - 02:01 PM

void ThreadPool::TerminateAllWorkers()
{
  mMutex->Lock();
  mDesiredNumThreads = 0;
  mMutex->Unlock();
  mCondVar_NewTaskOrKillWorker->Broadcast();
  while (mNumThreads > 0)

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).

  {
   mLogger.LogInfo() << "TerminateAllWorkers::mCondVar_WorkDoneOrWorkerKilled->Wait()" << std::endl;
   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)
   {
	mLogger.LogInfo() << "Worker::mCondVar_NewTaskOrKillWorker->Wait()" << std::endl;
	mCondVar_NewTaskOrKillWorker->Wait();

And again here.

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

void ConditionVariable::Wait()
{
  mCondVarState = ConditionVariable::WAITING;
  #if defined _WIN32 || _WIN64
   WaitForSingleObject(mCondVarHandle, INFINITE);
  #else
   mMutex.Lock();
   pthread_cond_wait(&mCondVarHandle, &mMutex.GetNativeHandle());
   mMutex.Unlock();
  #endif
  mCondVarState = ConditionVariable::READY;
}
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.

Edited to add:

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
task *get_next_task()
{
    scoped_lock lock(mtx);

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

    if (state != running)
	    return 0;

    assert(!tasks.empty());
    task *ret = tasks.front();
    tasks.pop_front();

    return task;
}

void worker()
{
    while (true)
    {
	    task *next = get_next_task();
	    if (!next)
		    return;

	    next->run();
    }
}

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

    for each thread:
	    thread.join();
}

#1e‍dd

Posted 10 September 2012 - 01:45 PM

void ThreadPool::TerminateAllWorkers()
{
  mMutex->Lock();
  mDesiredNumThreads = 0;
  mMutex->Unlock();
  mCondVar_NewTaskOrKillWorker->Broadcast();
  while (mNumThreads > 0)

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).

  {
   mLogger.LogInfo() << "TerminateAllWorkers::mCondVar_WorkDoneOrWorkerKilled->Wait()" << std::endl;
   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)
   {
	mLogger.LogInfo() << "Worker::mCondVar_NewTaskOrKillWorker->Wait()" << std::endl;
	mCondVar_NewTaskOrKillWorker->Wait();

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();
   pthread_cond_wait(&mCondVarHandle, &mMutex.GetNativeHandle());
   mMutex.Unlock();
  #endif
  mCondVarState = ConditionVariable::READY;
}
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