Cook Computing

After looking at Jeff Richter's Optex version of a critical section I realized there is a bug in the Leave function of my CritSect class: the curtid member must be set to 0 whenever a thread leaves the CS and before the event is signalled. This avoids the following race condition:

1. thread 0xaa leaves the CS with curtid set to 0xaa
2. thread 0xbb enters the CS by performing the InterlockedIncrement but is pre-empted before setting curtid
3. thread 0xaa attempts to enter the CS and finds curtid still set to 0xaa and so just increments depth - WRONG

The modified code is:

void CritSect::Leave()
{
  if (depth)
  {
    depth--;
    InterlockedDecrement(&cntr);
  }
  else
  {
    InterlockedExchange(&curtid, 0);
    if (InterlockedDecrement(&cntr) > 0)
      SetEvent(hevt);
  }
}

The faulty version of Leave passed my tests which just goes to demonstrate why the XP approach of writing tests before coding doesn't apply to multi-threaded code.

In the Optex class Richter doesn't use InterlockedCompareExchange to test whether a thread owns the CS:

if (m_pSharedInfo->m_dwThreadId == dwThreadId) { 

At first sight this seems wrong to me. If this operation is atomic then surely setting the value of m_dwThreadId is also atomic, yet he uses InterlockedExchange for that? I suspect the use of InterlockedExchange ensures that it is safe to make the comparision because InterlockedExchange prevents a read from occuring while the exchange is taking place?