This code smells

private void cacheUser(User user)
{

   userLock.AcquireReaderLock(Timeout.Infinite);

   try
   {
      if (! _users.TryGetValue(user.Email, out user))
      {
         userLock.UpgradeToWriterLock(Timeout.Infinite);
         _users.Add(user.Email, user);
      }
   }
   finally
   {
      userLock.ReleaseLock();
   }

}

A couple of questions regarding the above code.

1) Is the locking being used correctly? How can I keep track of the locks? Should I use Monitor instead?

2) The collection stores a User object, but uses User.Email as the collection key, seems counter intuitive to me, any suggestions?

I don't have much experience with ReaderWriterLock, but to answer your second question: I see no problem with this. To use a property of an object as the key in a collection is something I have also done before. In this case, an e-mail address is pretty unique, and is a good choice for a key. I don't know in what situation this code is used, but I'm guessing it's a web application of some kind.

Thanks for the speedy reply.
It is actually for a Messenger add-in.

I'm working on a custom implementation of group chats, so the collection stores all the participants.

The issue I see with this block of code is that it does not protect itself from itself… Whenever you get into the realm of locking resources when dealing with threads it is wise to verify that u can indeed lock the resource.. Example let say somehow you got this code to execute twice, how does it know that you have already locked or unlock the resource? Well the answer based on this code is it does not, it has no clue as to whether user is currently being used on another thread or not, which will cause some nasty error that will make you smash ahead against the wall. So what’s the answer to this issue? Well once again .net has done the work for you, in what they call an Interlocked, the easy way of looking at this is it compares values and depending on the returned value will indicate whether there is a lock currently on the method or not.  So how would this work into your example is as following:

private static int currentlyinuse = 0;

private void cacheUser(User user)
{
   if (0 == Interlocked.Exchange(ref currentlyinuse, 1))
   {
      userLock.AcquireReaderLock(Timeout.Infinite);
      try
      {
        if (!_users.TryGetValue(user.Email, out user))
        {
           userLock.UpgradeToWriterLock(Timeout.Infinite);
            _users.Add(user.Email, user);
        }
      }
      finally
      {
          userLock.ReleaseLock();
          Interlocked.Exchange(ref currentlyinuse, 0);
       }
   }
}

Actually your code looks correct to me.

Now a ReaderWriter lock is used when multiple threads need to read and write to the same datastructure. The idea is to allow multiple readers at a time to increase performance while a writer will need exclusive access to ensure consistency - e.g. what happens if the thread doing the writing has temporarily left the collection in an invalid state and some other thread tries to read from the collection? Nobody knows - it could crash, it could get the wrong value and so on.

One thing though: I would probably wrap the collection in a thread-safe wrapper. Otherwise you will have to get the locking correct everywhere you read/write from the collection - in this case retrieve an item, test for existence, add an item.

If there are n threads accessing this piece of code, all the threads could acquire the readlock. None of the threads may upgrade to a writelock as there are multiple readers holding onto the lock.

This code could deadlock

test-page wrote:



If there are n threads accessing this piece of code, all the threads could acquire the readlock. None of the threads may upgrade to a writelock as there are multiple readers holding onto the lock.

This code could deadlock

Ah, yes. Good catch. But you wouldn't even need multi-thread, would you? Would this deadlock on the very first thread?

Aahh..UpgradeToWriterLock...  needs multi-thread

Also...

"userLock"
"_users"

Seems to be using different conventions

Minh wrote:



test-page wrote:  If there are n threads accessing this piece of code, all the threads could acquire the readlock. None of the threads may upgrade to a writelock as there are multiple readers holding onto the lock. This code could deadlock

Ah, yes. Good catch. But you wouldn't even need multi-thread, would you? Would this deadlock on the very first thread?

Aahh..UpgradeToWriterLock...  needs multi-thread

Also...

"userLock"
"_users"

Seems to be using different conventions

Yeah, I need to fix the naming convention.
As for the other stuff, what exactly do I need to do, I'm a bit confused here.

yellow += ???

Here's the problem

Thread # 1

private void cacheUser(User user) --> Called w/ any "user"
{

   userLock.AcquireReaderLock(Timeout.Infinite); --> First thread here, so no prob, will get the lock

...

-----------
In the mean time...

Thread # 2

private void cacheUser(User user) --> Called w/ a new user
{

   userLock.AcquireReaderLock(Timeout.Infinite); --> 2nd thread here, but since 1st thread only has a Read lock, so no prob, got the Read lock

   try
   {
      if (! _users.TryGetValue(user.Email, out user)) --> New user, so, will execute if block
      {
         userLock.UpgradeToWriterLock(Timeout.Infinite); --> Uh oh, thread # 1 already has a Read lock in progress, so you'll NEVER be able up upgrade thread # 2's Read lock to a Write lock... this will pause here indefintely

Minh wrote:



Here's the problem

Thread # 1

private void cacheUser(User user) --> Called w/ any "user"
{

   userLock.AcquireReaderLock(Timeout.Infinite); --> First thread here, so no prob, will get the lock

...

-----------
In the mean time...

Thread # 2

private void cacheUser(User user) --> Called w/ a new user
{

   userLock.AcquireReaderLock(Timeout.Infinite); --> 2nd thread here, but since 1st thread only has a Read lock, so no prob, got the Read lock

   try
   {
      if (! _users.TryGetValue(user.Email, out user)) --> New user, so, will execute if block
      {
         userLock.UpgradeToWriterLock(Timeout.Infinite); --> Uh oh, thread # 1 already has a Read lock in progress, so you'll NEVER be able up upgrade thread # 2's Read lock to a Write lock... this will pause here indefintely

OK, but after some time thread 1 will release it's lock and thread 2 will be able to upgrade to writer lock. Moreover most reasonable implementations of R/W locks prioritize write locks so that when there's a thread waiting on the write lock, no other thread will be granted read lock, until the write lock is acquired and released.

Or am I missing something here?

I thought of a scenario where you can get into a deadlock:

Thread # 1

private void cacheUser(User user) --> Calls w/ new user
{

   userLock.AcquireReaderLock(Timeout.Infinite); --> OK

   try
   {
      if (! _users.TryGetValue(user.Email, out user)) --> Will execute if block
      {

(Thread switch)

Thread # 2

private void cacheUser(User user) --> Calls w/ new user
{

   userLock.AcquireReaderLock(Timeout.Infinite); --> OK

   try
   {
      if (! _users.TryGetValue(user.Email, out user)) --> Will execute if block
      {

         userLock.UpgradeToWriterLock(Timeout.Infinite); --> Will wait until thread # 1 release its lock

(Thread switch)

Thread # 1 (continuing the if block)

         userLock.UpgradeToWriterLock(Timeout.Infinite); --> Will wait until thread # 2 release its lock

***Deadlock***

A agree with blackstar:  Usually (and I don't know the .NET internals), calls to acquire (or upgrade) R/W locks are queued so that all requests get satisfied so long as all threads *eventually* release their locks.  Remember that once there is no thread writing, that all requests for reader locks are summarily granted.  If a new request for a writer lock is received, it AND any new reader requests block until all the existing reader threads are released.  If those new reader requests weren't also blocked, requests for writer locks might never be granted.

One other issue I see in the code example is that once the writer lock is granted, you need to see whether the data has already been added -- perhaps by another thread also looking for the same resource, which got the writer lock while this thread was blocked:

  try
   {
      if (! _users.TryGetValue(user.Email, out user))
      {
         userLock.UpgradeToWriterLock(Timeout.Infinite);
         if(!_users.ContainsKey(user.email)
         {
            _users.Add(user.Email, user);
         }
      }
   }
   finally
   {
      userLock.ReleaseLock();
   }

Minh wrote:


I thought of a scenario where you can get into a deadlock:

It won't deadlock. As the documentation says: "When a thread calls UpgradeToWriterLock the reader lock is released, regardless of the lock count, and the thread goes to the end of the queue for the writer lock."

Calling UpgradeToWriterLock releases the reader lock, so the deadlock situation doesn't occur.

Erik is right about the need to double check though. That is in fact a common pattern called, rather unimaginitively, "double-checked locking".

EDIT: And yes, writer locks get priority over reader locks. As long as there are writer locks in the queue, no reader locks will be granted.

Besides the double-checked locking I see no problems with the original code.

Sven Groot wrote:



Minh wrote: I thought of a scenario where you can get into a deadlock:

It won't deadlock. As the documentation says: "When a thread calls UpgradeToWriterLock the reader lock is released, regardless of the lock count, and the thread goes to the end of the queue for the writer lock."

Calling UpgradeToWriterLock releases the reader lock, so the deadlock situation doesn't occur.

Maybe I misread this... but it doesn't make sense to me. If I interpret this right, you're saying that when a thread is trying to upgrade to a writer lock, clearly, it would release its own read lock.

But it can't make any assumptions about  other threads' read locks -- can't it?

If there are other read locks beside its own, it can't possibly upgrade. Can't it?

Minh wrote:



Sven Groot wrote:  Minh wrote:  I thought of a scenario where you can get into a deadlock: It won't deadlock. As the documentation says: "When a thread calls UpgradeToWriterLock the reader lock is released, regardless of the lock count, and the thread goes to the end of the queue for the writer lock." Calling UpgradeToWriterLock releases the reader lock, so the deadlock situation doesn't occur.

Maybe I misread this... but it doesn't make sense to me. If I interpret this right, you're saying that when a thread is trying to upgrade to a writer lock, clearly, it would release its own read lock.

But it can't make any assumptions about  other threads' read locks -- can't it?

If there are other read locks beside its own, it can't possibly upgrade. Can't it?

Not straight away, no. But it will release the read lock, and then start waiting until it can get a writer lock. Your deadlock scenario when two threads simultaneously call UpgradeToWriterLock would only deadlock if those threads still hold the reader lock while waiting for the writer lock, which they don't.

There would only be a problem if a thread starts waiting for writer lock while still holding the reader lock. But it doesn't, since UpgradeToWriterLock releases the reader lock first. Thus in this piece of code no thread will ever be waiting for a lock while holding a lock, thus it cannot deadlock.

Sven Groot wrote:


Not straight away, no. But it will release the read lock, and then start waiting until it can get a writer lock. Your deadlock scenario when two threads simultaneously call UpgradeToWriterLock would only deadlock if those threads still hold the reader lock while waiting for the writer lock, which they don't.

Aahh. OK. This makes more sense now.

A write lock request must have a higher priority than any read lock request. I didn't realize there was a priority system in there.

Thanks Sven.

Sven saves the day.

Thanks, so what is the need for the inerlocked example as cited earlier in this thread? I thought I had seen you using interlocked code somewhere as well.

Any clarifications would be appreciatted, but I really do appreciatte the contributions to wit so far.

There is one more bug in this example. If TryGetValue fails, it will set its out parameter to null, thus the subsequent call to Add will fail with a NullReferenceException when it tries to access user.Email.
Here's the corrected code.

private void cacheUser(User user)
{
   _userLock.AcquireReaderLock(Timeout.Infinite);
   try
   {
      User user2;
      if( !_users.TryGetValue(user.Email, out user2) )
      {
         _userLock.UpgradeToWriterLock(Timeout.Infinite);
         if( !_users.TryGetValue(user.Email, out user2) )
           _users.Add(user.Email, user);
      }
   }
   finally
   {
      _userLock.ReleaseLock();
   }
}

The reason for the double check is as follows. If two threads get called with the same user, the following could happen without double checking.

Thread 1:
   _userLock.AcquireReaderLock(Timeout.Infinite);
Reader lock acquired, context switch.

Thread 2:
   _userLock.AcquireReaderLock(Timeout.Infinite);
Reader lock acquired.
   try
   {
      User user2;
      if( !_users.TryGetValue(user.Email, out user2) )
      {
         _userLock.UpgradeToWriterLock(Timeout.Infinite);
Reader lock released, starts waiting for writer lock. Context switch.

Thread 1:
   try
   {
      User user2;
      if( !_users.TryGetValue(user.Email, out user2) )
      {
         _userLock.UpgradeToWriterLock(Timeout.Infinite);
Reader lock released, starts waiting for writer lock. Context switch.

Thread 2:
Gets the writer lock
         _users.Add(user.Email, user);
      }
   }
   finally
   {
      _userLock.ReleaseLock();
   }
Writer lock released. Context switch

Thread 1:
Gets the writer lock
         _users.Add(user.Email, user);
Argument exception thrown; key already exists.

The extra check after taking the writer lock prevents the same user accidentally getting added twice. You still check check before taking the lock to prevent needlessly taking the lock.

You see this double-checked locking pattern a lot with singleton implementations, e.g.:

public static MySingleton GetInstance()
{
  if( _instance == null )
  {
    lock( _instanceLock )
    {
      if( _instance == null )
       _instance = new MySingleton();
    }
  }
  return _instance;
}

(Note that there are compiler optimisations that can be done that make double-checked locking for singleton creation incorrect; this doesn't happen in .Net, but I believe it does happen in Java and it can happen in C++ depending on what compiler you are using. That however doesn't apply to the dictionary example and isn't really the point of this thread. If you're more interested you can see here. Note once again that the .Net CLR doesn't behave like java and double-checked locking is safe in .Net.).