Use SemaphoreSlim to control access to a resource

Question

We have an existing situation in an MVC ASP.NET app where it's possible for two threads to come back asynchronously, one from an external api(the payment gateway) and one from within the browser for each cart in an online shop. Only one should be able to run some further code, but they can come back almost simultaneously. We have something in place that kind of works, but is not working correctly all the time. The following code has been suggested to replace it.

using System;
using System.Collections.Generic;
using System.Threading;


namespace semaphoreTest.Semaphore
{
    public class NxSemaphore : SemaphoreSlim
    {
        public NxSemaphore() : base(1, 1){}
        public DateTime Created { get; set; }
        public DateTime LastUsed { get; set; }
        public string Key { get; set; }
    }


    public static class AppSemaphoreDict
    {
        private static Dictionary<string, NxSemaphore> _cartSemaphores = new Dictionary<string, NxSemaphore>();
        private static object _lockObj = new object();

        public static NxSemaphore GetForCart(string nxID)
        {
            lock (_lockObj)
            {
                NxSemaphore semaphore;
                if (_cartSemaphores.TryGetValue(nxID, out semaphore))
                {
                    semaphore.LastUsed = DateTime.Now;
                }
                else
                {
                    semaphore = new NxSemaphore()
                    {
                        Created = DateTime.Now,
                        Key = nxID,
                        LastUsed = DateTime.Now
                    };
                    _cartSemaphores.Add(semaphore.Key, semaphore);
                }
                return semaphore;
            }
        }

    }
}

The idea is that a semaphore should be created for each cart (which has a unique id - NxId) and would be used in a manner below

public async Task<string> DoStuffDict()
{
    NxSemaphore semaphore = null;
    try
    {
        //get a random cart id for testing
        string NxGuid = SempahoreTest.Models.RandomNxGuid.GetRestrictedNxGuid(300);
        semaphore = AppSemaphoreDict.GetForCart(NxGuid);

        await semaphore.WaitAsync();
        //Get some stuff from an api
        NxApi api = new NxApi(NxRequestContext);
        var data = await api.GetSomeStuff;
        //write some stuff to the database
        using (DataContext dbContext = new DataContext())
        {
            SemaphoreTest test = new SemaphoreTest();
            test.Key = semaphore.Key;
            test.DateTimeCreated = semaphore.Created;
            test.DateTimeLastUsed = semaphore.LastUsed;
            dbContext.SemaphoreTest.InsertOnSubmit(test);
            dbContext.SubmitChanges();
        }
        return "All Good";

    }
    catch (Exception ex)
    {

        return ex.ToString();
    }
    finally
    {
        if (semaphore != null)
        {
            semaphore.Release();
        }

    };
}

I have run the above code using a WebSurge with a bunch of threads and it seems to pick up the correct existing SemaphoreSlim class for the same cart id. Obviously there needs to be some clean up run to remove old SemaphoreSlim instances from the in memory Dictionary which I have not added as yet. Not sure about the best way to go about this - any ideas ? Also, thoughts on this generally as a pattern ? Are there some glaring mistakes in there and a better method to employ ?

Edit There will potentially be a number of calls to this function with different cart ids. The same id can be used multiple times (but usually at least twice).The line below is just simulating passing an id to the function. It is using a fixed list of 300 keys and grabbing one each time. When I hit this with a load tester using 20 threads, it picks up the correct SempahoreSlim each time for the same id.

SempahoreTest.Models.RandomNxGuid.GetRestrictedNxGuid(300);

In practice, the cart key will be passed in to the function.Currently there may be a hundred or so sessions at any point in time, but they won't all be trying to hit the code at the same time, unless everyone is paying for their cart simultaneously. The crucial part is that for each different cart id, there may be two different threads hitting the function and only one of them should run the task. I also omitted in the code above a check that is made to see if the other thread has done the job (and saved a record in the database), in which case the second thread coming through does not need to do anything. If we just have one SemaphoreSlim for all carts, my worry was that it would block the others too much as the api and database calls are quite heavy and may take up around 500ms to complete.

Answer 1

Review

I would say, KISS (keep it stupid simple). Try going for a single lock and benchmark both normal expected thoughput and peek scenarios. If you would manage with this setup, you're done.

In case you would require a mutex for every unique key, using a lock to get that key is good practice. You have optimized lookup (tryget vs contains and get) if (_cartSemaphores.TryGetValue(nxID, out semaphore)) and all actions inside this lock are fast.

However, as mentioned in the comments, your cache might grow over time, and you'll eventually reach a point most keys would become lingering. You would need to think about how to perform some garbage collection on this cache. This can be tricky. Check out this post force-concurrentdictionary-in-a-singleton-registry-to-collect-removed-items-spac to figure out C# collections are optimized for performance, not for memory management when cleaning up lingering items. So at some point, you would probably want to clear the cache completely, and re-assign a new instance of the cache. This could be very expensive, but required when your server runs 24/7 and has lots of new and stale semaphores.

One possible way of cleaning the cache is to have a scheduled task that periodically (you should figure out a good cycle window) clears the cache.

_{in pseudo code (comments)}

 lock (_lockObj)
 {
     // acquire the semaphore for each key in the dictionary
     // clear the dictionary
     // assign a new empty dictionary
 }

One other thing, you are assigning a slightly different update time than creation time:

semaphore = new NxSemaphore()
{
    Created = DateTime.Now,
    Key = nxID,
    LastUsed = DateTime.Now
};

This probably doesn't have a major impact, but it would better to have the initial state consistent.

public NxSemaphore()
{
    Created = DateTime.Now;
    LastUsed  = Created;
}