EnterCriticalSection Deadlocking

Question

I found some code that claimed to be able to make a thread sleep for an accurate amount of time. Testing the code out, it seems to work great, however it always deadlocks after a short amount of time.

Here is the original code. I put prints before entering and leaving the critical section, and saw that sometimes it leaves or enters twice in a row. It seems to deadlock at the EnterCriticalSection call within the Wait function.

Is there a way I can modify this code to retain its functionality while not deadlocking?

//----------------------------------------------------------------
class PreciseTimer
{
public:
   PreciseTimer() : mRes(0), toLeave(false), stopCounter(-1)
   {
      InitializeCriticalSection(&crit);
      mRes = timeSetEvent(1, 0, &TimerProc, (DWORD)this,
                          TIME_PERIODIC);
   }
   virtual ~PreciseTimer()
   {
      mRes = timeKillEvent(mRes);
      DeleteCriticalSection(&crit);
   }

   ///////////////////////////////////////////////////////////////
   // Function name   : Wait
   // Description     : Waits for the required duration of msecs.
   //                 : Timer resolution is precisely 1 msec
   // Return type     : void  :
   // Argument        : int timeout : timeout in msecs
   ///////////////////////////////////////////////////////////////
   void Wait(int timeout)
   {
      if ( timeout )
      {
         stopCounter = timeout;
         toLeave = true;
         // this will do the actual delay - timer callback shares
         // same crit section
         EnterCriticalSection(&crit);
         LeaveCriticalSection(&crit);
      }
   }
   ///////////////////////////////////////////////////////////////
   // Function name   : TimerProc
   // Description     : Timer callback procedure that is called
   //                 : every 1msec
   //                 : by high resolution media timers
   // Return type     : void CALLBACK  :
   // Argument        : UINT uiID :
   // Argument        : UINT uiMsg :
   // Argument        : DWORD dwUser :
   // Argument        : DWORD dw1 :
   // Argument        : DWORD dw2 :
   ///////////////////////////////////////////////////////////////
   static void CALLBACK TimerProc(UINT uiID, UINT uiMsg, DWORD
                                  dwUser, DWORD dw1, DWORD dw2)
   {
      static volatile bool entered = false;

      PreciseTimer* pThis = (PreciseTimer*)dwUser;
      if ( pThis )
      {
         if ( !entered && !pThis->toLeave )   // block section as
                                              // soon as we can
         {
            entered = true;
            EnterCriticalSection(&pThis->crit);
         }
         else if ( pThis->toLeave && pThis->stopCounter == 0 )
                                              // leave section
                                              // when counter
                                              // has expired
         {
            pThis->toLeave = false;
            entered = false;
            LeaveCriticalSection(&pThis->crit);
         }
         else if ( pThis->stopCounter > 0 )   // if counter is set
                                              // to anything, then
                                              // continue to drop
                                              // it...
            --pThis->stopCounter;
      }
   }

private:
   MMRESULT         mRes;
   CRITICAL_SECTION crit;
   volatile bool    toLeave;
   volatile int     stopCounter;
};

Answer 1

A deadlock in EnterCriticalSection() usually means that another thread called EnterCriticalSection() but never called LeaveCriticalSection().

As shown, this code is not very thread-safe (and timeSetEvent() is a threaded timer). If multiple PreciseTimer timers are running at the same time, they are using the same TimerProc() callback, and thus are sharing the same entered variable without protecting it from concurrent access. And if multiple threads call Wait() on the same PreciseTimer object at the same time, they are going to step over each other's use of the stopCounter and toLeave members, which are also not protected them from concurrent access. Even a single thread calling Wait() on a single PreciseTimer is not safe since TimerProc() runs in its own thread and stopCounter is not adequately protected.

This code is full of race conditions.