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).

"REMOVE the UserName from the ConcurrentDictionary" I dont see any such code at #5. — Magnus, May 19 '14 at 07:49
Show some more code. Where do you remove? `ConcurrentDictionary` has only `TryRemove` method which is also thread safe. So that shouldn't be your problem. What is your problem? — Sriram Sakthivel, May 19 '14 at 07:50
@SriramSakthivel I know the public operations are threadsafe. but still everything can still be OK , but another thread can step in in the line#5 quanta and do remove ( becuase the user logs out) - and so I need atomicy here. — Royi Namir, May 19 '14 at 07:52
@RoyiNamir You still did not answer where the remove happens! — toATwork, May 19 '14 at 07:53
@Magnus It is possible that when context switch occures in line #5 , a user would quickly logs out and entry removed , and then when the thread comes back - there would be a problem. — Royi Namir, May 19 '14 at 07:53
@RoyiNamir Can RemoveUser be called at any time? Does logging remove the user by any chance? What is the problem with logging, if the user was logged on, when you started logging? — toATwork, May 19 '14 at 08:01
Why does it matter if the user was logged out? Were are talking about milliseconds here. Unless `DB.InsertMessage` will crash if there is no user. — Magnus, May 19 '14 at 08:05
@toATwork yes. I mentioned this in my question :-) — _But it possible that contexts will occurs in line #5_ I dont want to log if the condition is not true — Royi Namir, May 19 '14 at 08:05
@Magnus it is a simplification of the problem. there are some things which I can't do if its not relevant anymore. ( like loging a user without knowing it was removed) — Royi Namir, May 19 '14 at 08:06

score 4 · Accepted Answer · answered May 19 '14 at 09:07

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();
    }
}

score 1 · Answer 2 · answered May 19 '14 at 08:17

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).

This sample is a simplification . there some things that can't happen if they aren't relevant anymore. I dont see how a lock could NOt be used here. — Royi Namir, May 19 '14 at 08:28
*I dont see how a lock could NOt be used here*, that's why I wrote the answer. Can you use either solution from it? — Sinatr, May 19 '14 at 08:31

ConcurrentDictionary & atomic operations- sometimes Lock is needed?

2 Answers2