Thread-safe session manager

Question

Expected behavior of the code below:

• There will be one static SessionManager accessed by many threads.
• Multiple calls to SessionManager.UseTheSession for a given session ID must be processed on a first come first serve basis (similarly, calling EndSession should allow any currently executing UseTheSession's to finish).
• The goal of the individual session locks is that a call to SessionManager.UseTheSession by one thread doesn't hold up another thread that is calling the same method with a different session ID.

Things I'm looking for input on are:

• Will this perform as expected.
• Does sessions need to be a ConcurrentDictionary or does a regular Dictionary work fine here since I'm locking any access of the one key.
• What's a good way to handle the memory leak that happens when I keep creating new locks but don't remove them for a given id after I stop using that it?

The code (runnable in Linqpad with the inclusion of the System.Collections.Concurrent namespace):

void Main()
{
 var sm = new SessionManager();
 Guid id = sm.StartSession();
 sm.UseTheSession(id).Dump();
 sm.UseTheSession(id).Dump();
 sm.EndSession(id);
}
public class SessionManager
{
 private ConcurrentDictionary<Guid, object> sessionLocks =
 new ConcurrentDictionary<Guid, object>();
 private ConcurrentDictionary<Guid, ASession> sessions =
 new ConcurrentDictionary<Guid, ASession>();
 public Guid StartSession()
 {
 Guid id = Guid.NewGuid();
 // Takes a sec to create the session.
 var session = new ASession(string.Format("Session {0}", id));
 Thread.Sleep(1000);
 if(!sessions.TryAdd(id, session))
 throw new Exception("Astronomically unlikely situation encountered.");
 return id;
 }
 public int UseTheSession(Guid id)
 {
 lock(this.GetSessionLocker(id))
 {
 ASession session;
 if(sessions.TryGetValue(id, out session))
 {
 return this.DoSomethingWithSession(session);
 }
 else
 {
 throw new Exception(string.format("Session with id {0} does not exist.",
 id));
 }
 }
 }
 public void EndSession(Guid id)
 {
 lock(this.GetSessionLocker(id))
 {
 ASession removedSession;
 if(sessions.TryRemove(id, out removedSession))
 {
 this.CleanUpSessionRemnants(removedSession);
 }
 }
 }
 private object GetSessionLocker(Guid id)
 {
 return sessionLocks.GetOrAdd(id, x => new object());
 }
 private int DoSomethingWithSession(ASession session)
 {
 Thread.Sleep(1000);
 return 1;
 }
 private void CleanUpSessionRemnants(ASession session)
 {
 Thread.Sleep(1000);
 }
}
public class ASession
{
 public ASession(string name)
 {
 this.Name = name;
 }
 public string Name { get; private set; }
}

Answer 1

A few notes to address your specific questions:

Will this perform as expected?

Only you can answer that after testing it thoroughly.

Does sessions need to be a ConcurrentDictionary or does a regular Dictionary work fine here since I'm locking any access of the one key.

As the lock keys are different for different sessions, it is a whole lot more safe to use ConcurrentDictionary. That way you are at least sure that the dictionary won't screw up.

However, your approach of the sessionLocks map I have some doubts about. What you really seem to want to synchronize is the return this.DoSomethingWithSession(session); and this.CleanUpSessionRemnants(removedSession); calls, so I would place the lock only around those.

Speaking of placing the locks, consider placing them within the ASession class. In fact, I would try to move the CleanUpSessionRemnants and DoSomethingWithSession methods to the ASession class. If it needs the SessionManager object to perform its job, then pass the SessionManager as a parameter to the method. Right now your ASession seems to be only a String, make better use of that class. I think you created it for a reason.

What's a good way to handle the memory leak that happens when I keep creating new locks but don't remove them for a given id after I stop using that it?

Don't create them in the first place. Really. Or at least remove them when you're done. See above. Or the code below.

if(sessions.TryRemove(id, out removedSession))
{
 sessionLocks.TryRemove(id);
 this.CleanUpSessionRemnants(removedSession);
}

• Ocelot20: Good points. This was a contrived example similar to what I'm doing, so ASession definitely holds more than just a name :).

Answer 2

In this answer I explain in details why one should not throw System.Exception. You should be throwing InvalidOperationException in the case of the astronomically unlikely situation, and probably an ArgumentException in the case of the non-existing session.

As far as thread safety is concerned, I don't write multithreaded code very often so I might be completely wrong, but I since ConcurrentDictionary is a thread-safe IDictionary implementation, sessions.TryGetValue(id, out session) is already a thread-safe call, doesn't need to be wrapped in a lock block.

• Simon Forsberg: You have many good points there, but the reason for the lock block seems to be to prevent multiple threads from simultaneously calling DoSomethingWithSession for the same session.
• Mathieu Guindon: @SimonAndréForsberg that's right, thanks - edited out that part ;)
• Ocelot20: Good points about System.Exception. I wasn't really thinking much about the exceptions when putting this example together but I'll be sure to put more thought into the real version of the code.