Async distributed locking with StackExchange.Redis [closed]

Question

I have been trying to write an analogy to this approach to distributed locking with two differences:

• making it asynchronous
• making it work with StackExchange.Redis rather than AppFabric Cache.

My biggest concern is going out of my way to use IDisposable to make it compatible with using() but thus calling an async method from within Dispose (in a sort of 'fire and forget' way). The basic tests I have run so far seem to pass ok but it doesn't seem right.

public static class RedisExtensions
{
    public static Task<IDisposable> AcquireLockAsync(this IDatabaseAsync db, string key, TimeSpan? expiry = null, TimeSpan? retryTimeout = null)
    {
        if (db == null)
        {
            throw new ArgumentNullException("db");
        }
        if (key == null)
        {
            throw new ArgumentNullException("key");
        }
        return DataCacheLock.AcquireAsync(db, key, expiry, retryTimeout);

    }

    private class DataCacheLock : IDisposable
    {
        private static StackExchange.Redis.IDatabaseAsync _db;
        public readonly RedisKey Key;
        public readonly RedisValue Value;
        public readonly TimeSpan? Expiry;
        private DataCacheLock(IDatabaseAsync db, string key, TimeSpan? expiry)
        {
            _db = db;
            Key = "ninlock:" + key;
            Value = Guid.NewGuid().ToString();
            Expiry = expiry;
        }
        public static async Task<IDisposable> AcquireAsync(IDatabaseAsync db, string key, TimeSpan? expiry, TimeSpan? retryTimeout)
        {
            DataCacheLock dataCacheLock = new DataCacheLock(db, key, expiry);
            Debug.WriteLine(dataCacheLock.Key.ToString() + ":" + dataCacheLock.Value.ToString());
            Func<Task<bool>> task = async () =>
            {
                try
                {
                    return await _db.LockTakeAsync(dataCacheLock.Key, dataCacheLock.Value, dataCacheLock.Expiry ?? TimeSpan.MaxValue);
                }
                catch
                {
                    return false;
                }
            };

            await RetryUntilTrueAsync(task, retryTimeout);
            return dataCacheLock;
        }
        public void Dispose()
        {
            Debug.WriteLine("release the lock:" + Value);
            _db.LockReleaseAsync(Key, Value);
        }
    }
    private static readonly Random _random = new Random();
    private static async Task<bool> RetryUntilTrueAsync(Func<Task<bool>> task, TimeSpan? retryTimeout)
    {
        int i = 0;
        DateTime utcNow = DateTime.UtcNow;
        while (!retryTimeout.HasValue || DateTime.UtcNow - utcNow < retryTimeout.Value)
        {
            i++;
            if (await task())
            {
                return true;
            }
            var waitFor = _random.Next((int)Math.Pow(i, 2), (int)Math.Pow(i + 1, 2) + 1);
            Debug.WriteLine(waitFor);
            await Task.Delay(waitFor);
        }
        throw new TimeoutException(string.Format("Exceeded timeout of {0}", retryTimeout.Value));
    }
}

Answer 1

As mentioned in my comment I think in general Dispose is expected to be synchronous hence it should block until finished which is easy enough to achieve:

_db.LockReleaseAsync(Key, Value).Wait();

One other gripe I have with the code is the static IDatabaseAsync member of DataCacheLock. Maybe I'm missing something but I think this can get you into trouble. Assume you have two databases you want to talk to and you fire off to calls to AcquireLockAsync - now the member is set to whatever instance where the calls happened to be executed last which will probably cause funny results as LockTakeAsync might now wait on the wrong database instance for one of the calls and/or in Dispose the lock will be released against the wrong instance.

While this might be a rare use case I don't see the benefit of making this a static member in the first place - it's only accessed locally in either the task trying to acquire the lock or in the Dispose method. For the Dispose case a non-static member would work fine. For the locking task the instance can be captured locally in the closure as it's passed in. So it should be easy enough to remove the problematic hidden gotcha.