Bug in CThreadPool

Bob Etheridge reported a bug in the socket server code from codeproject, this is probably the oldest version of The Server Framework’s Free Framework code. He was noticing a small amount of memory corruption on server shutdown. I’ve narrowed it down to a bug in the CThreadPool class in the Win32 tools library. The bug is present in all versions of the class.

void CThreadPool::ThreadStopped(WorkerThread *pThread)
{
   ::InterlockedDecrement(&m_activeThreads);
   ::InterlockedDecrement(&m_initialisedThreads);
 
   RemoveThreadFromList(pThread);
 
   OnThreadStopped();
}

Should actually be

void CThreadPool::ThreadStopped(WorkerThread *pThread)
{
   ::InterlockedDecrement(&m_activeThreads);
   ::InterlockedDecrement(&m_initialisedThreads);
 
   OnThreadStopped();
 
   RemoveThreadFromList(pThread);
}

This function is called when worker threads shut down. When RemoveThreadFromList() is called for the last active thread an event is set and this causes the main thread to complete its wait for the thread pool to shut down cleanly and continue. This usually results in the thread pool object being destroyed. There’s a race condition between the destruction of the object and the completion of the final call to the virtual function OnThreadStopped().