HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Thread-safe session manager

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managersessionthreadsafe

Problem

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 sessionLocks =
new ConcurrentDictionary();

private ConcurrentDictionary sessions =
new ConcurrentDictionary();

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(s

Solution

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.

Context

StackExchange Code Review Q#45655, answer score: 5

Revisions (0)

No revisions yet.