3
\$\begingroup\$

I'm writing a method to throttle duplicate requests taking place within multiple HttpModule instances within a web application.

Currently, I have the following setup:

// Container for semaphores
private static readonly ConcurrentDictionary<string, SemaphoreSlim> 
SemaphoreSlims = new ConcurrentDictionary<string, SemaphoreSlim>();

// Wrapper for getting semaphore
private static SemaphoreSlim GetSemaphoreSlim(string id)
{
    return SemaphoreSlims.GetOrAdd(id, new SemaphoreSlim(1, 1));
}


private async Task ProcessImageAsync(HttpContext context)
{
    // `hash` is the request path hashed.
    SemaphoreSlim semaphore = GetSemaphoreSlim(hash);
    await semaphore.WaitAsync();

    try
    {

    // Do awaitable task

    }
    finally
    {
        semaphore.Release();
    }
}

This will work but I have no way of disposing of the semaphore instances since I need to preserve the ConcurrentDictionary and cannot dispose of a semaphore within the Dispose() method for that HttpModule instance as it could be currently waiting in another instance.

By my calculation it would take up ~156 megabytes of memory for 1 million items stored in the dictionary. It's not a massive amount but I'd really like it to be scalable and somehow find a way to clean up after myself.

Is this a sensible pattern and is there a way I can improve on it?

\$\endgroup\$
2
\$\begingroup\$

Semaphore has a bigger overhead than a traditional lock pattern, aside being too complex for what you need. You could use a class such as this:

public class DeDuper : IDisposable
{
    private static readonly ConcurrentDictionary<String, Object> currentRequests = new ConcurrentDictionary();
    private string _hash;

    public DeDuper(string hash)
    {
        _hash = hash;
        var lockable = currentRequests.GetOrAdd(hash, () => new Object());
        Monitor.Enter(lockable);
    }


    public void Dispose()
    {
        object lockable;
        if(currentRequests.TryGetValue(_hash, out lockable) && Monitor.IsEntered(lockable))
        {
            Monitor.Exit(lockable);
        }
    }

    ~DeDuper()
    {
        Dispose();
    }
}

Then your class can simply be written as:

private async Task ProcessImageAsync(HttpContext context)
{
    using(new DeDuper(hash))
    {
        // do awaitable task
    }
}

You will have a class that only allows a single hash to be run at any one time, with the added side effect of not having to release the Semaphore ;)

Not sure if this would work, i'd need to benchmark but, you could reduce the amount of memory further by reducing the commonality before you hash, eg:

http://mydomain.com/path1/part1   becomes   path1/part1
http://mydomain.com/path1/part2   becomes   path1/part2

Update

I thought that you might be able to use Interlocked.CompareExchange to do the job that you want, but it turns out AutoResetEvent works better. Here is a working sample:

Actual locking code

public class LockAcquisition<T> : IDisposable
{
    private static readonly ConcurrentDictionary<T, LockWrapper> currentRequests = new ConcurrentDictionary<T, LockWrapper>();
    private readonly LockWrapper _currentLock;

    public LockAcquisition(T key)
    {
        _currentLock = currentRequests.GetOrAdd(key, new LockWrapper());
        _currentLock.AcquireLock();
    }

    public void Dispose()
    {
        if (_currentLock != null)
        {
            _currentLock.Dispose();
        }
    }

    ~LockAcquisition()
    {
        Dispose();
    }

    private class LockWrapper : IDisposable
    {
        private readonly AutoResetEvent _locker = new AutoResetEvent(true);

        public void AcquireLock()
        {
            _locker.WaitOne();
        }

        public void Dispose()
        {
            _locker.Set();
        }

        ~LockWrapper()
        {
            Dispose();
        }

    }
}

Wrapped to use a string as the key

public class LockAcquisition : LockAcquisition<String>
{
    public LockAcquisition(string key)
        : base(key)
    {
    } 
}

Testing code

internal class Program
{
    private static void Main(string[] args)
    {

        Run();
        Console.ReadKey();
    }

    private static async void Run()
    {
        const string hash = "123456";
        const string hash2 = "1234567";

        using (new LockAcquisition(hash))
        {
            using (new LockAcquisition(hash2))
            {
                using (new LockAcquisition(hash)) // This will deadlocked
                {   
                }
                await Task.Delay(TimeSpan.FromSeconds(2));
                Console.WriteLine("Done 2");
            }
            await Task.Delay(TimeSpan.FromSeconds(2));
            Console.WriteLine("Done 1");
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for the example, definitely a better direction. I'm fairly certain though that you can't use await within a lock though. See this. stackoverflow.com/questions/21404144/… \$\endgroup\$ – James South Aug 4 '14 at 8:59
  • \$\begingroup\$ Ah, I didn't realise that you couldn't do that. I will test an alternative approach on lunch (approx 1 hour from now). My main point sticks though, wrap the locking code into a separate class that hides away the details of the locking mechanism. \$\endgroup\$ – Stuart Blackler Aug 4 '14 at 9:46
  • \$\begingroup\$ Much appreciated. Yeah, definitely a good point. I'm gonna have a look also, maybe something based on the AsyncLock here. stackoverflow.com/questions/21011179/… \$\endgroup\$ – James South Aug 4 '14 at 10:27
  • \$\begingroup\$ @JamesSouth hopefully my update should be better for you. I think it should work with async/await. Will test more later. \$\endgroup\$ – Stuart Blackler Aug 4 '14 at 13:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.