6
\$\begingroup\$

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));
    }
}
\$\endgroup\$

closed as off-topic by rolfl Jul 1 '15 at 2:55

If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Hm, my feeling is that Dispose is expected to be synchronous hence you should be waiting for it: _db.LockReleaseAsync(Key, Value).Wait(). \$\endgroup\$ – ChrisWue Mar 1 '15 at 21:19
  • \$\begingroup\$ @ChrisWue, to be honest I completely agree. If you want to write that with a bit more conviction as an answer I'll happily accept it \$\endgroup\$ – ChrisT Mar 2 '15 at 19:07
  • \$\begingroup\$ I'm voting to close this question as off-topic because it is using a specific example to get input on a concept that is more general. This is not asking for a code review, but using the code as an example for feedback on a concept. \$\endgroup\$ – rolfl Jul 1 '15 at 2:55
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm going to check the stackexchange.redis documentation but I recall that it was fine to have this as static or even singleton. Perhaps I'm thinking of the parent ConnectionMultiplexer class though. Many thanks either way \$\endgroup\$ – ChrisT Mar 2 '15 at 19:41
  • \$\begingroup\$ Either way your route obviously is more extensible \$\endgroup\$ – ChrisT Mar 2 '15 at 19:43

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