From da6fe8053147c7901712095146c6864359319c11 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Tue, 6 Feb 2018 19:35:06 -0500 Subject: [PATCH] Address #11 by throwing a specific exception type (DeadlockException) when deadlocks are detected. --- ...onnectionOrTransactionStrategyTestCases.cs | 64 +++++++++++++++++++ ...stributedSemaphoreSelfDeadlockTestCases.cs | 2 +- .../DistributedLock.Tests.csproj | 2 + .../Tests/CombinatorialTests.cs | 18 ++++++ .../Tests/DeadlockExceptionTest.cs | 35 ++++++++++ DistributedLock.Tests/Tests/TestSetupTest.cs | 3 +- DistributedLock/DeadlockException.cs | 37 +++++++++++ DistributedLock/DistributedLock.csproj | 5 +- DistributedLock/Sql/SqlApplicationLock.cs | 2 +- DistributedLock/Sql/SqlSemaphore.cs | 2 +- README.md | 1 + 11 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 DistributedLock.Tests/AbstractTestCases/ExternalConnectionOrTransactionStrategyTestCases.cs create mode 100644 DistributedLock.Tests/Tests/DeadlockExceptionTest.cs create mode 100644 DistributedLock/DeadlockException.cs diff --git a/DistributedLock.Tests/AbstractTestCases/ExternalConnectionOrTransactionStrategyTestCases.cs b/DistributedLock.Tests/AbstractTestCases/ExternalConnectionOrTransactionStrategyTestCases.cs new file mode 100644 index 00000000..5776687b --- /dev/null +++ b/DistributedLock.Tests/AbstractTestCases/ExternalConnectionOrTransactionStrategyTestCases.cs @@ -0,0 +1,64 @@ +using Medallion.Threading.Sql; +using Medallion.Threading.Tests.Sql; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Collections.Generic; +using System.Data.SqlClient; +using System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +namespace Medallion.Threading.Tests +{ + [TestClass] + public abstract class ExternalConnectionOrTransactionStrategyTestCases : TestBase + where TEngineFactory : ITestingSqlDistributedLockEngineFactory, new() + where TConnectionManagementProvider : TestingSqlConnectionManagementProvider, IExternalConnectionOrTransactionTestingSqlConnectionManagementProvider, new() + { + [TestMethod] + public void TestDeadlockDetection() + { + var timeout = TimeSpan.FromSeconds(30); + + using (var engine = this.CreateEngine()) + using (var provider = new TConnectionManagementProvider()) + using (var barrier = new Barrier(participantCount: 2)) + { + const string LockName1 = nameof(TestDeadlockDetection) + "_1", + LockName2 = nameof(TestDeadlockDetection) + "_2"; + + Task RunDeadlock(bool isFirst) + { + var connectionInfo = provider.GetConnectionInfo(); + IDistributedLock lock1, lock2; + using (connectionInfo.Transaction != null ? TransactionProvider.UseTransaction(connectionInfo.Transaction) : ConnectionProvider.UseConnection(connectionInfo.Connection)) + { + lock1 = engine.CreateLock(isFirst ? LockName1 : LockName2); + lock2 = engine.CreateLock(isFirst ? LockName2 : LockName1); + } + return Task.Run(async () => + { + using (await lock1.AcquireAsync(timeout)) + { + barrier.SignalAndWait(); + (await lock2.AcquireAsync(timeout)).Dispose(); + } + }); + } + + var tasks = new[] { RunDeadlock(isFirst: true), RunDeadlock(isFirst: false) }; + + Task.WhenAll(tasks).ContinueWith(_ => { }).Wait(TimeSpan.FromSeconds(10)).ShouldEqual(true, this.GetType().Name); + + var deadlockVictim = tasks.Single(t => t.IsFaulted); + Assert.IsInstanceOfType(deadlockVictim.Exception.GetBaseException(), typeof(InvalidOperationException)); // backwards compat check + Assert.IsInstanceOfType(deadlockVictim.Exception.GetBaseException(), typeof(DeadlockException)); + + tasks.Count(t => t.Status == TaskStatus.RanToCompletion).ShouldEqual(1); + } + } + + private TestingDistributedLockEngine CreateEngine() => new TEngineFactory().Create(); + } +} diff --git a/DistributedLock.Tests/AbstractTestCases/SqlDistributedSemaphoreSelfDeadlockTestCases.cs b/DistributedLock.Tests/AbstractTestCases/SqlDistributedSemaphoreSelfDeadlockTestCases.cs index 889103f2..2cca01a7 100644 --- a/DistributedLock.Tests/AbstractTestCases/SqlDistributedSemaphoreSelfDeadlockTestCases.cs +++ b/DistributedLock.Tests/AbstractTestCases/SqlDistributedSemaphoreSelfDeadlockTestCases.cs @@ -27,7 +27,7 @@ public void TestSelfDeadlockThrowsOnInfiniteWait() var semaphore = engine.CreateSemaphore(nameof(TestSelfDeadlockThrowsOnInfiniteWait), maxCount: 2); semaphore.Acquire(); semaphore.Acquire(); - var ex = TestHelper.AssertThrows(() => semaphore.Acquire()); + var ex = TestHelper.AssertThrows(() => semaphore.Acquire()); ex.Message.Contains("Deadlock").ShouldEqual(true, ex.Message); } } diff --git a/DistributedLock.Tests/DistributedLock.Tests.csproj b/DistributedLock.Tests/DistributedLock.Tests.csproj index 53b64ddc..349684c5 100644 --- a/DistributedLock.Tests/DistributedLock.Tests.csproj +++ b/DistributedLock.Tests/DistributedLock.Tests.csproj @@ -63,6 +63,7 @@ + @@ -85,6 +86,7 @@ + diff --git a/DistributedLock.Tests/Tests/CombinatorialTests.cs b/DistributedLock.Tests/Tests/CombinatorialTests.cs index 33144c56..688d3c1e 100644 --- a/DistributedLock.Tests/Tests/CombinatorialTests.cs +++ b/DistributedLock.Tests/Tests/CombinatorialTests.cs @@ -240,4 +240,22 @@ public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_TransactionBased [TestClass] public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_MultiplexedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ExternalConnectionOrTransactionStrategy_SqlEngineFactory_ConnectionProviderTest : ExternalConnectionOrTransactionStrategyTestCases { } + + [TestClass] + public class ExternalConnectionOrTransactionStrategy_SqlEngineFactory_TransactionProviderTest : ExternalConnectionOrTransactionStrategyTestCases { } + + [TestClass] + public class ExternalConnectionOrTransactionStrategy_SqlReaderWriterEngineFactory_ConnectionProviderTest : ExternalConnectionOrTransactionStrategyTestCases { } + + [TestClass] + public class ExternalConnectionOrTransactionStrategy_SqlReaderWriterEngineFactory_TransactionProviderTest : ExternalConnectionOrTransactionStrategyTestCases { } + + [TestClass] + public class ExternalConnectionOrTransactionStrategy_SqlSemaphoreEngineFactory_ConnectionProviderTest : ExternalConnectionOrTransactionStrategyTestCases { } + + [TestClass] + public class ExternalConnectionOrTransactionStrategy_SqlSemaphoreEngineFactory_TransactionProviderTest : ExternalConnectionOrTransactionStrategyTestCases { } } diff --git a/DistributedLock.Tests/Tests/DeadlockExceptionTest.cs b/DistributedLock.Tests/Tests/DeadlockExceptionTest.cs new file mode 100644 index 00000000..80e91f19 --- /dev/null +++ b/DistributedLock.Tests/Tests/DeadlockExceptionTest.cs @@ -0,0 +1,35 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.Serialization.Formatters.Binary; +using System.Text; +using System.Threading.Tasks; + +namespace Medallion.Threading.Tests.Tests +{ + [TestClass] + public class DeadlockExceptionTest : TestBase + { + [TestMethod] + public void TestDeadlockExceptionSerialization() + { + void ThrowDeadlockException() => throw new DeadlockException(nameof(TestDeadlockExceptionSerialization), new InvalidOperationException("foo")); + + DeadlockException deadlockException = null; + try { ThrowDeadlockException(); } + catch (DeadlockException ex) { deadlockException = ex; } + + var formatter = new BinaryFormatter(); + var stream = new MemoryStream(); + formatter.Serialize(stream, deadlockException); + + stream.Position = 0; + var deserialized = (DeadlockException)formatter.Deserialize(stream); + deserialized.Message.ShouldEqual(deadlockException.Message); + deserialized.StackTrace.ShouldEqual(deadlockException.StackTrace); + deserialized.InnerException.Message.ShouldEqual(deadlockException.InnerException.Message); + } + } +} diff --git a/DistributedLock.Tests/Tests/TestSetupTest.cs b/DistributedLock.Tests/Tests/TestSetupTest.cs index aa41f42b..396b4077 100644 --- a/DistributedLock.Tests/Tests/TestSetupTest.cs +++ b/DistributedLock.Tests/Tests/TestSetupTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Reflection; using System.Text; using System.Text.RegularExpressions; using System.Threading.Tasks; @@ -90,7 +91,7 @@ private Type[] GetTypesForGenericParameter(Type genericParameter) var constraints = genericParameter.GetGenericParameterConstraints(); return this.GetType().Assembly .GetTypes() - // this doesn't support fancy constraints like class + // this doesn't support all fancy constraints like class or new() // see https://stackoverflow.com/questions/4864496/checking-if-an-object-meets-a-generic-parameter-constraint .Where(t => !t.IsAbstract && constraints.All(c => c.IsAssignableFrom(t))) .SelectMany(t => t.IsGenericTypeDefinition ? this.GetTypesForGenericParameters(t.GetGenericArguments()).Select(p => t.MakeGenericType(p)) : new[] { t }) diff --git a/DistributedLock/DeadlockException.cs b/DistributedLock/DeadlockException.cs new file mode 100644 index 00000000..cf2a1394 --- /dev/null +++ b/DistributedLock/DeadlockException.cs @@ -0,0 +1,37 @@ +using System; +using System.Collections.Generic; +using System.Text; + +namespace Medallion.Threading +{ + /// + /// An exception that SOME distributed locks will throw under SOME deadlock conditions. Note that even locks + /// that throw this exception under some circumstances cannot detect ALL deadlock conditions + /// +#if !NETSTANDARD_1_3 + [Serializable] +#endif + public sealed class DeadlockException + // for backwards compat + : InvalidOperationException + { + /// + /// Constructs a new instance of with a default message + /// + public DeadlockException() : this("A deadlock occurred") { } + + /// + /// Constructs an instance of with the given + /// + public DeadlockException(string message) : base(message) { } + + /// + /// Constructs an instance of with the given and + /// + public DeadlockException(string message, Exception innerException) : base(message, innerException) { } + +#if !NETSTANDARD_1_3 + private DeadlockException(System.Runtime.Serialization.SerializationInfo info, System.Runtime.Serialization.StreamingContext context) : base(info, context) { } +#endif + } +} diff --git a/DistributedLock/DistributedLock.csproj b/DistributedLock/DistributedLock.csproj index 463498c7..7a55474a 100644 --- a/DistributedLock/DistributedLock.csproj +++ b/DistributedLock/DistributedLock.csproj @@ -39,8 +39,11 @@ 1591 + + NETSTANDARD_1_3 + + - diff --git a/DistributedLock/Sql/SqlApplicationLock.cs b/DistributedLock/Sql/SqlApplicationLock.cs index e0cac040..49279107 100644 --- a/DistributedLock/Sql/SqlApplicationLock.cs +++ b/DistributedLock/Sql/SqlApplicationLock.cs @@ -133,7 +133,7 @@ public static bool ParseExitCode(int exitCode) case -2: // canceled throw new OperationCanceledException(GetErrorMessage(exitCode, "canceled")); case -3: // deadlock - throw new InvalidOperationException(GetErrorMessage(exitCode, "deadlock")); + throw new DeadlockException(GetErrorMessage(exitCode, "deadlock")); case -999: // parameter / unknown throw new ArgumentException(GetErrorMessage(exitCode, "parameter validation or other error")); diff --git a/DistributedLock/Sql/SqlSemaphore.cs b/DistributedLock/Sql/SqlSemaphore.cs index 3aa15e9f..c7f149bc 100644 --- a/DistributedLock/Sql/SqlSemaphore.cs +++ b/DistributedLock/Sql/SqlSemaphore.cs @@ -162,7 +162,7 @@ private static Task ProcessAcquireResultAsync( // forever but also reasonable that someone should be able to test for lock acquisition without getting a throw if (timeoutMillis == Timeout.Infinite) { - throw new InvalidOperationException("Deadlock detected: attempt to acquire the semaphore cannot succeed because all tickets are held by the current connection"); + throw new DeadlockException("Deadlock detected: attempt to acquire the semaphore cannot succeed because all tickets are held by the current connection"); } ticketLockName = markerTableName = null; diff --git a/README.md b/README.md index 08af6465..10417c9c 100644 --- a/README.md +++ b/README.md @@ -200,6 +200,7 @@ Most of the time, you'll want to use the default connection strategy. See more d - 1.4.0 - Added a SQL-based distributed semaphore ([#7](https://github.com/madelson/DistributedLock/issues/7)) - Fix bug where SqlDistributedLockConnectionStrategy.Azure would leak connections, relying on GC to reclaim them ([#14](https://github.com/madelson/DistributedLock/issues/14)). Thanks [zavalita1](https://github.com/zavalita1) for investigating this issue! + - Throw a specific exception type (`DeadlockException`) rather than the generic `InvalidOperationException` when a deadlock is detected ([#11](https://github.com/madelson/DistributedLock/issues/11)) - 1.3.1 Minor fix to avoid "leaking" isolation level changes in transaction-based locks ([#8](https://github.com/madelson/DistributedLock/issues/8)). Also switched to the VS2017 project file format - 1.3.0 Added an Azure connection strategy to keep lock connections from becoming idle and being reclaimed by Azure's connection governor ([#5](https://github.com/madelson/DistributedLock/issues/5)) - 1.2.0