ReaderWriterLock, my naive implementation

Ron challenged me to write my own implementation of reader/writers lock. He did a great job of doing his own implementation.

I came up with this:
(note: this is really naive implementation, it’s far from ideal in terms of fairness and possible racing conditions. Just take it as brain-teaser)

   1: public class StateReaderWriterLock : IDisposable
   2:     {
   3:         private readonly AutoResetEvent _changeStateAutoLock = new AutoResetEvent(false);
   4:         private readonly AutoResetEvent _writerDone = new AutoResetEvent(false);
   5:         private readonly object _readersLock = new object();
   6:         private readonly object _writersLock = new object();
   7:         private int _readers;
   8:         private int _state; // 0 is "neutral", 1 is read, 2 is write
   9:         private int _writers; 
  10:  
  11:         public void LockReader()
  12:         {
  13:             while (true)
  14:             {
  15:                 // try to bring the state from "neutral" or "read" to "read". If the current state is "write", let's wait.
  16:                 while (Interlocked.CompareExchange(ref _state, 1, 0) == 2)
  17:                     _changeStateAutoLock.WaitOne(); 
  18:  
  19:                 // an interesting case here where the last reader is now in ReleaseRead and we're trying to read as well, we might be too late (the writer might have changed the _state already)
  20:                 // if that happens - we're back to square one, but we still want to avoid recursive locking!
  21:                 bool loseInRace = false;
  22:                 lock (_readersLock)
  23:                 {
  24:                     if (Interlocked.CompareExchange(ref _state, 1, 0) == 2)
  25:                         loseInRace = true;
  26:                     else
  27:                         _readers++;
  28:                 }
  29:                 if (!loseInRace)
  30:                     return; // success!
  31:             }
  32:         } 
  33:  
  34:         public void ReleaseReader()
  35:         {
  36:             lock (_readersLock)
  37:             {
  38:                 _readers--; 
  39:  
  40:                 // if I am the last reader, let's reset the state so any given reader/writer can take it
  41:                 if (_readers == 0)
  42:                 {
  43:                     Thread.VolatileWrite(ref _state, 0);
  44:                     _changeStateAutoLock.Set();
  45:                 }
  46:             }
  47:         } 
  48:  
  49:         public void LockWriter()
  50:         {
  51:             while (true)
  52:             {
  53:                 // try to bring the state from "neutral" or "write" to "write". If the current state is "read", let's wait.
  54:                 while (Interlocked.CompareExchange(ref _state, 2, 0) == 1)
  55:                     _changeStateAutoLock.WaitOne(); 
  56:  
  57:                 bool loseInRace = false;
  58:                 lock (_writersLock)
  59:                 {
  60:                     if (Interlocked.CompareExchange(ref _state, 2, 0) == 1)
  61:                         loseInRace = true;
  62:                     else
  63:                         _writers++;
  64:                 } 
  65:  
  66:                 if (!loseInRace)
  67:                     break; // great success!
  68:             } 
  69:  
  70:             // allow 1 writer only
  71:             if (Thread.VolatileRead(ref _writers) > 1)
  72:             {
  73:                 _writerDone.WaitOne();
  74:             }
  75:         } 
  76:  
  77:         public void ReleaseWriter()
  78:         {
  79:             lock (_writersLock)
  80:             {
  81:                 _writers--; 
  82:  
  83:                 // if I am the last writer, let's reset the state so any given reader/writer can take it
  84:                 if (_writers == 0)
  85:                 {
  86:                     Thread.VolatileWrite(ref _state, 0);
  87:                     _changeStateAutoLock.Set();
  88:                 }
  89:                 else
  90:                 {
  91:                     _writerDone.Set();
  92:                 }
  93:             }
  94:         } 
  95:  
  96:         public void Dispose()
  97:         {
  98:             _changeStateAutoLock.Close();
  99:             _writerDone.Close();
 100:         }
 101:     }
 102:  

Not sure it’s the greatest job interview question, but it is indeed challenging and fun to play with.

 

On Interlocked.Increment and volatile

Imagine you’ve got the following code, running in 2 separate threads T1 and T2:

private bool _go = true;

T1                                      T2
while (_go)                        // … some code here …
{                                         _go = false; // something happened, let’s stop
   // … do work
}

Even if T2 will set _go to false, this code might lead to endless loop in T1. Why is that?
Each CPU have a local memory called L1 Cache, that might cache the value of _go. Multiple threads running on multiple CPU’s can (and will) cache data and re-order instructions to optimize code. So if for example, T1 is running on processorA and T2 is running on processorB, we might have an endless loop here. A simple solution here is to add the volatile keyword on the _go field definition to assure order and avoid caching on CPU level:

“The volatile modifier is usually used for a field that is accessed by multiple threads without using the lock statement to serialize access. Using the volatile modifier ensures that one thread retrieves the most up-to-date value written by another thread.” (MSDN)

Now let’s examine another example, again 2 threads T1 and T2:

private int _writers = 0;

T1                                                         T2
while (_writers > 1)                              ….
{                                                           Interlocked.Increment(ref _writers);
    _someAutoEvent.WaitOne();
}

You can see that we’re using an atomic increment via Interlocked.Increment method, so we should be thread-safe here right? not quite.
We’ve got one thread that is reading (T1) and another thread that is writing (T2) to _writers. If T1 is running in processorA and T2 in processorB, the _writers value will be cached on the CPU level for some time which means T1 might see different value than T2. If T2 would have catch the returned value from Interlocked.Increment and it as the only reader of that field, then it was thread-safe. This is not the case here.
The solution here is to use Thread.VolatileRead(ref _writers) to make sure T1 gets the latest value. We could have used lock keyword as well of course to serialize access to _writers field.

Summary:

  • Although setting a word size variable is atomic (like in our first example), it doesn’t mean it is thread-safe! For “boolean flags” I would recommend using volatile as a clean and simple solution.
  • For “counters scenarios”, I would have used Interlocked with Thread.ReadVolatile. This should outperform lock usage and still keep your code neat and shiny.
  • The lock keyword is probably the safest way to avoid dangerous race conditions, so unless you’re sure about your solution, keep it simple and use lock.