ASP.NET/Static class Race Condition?

Question

I have an ASP.NET application with a lot of dynamic content. The content is the same for all users belonging to a particular client. To reduce the number of database hits required per request, I decided to cache client-level data. I created a static class ("ClientCache") to hold the data.
The most-often used method of the class is by far "GetClientData", which brings back a ClientData object containing all stored data for a particular client. ClientData is loaded lazily, though: if the requested client data is already cached, the caller gets the cached data; otherwise, the data is fetched, added to the cache and then returned to the caller.

Eventually I started getting intermittent crashes in the the GetClientData method on the line where the ClientData object is added to the cache. Here's the method body:

public static ClientData GetClientData(Guid fk_client)
{
 if (_clients == null)
  _clients = new Dictionary<Guid, ClientData>();

 ClientData client;
 if (_clients.ContainsKey(fk_client))
 {
  client = _clients[fk_client];
 }
 else
 {
  client = new ClientData(fk_client);
  _clients.Add(fk_client, client);
 }
 return client;
}

The exception text is always something like "An object with the same key already exists." Of course, I tried to write the code so that it just wasn't possible to add a client to the cache if it already existed.

At this point, I'm suspecting that I've got a race condition and the method is being executed twice concurrently, which could explain how the code would crash. What I'm confused about, though, is how the method could be executed twice concurrently at all. As far as I know, any ASP.NET application only ever fields one request at a time (that's why we can use HttpContext.Current).

So, is this bug likely a race condition that will require putting locks in critical sections? Or am I missing a more obvious bug?

Answer 1

If an ASP.NET application only handles one request at a time all ASP.NET sites would be in serious trouble. ASP.NET can process dozens at a time (typically 25 per CPU core).

You should use ASP.NET Cache instead of using your own dictionary to store your object. Operations on the cache are thread-safe.

Note you need to be sure that read operation on the object you store in the cache are threadsafe, unfortunately most .NET class simply state the instance members aren't thread-safe without trying to point any that may be.

Edit:

A comment to this answer states:-

Only atomic operations on the cache are thread safe. If you do something like check
if a key exists and then add it, that is NOT thread safe and can cause the item to
overwritten.

Its worth pointing out that if we feel we need to make such an operation atomic then the cache is probably not the right place for the resource.

I have quite a bit of code that does exactly as the comment describes. However the resource being stored will be the same in both places. Hence if an existing item on rare occasions gets overwritten the only the cost is that one thread unnecessarily generated a resource. The cost of this rare event is much less than the cost of trying to make the operation atomic every time an attempt to access it is made.

Answer 2

Your code really does assume only one thread is in the function at a time.

This just simply won't be true in ASP.NET

If you insist on doing it this way, use a static semaphore to lock the area around this class.

Answer 3

This is very easy to fix:

private _clientsLock = new Object();

public static ClientData GetClientData(Guid fk_client)
{
  if (_clients == null)
    lock (_clientsLock)
      // Check again because another thread could have created a new 
      // dictionary in-between the lock and this check
      if (_clients == null) 
        _clients = new Dictionary<Guid, ClientData>();

  if (_clients.ContainsKey(fk_client))
    // Don't need a lock here UNLESS there are also deletes. If there are
    // deletes, then a lock like the one below (in the else) is necessary
    return _clients[fk_client];
  else
  {
    ClientData client = new ClientData(fk_client);

    lock (_clientsLock)
      // Again, check again because another thread could have added this
      // this ClientData between the last ContainsKey check and this add
      if (!clients.ContainsKey(fk_client))
       _clients.Add(fk_client, client);

    return client;
  }
}

Keep in mind that whenever you mess with static classes, you have the potential for thread synchronization problems. If there's a static class-level list of some kind (in this case, _clients, the Dictionary object), there's DEFINITELY going to be thread synchronization issues to deal with.

Answer 4

you need thread safe & minimize lock.
see Double-checked locking (http://en.wikipedia.org/wiki/Double-checked%5Flocking)

write simply with TryGetValue.

public static object lockClientsSingleton = new object();

public static ClientData GetClientData(Guid fk_client)
{
    if (_clients == null) {
        lock( lockClientsSingleton ) {
            if( _clients==null ) {
                _clients = new Dictionary``();
            }
        }
    }
    ClientData client;
    if( !_clients.TryGetValue( fk_client, out client ) )
    {
        lock(_clients) 
        {
            if( !_clients.TryGetValue( fk_client, out client ) ) 
            {
                client = new ClientData(fk_client)
                _clients.Add( fk_client, client );
            }
        }
    }
    return client;
}