Thread-safe session manager 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&lt;Guid, object&gt; sessionLocks =
        new ConcurrentDictionary&lt;Guid, object&gt;();

    private ConcurrentDictionary&lt;Guid, ASession&gt; sessions =
        new ConcurrentDictionary&lt;Guid, ASession&gt;();

    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 =&gt; 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; }
}
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.