diff --git a/src/DistributedLock.Redis/RedLock/RedLockAcquire.cs b/src/DistributedLock.Redis/RedLock/RedLockAcquire.cs index 9357570a..0a860152 100644 --- a/src/DistributedLock.Redis/RedLock/RedLockAcquire.cs +++ b/src/DistributedLock.Redis/RedLock/RedLockAcquire.cs @@ -133,8 +133,13 @@ private async Task WaitForAcquireAsync(IReadOnlyDictionary t.IsCanceled || t.IsFaulted) + var faultingTasks = tryAcquireTasks.Values + .Where(t => t.IsCanceled || t.IsFaulted) .ToArray(); + if (faultingTasks.Length == 0) + { + await completed.ConfigureAwait(false); // propagate a synthetic disconnected fault + } if (faultingTasks.Length == 1) { await faultingTasks[0].ConfigureAwait(false); // propagate the error @@ -167,7 +172,7 @@ private async Task WaitForAcquireAsync(IReadOnlyDictionary WaitForAcquireAsync(IReadOnlyDictionary 0 && RedLockHelper.HasSufficientSuccesses(successCount + 1, @this._databases.Count)) + // For multiple databases, first check to see if (a) we have at least 1 success/failure and (b) one more would be decisive. + // A disconnected single database is always decisive. + if (@this._databases.Count != 1 + && !((successCount > 0 && RedLockHelper.HasSufficientSuccesses(successCount + 1, @this._databases.Count)) || (failCount > 0 && RedLockHelper.HasTooManyFailuresOrFaults(failCount + 1, @this._databases.Count)))) { return null; diff --git a/src/DistributedLock.Tests/Tests/Redis/RedisDistributedLockTest.cs b/src/DistributedLock.Tests/Tests/Redis/RedisDistributedLockTest.cs index 6ea2d757..238ecf8a 100644 --- a/src/DistributedLock.Tests/Tests/Redis/RedisDistributedLockTest.cs +++ b/src/DistributedLock.Tests/Tests/Redis/RedisDistributedLockTest.cs @@ -29,6 +29,109 @@ public void TestValidatesConstructorParameters() Assert.Throws(() => new RedisDistributedLock("key", Enumerable.Empty())); } + [Test, Category("CI")] + public void TestDisconnectedSingleDatabaseCausesTryAcquireAsyncToThrow() + { + var pendingAcquire = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var database = new Mock(MockBehavior.Strict); + database + .Setup(d => d.StringSetAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(pendingAcquire.Task); + database + .Setup(d => d.IsConnected(It.IsAny(), It.IsAny())) + .Returns(false); + + var @lock = new RedisDistributedLock( + "key", + database.Object, + options => options + .Expiry(TimeSpan.FromMilliseconds(200)) + .MinValidityTime(TimeSpan.FromMilliseconds(100)) + ); + + Assert.ThrowsAsync(() => @lock.TryAcquireAsync().AsTask()); + } + + [Test, Category("CI")] + public void TestSyntheticDisconnectedFaultDoesNotMaskRealFault() + { + var expectedException = new TimeZoneNotFoundException(); + var faultedDatabase = new Mock(MockBehavior.Strict); + faultedDatabase + .Setup(d => d.StringSetAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(Task.FromException(expectedException)); + faultedDatabase + .Setup(d => d.ScriptEvaluateAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(RedisResult.Create(false)); + + var connectedPendingAcquire = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var connectedDatabase = new Mock(MockBehavior.Strict); + connectedDatabase + .Setup(d => d.StringSetAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(connectedPendingAcquire.Task); + connectedDatabase + .Setup(d => d.IsConnected(It.IsAny(), It.IsAny())) + .Returns(true); + + var disconnectedPendingAcquire = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var disconnectedDatabase = new Mock(MockBehavior.Strict); + disconnectedDatabase + .Setup(d => d.StringSetAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .Returns(disconnectedPendingAcquire.Task); + disconnectedDatabase + .Setup(d => d.IsConnected(It.IsAny(), It.IsAny())) + .Returns(false); + + var @lock = new RedisDistributedLock( + "key", + new[] { faultedDatabase.Object, connectedDatabase.Object, disconnectedDatabase.Object } + ); + + Assert.ThrowsAsync(() => @lock.TryAcquireAsync().AsTask()); + } + + [Test, Category("CI")] + public async Task TestSingleDatabaseContentionCausesTryAcquireAsyncToReturnNull() + { + var database = new Mock(MockBehavior.Strict); + database + .Setup(d => d.StringSetAsync( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync(false); + + var @lock = new RedisDistributedLock("key", database.Object); + + Assert.That(await @lock.TryAcquireAsync(), Is.Null); + } + /// /// Reproduces the bug in https://github.com/madelson/DistributedLock/issues/162 /// where a Redis lock couldn't be acquired if the current CultureInfo was tr-TR,