ConcurrentDictionary & atomic operations- sometimes Lock is needed?

Question

Let's say I have this simple code : (simplification)

Where MyConCurrentDictionary is a static ConcurrentDictionary<string, string> ( which resides in a different class).

/*1*/   public void Send(string Message, string UserName)
/*2*/   {            
/*3*/       string ConnectionId;
/*4*/       if (MyConCurrentDictionary.TryGetValue(UserName,out ConnectionId))
/*5*/       {
/*6*/       //...   
/*7*/           DB.InsertMessage( Message, ConnectionId);
/*8*/           LOG.LogMessage  ( Message, ConnectionId);
/*9*/       //...
/*10*/       }
/*11*/       else ...
/*12*/   }

This method runs from many instances. ( signalR hub , if you will)

I need to insert to DB / log ONLY if the user/connectionID exists.

Ok so line #4 is thread safe when multi threads are accessing the ConcurrentDictionary

But there is another method which is RemoveUser - which removes the user from dictionary :

 public void RemoveUser (string userName)
        {
           string removed;
           if ( MyConCurrentDictionary.TryRemove(userName,out removed ))
              Clients.All.removeClientfromChat(userName);
        }

But it possible that contexts will occurs in line #5 which will do RemoveUser and REMOVE the UserName from the ConcurrentDictionary.

So in order to solve this - the code would be something like this :

lock(locker)
{
  if (MyConCurrentDictionary.TryGetValue(UserName,out ConnectionId))
   {
    //...
   }

}

Which completely defeats the purpose of my ConcurrentDictionary.

Question

What is the right way of doing such thing in a multithreaded environment while - still having the benefit of ConcurrentDictionary ?

nb , Yes ConcurrentDictionary is thread safe only for operation within the Dictionary. but what im trying to say is that with specific scenario , I loose the benefit of ConcurrentDictionary because I still need to use lock.( and I might be wrong about it).

Answer 1

Based on what you have written and what I understand so far: ConcurrentDictionary is not the appropriate choice!

Write a class which wraps an ordinary Dictionary and encapsulate the read and write operations by an ReaderWriterLockSlim. This is much faster than an ordinary lock. Also you can have multiple read but only a single write operation at a time.

private ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();

public void DoRead(string xyz)
{
    try
    {
        _lock.EnterReadLock();
        // whatever
    }
    finally
    {
        _lock.ExitReadLock();
    }
}

public void DoWrite(string xyz)
{
    try
    {
        _lock.EnterWriteLock();
        // whatever
    }
    finally
    {
        _lock.ExitWriteLock();
    }
}

Answer 2

It looks like design issue.

You have ConcurentDictionary, but you don't use it atomically, there is an operation which consist of several commands, so that you have to use lock (or other synchronization).

Simplest solution is to allow LOG to have message with already deleted user id, in other words, process this cas (use TryGetValue from MyConcurentDictionary) whenever you processing LOG messages.

I could also think about having 2 collections: one where you have user id which is valid and other, where you have deleted user. When you want to add to LOG, then both collections are checked. When you want to delete user, then you add that user into deleted user collection and after some time you delete that user from valid collection (some time = 1 second or whatever).