From 711f558a07d7802dba43ffeb5c1462cd48b2f538 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Sat, 3 Feb 2018 15:48:21 -0500 Subject: [PATCH 1/4] Add test for confirming that internally-owned connections are cleaned up. Fix #14 (issue with Azure connection strategy which failed the test). --- .../ConnectionStringStrategyTestCases.cs | 80 +++++++++++++++++++ .../DistributedLock.Tests.csproj | 7 ++ ...TestingSqlConnectionManagementProviders.cs | 10 ++- .../Tests/CombinatorialTests.cs | 54 +++++++++++++ DistributedLock.Tests/Tests/TestSetupTest.cs | 24 ++++-- DistributedLock.Tests/packages.config | 2 + .../InternalLocks/AzureSqlDistributedLock.cs | 26 +++--- 7 files changed, 187 insertions(+), 16 deletions(-) create mode 100644 DistributedLock.Tests/AbstractTestCases/ConnectionStringStrategyTestCases.cs diff --git a/DistributedLock.Tests/AbstractTestCases/ConnectionStringStrategyTestCases.cs b/DistributedLock.Tests/AbstractTestCases/ConnectionStringStrategyTestCases.cs new file mode 100644 index 00000000..1a31740e --- /dev/null +++ b/DistributedLock.Tests/AbstractTestCases/ConnectionStringStrategyTestCases.cs @@ -0,0 +1,80 @@ +using Medallion.Threading.Tests.Sql; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Collections.Generic; +using System.Data.SqlClient; +using System.Diagnostics; +using System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +namespace Medallion.Threading.Tests +{ + [TestClass] + public abstract class ConnectionStringStrategyTestCases : TestBase + where TEngineFactory : ITestingSqlDistributedLockEngineFactory, new() + where TConnectionManagementProvider : ConnectionStringProvider, new() + { + /// + /// Tests that internally-owned connections are properly cleaned up by disposing the lock handle + /// + [TestMethod] + public void TestConnectionDoesNotLeak() + { + var applicationName = nameof(TestConnectionDoesNotLeak) + Guid.NewGuid(); + var connectionString = new SqlConnectionStringBuilder(ConnectionStringProvider.ConnectionString) + { + ApplicationName = applicationName, + } + .ConnectionString; + + using (ConnectionStringProvider.UseConnectionString(connectionString)) + using (var engine = this.CreateEngine()) + { + var @lock = engine.CreateLock(nameof(TestConnectionDoesNotLeak)); + + for (var i = 0; i < 30; ++i) + { + using (@lock.Acquire()) + { + CountActiveSessions().ShouldEqual(1, this.GetType().Name); + } + // still alive due to pooling + CountActiveSessions().ShouldEqual(1, this.GetType().Name); + } + } + + using (var connection = new SqlConnection(connectionString)) + { + SqlConnection.ClearPool(connection); + // checking immediately seems flaky; likely clear pool finishing + // doesn't guarantee that SQL will immediately reflect the clear + var maxWaitForPoolsToClear = TimeSpan.FromSeconds(5); + var stopwatch = Stopwatch.StartNew(); + do + { + var activeCount = CountActiveSessions(); + if (activeCount == 0) { return; } + Thread.Sleep(25); + } + while (stopwatch.Elapsed < maxWaitForPoolsToClear); + } + + int CountActiveSessions() + { + using (var connection = new SqlConnection(ConnectionStringProvider.ConnectionString)) + { + connection.Open(); + using (var command = connection.CreateCommand()) + { + command.CommandText = $@"SELECT COUNT(*) FROM sys.dm_exec_sessions WHERE program_name = '{applicationName}'"; + return (int)command.ExecuteScalar(); + } + } + } + } + + private TestingDistributedLockEngine CreateEngine() => new TEngineFactory().Create(); + } +} diff --git a/DistributedLock.Tests/DistributedLock.Tests.csproj b/DistributedLock.Tests/DistributedLock.Tests.csproj index f703a882..53b64ddc 100644 --- a/DistributedLock.Tests/DistributedLock.Tests.csproj +++ b/DistributedLock.Tests/DistributedLock.Tests.csproj @@ -36,12 +36,18 @@ 4 + + ..\packages\MedallionCollections.1.1.0\lib\net45\MedallionCollections.dll + ..\packages\MedallionShell.1.2.1\lib\net45\MedallionShell.dll True + + ..\packages\System.ValueTuple.4.4.0\lib\netstandard1.0\System.ValueTuple.dll + @@ -56,6 +62,7 @@ + diff --git a/DistributedLock.Tests/Infrastructure/TestingSqlConnectionManagementProviders.cs b/DistributedLock.Tests/Infrastructure/TestingSqlConnectionManagementProviders.cs index e520c595..78e1e978 100644 --- a/DistributedLock.Tests/Infrastructure/TestingSqlConnectionManagementProviders.cs +++ b/DistributedLock.Tests/Infrastructure/TestingSqlConnectionManagementProviders.cs @@ -23,6 +23,8 @@ public abstract class ConnectionStringProvider : TestingSqlConnectionManagementP } .ConnectionString; + private static volatile string currentConnectionString = ConnectionString; + private readonly SqlDistributedLockConnectionStrategy? _strategy; public ConnectionStringProvider(SqlDistributedLockConnectionStrategy? strategy) @@ -30,13 +32,19 @@ public ConnectionStringProvider(SqlDistributedLockConnectionStrategy? strategy) this._strategy = strategy; } - public override ConnectionInfo GetConnectionInfo() => new ConnectionInfo { ConnectionString = ConnectionString, Strategy = this._strategy }; + public override ConnectionInfo GetConnectionInfo() => new ConnectionInfo { ConnectionString = currentConnectionString, Strategy = this._strategy }; /// /// Since every lock handle has a dedicated connection (even in the case of multiplexing we don't share a connection for two acquires /// on the same name, we are not reentrant) /// internal override bool IsReentrantForAppLock => false; + + public static IDisposable UseConnectionString(string connectionString) + { + currentConnectionString = connectionString; + return new ReleaseAction(() => currentConnectionString = ConnectionString); + } } public sealed class NoStrategyConnectionStringProvider : ConnectionStringProvider diff --git a/DistributedLock.Tests/Tests/CombinatorialTests.cs b/DistributedLock.Tests/Tests/CombinatorialTests.cs index dfbb231f..33144c56 100644 --- a/DistributedLock.Tests/Tests/CombinatorialTests.cs +++ b/DistributedLock.Tests/Tests/CombinatorialTests.cs @@ -186,4 +186,58 @@ public class SqlSemaphoreSelfDeadlock_ConnectionProviderTest : SqlDistributedSem [TestClass] public class SqlSemaphoreSelfDeadlock_TransactionProviderTest : SqlDistributedSemaphoreSelfDeadlockTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlEngineFactory_NoStrategyConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlEngineFactory_DefaultConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlEngineFactory_AzureConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlEngineFactory_ConnectionBasedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlEngineFactory_TransactionBasedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlEngineFactory_MultiplexedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlReaderWriterEngineFactory_NoStrategyConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlReaderWriterEngineFactory_DefaultConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlReaderWriterEngineFactory_AzureConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlReaderWriterEngineFactory_ConnectionBasedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlReaderWriterEngineFactory_TransactionBasedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlReaderWriterEngineFactory_MultiplexedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_NoStrategyConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_DefaultConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_AzureConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_ConnectionBasedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_TransactionBasedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } + + [TestClass] + public class ConnectionStringStrategy_SqlSemaphoreEngineFactory_MultiplexedConnectionStringProviderTest : ConnectionStringStrategyTestCases { } } diff --git a/DistributedLock.Tests/Tests/TestSetupTest.cs b/DistributedLock.Tests/Tests/TestSetupTest.cs index 34ca1fc3..aa41f42b 100644 --- a/DistributedLock.Tests/Tests/TestSetupTest.cs +++ b/DistributedLock.Tests/Tests/TestSetupTest.cs @@ -1,4 +1,5 @@ -using Microsoft.VisualStudio.TestTools.UnitTesting; +using Medallion.Collections; +using Microsoft.VisualStudio.TestTools.UnitTesting; using System; using System.Collections.Generic; using System.Linq; @@ -69,18 +70,31 @@ public class {testClassName} : {GetCSharpName(testClassType)} {{ }}"; private static string RemoveGenericMarkers(string name) => Regex.Replace(name, @"`\d+", string.Empty); - private List GetTypesForGenericParameters(Type[] genericParameters) + private List GetTypesForGenericParameters(Type[] genericParameters) { - if (genericParameters.Length > 1) { throw new NotSupportedException(); } + var genericParameterTypes = genericParameters.Select(this.GetTypesForGenericParameter).ToArray(); + var allCombinations = Traverse.DepthFirst( + root: (index: 0, value: Enumerable.Empty()), + children: t => t.index == genericParameterTypes.Length + ? Enumerable.Empty<(int index, IEnumerable value)>() + : genericParameterTypes[t.index].Select(type => (index: t.index + 1, value: t.value.Append(type))) + ) + .Where(t => t.index == genericParameterTypes.Length) + .Select(t => t.value.ToArray()) + .ToList(); + return allCombinations; + } - var constraints = genericParameters[0].GetGenericParameterConstraints(); + private Type[] GetTypesForGenericParameter(Type genericParameter) + { + var constraints = genericParameter.GetGenericParameterConstraints(); return this.GetType().Assembly .GetTypes() // this doesn't support fancy constraints like class // 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 }) - .ToList(); + .ToArray(); } } } diff --git a/DistributedLock.Tests/packages.config b/DistributedLock.Tests/packages.config index ef98934e..0ca9a949 100644 --- a/DistributedLock.Tests/packages.config +++ b/DistributedLock.Tests/packages.config @@ -1,4 +1,6 @@  + + \ No newline at end of file diff --git a/DistributedLock/Sql/InternalLocks/AzureSqlDistributedLock.cs b/DistributedLock/Sql/InternalLocks/AzureSqlDistributedLock.cs index 504e82bf..5f7b6c76 100644 --- a/DistributedLock/Sql/InternalLocks/AzureSqlDistributedLock.cs +++ b/DistributedLock/Sql/InternalLocks/AzureSqlDistributedLock.cs @@ -31,7 +31,7 @@ public IDisposable TryAcquire(int timeoutMillis, ISqlSynchronizatio { var internalHandle = lockScope.InternalLock.TryAcquire(timeoutMillis, strategy, contextHandle: lockScope.InternalHandle); return internalHandle != null - ? new LockScope(internalHandle, lockScope.InternalLock, lockScope.Keepalive, ownsKeepalive: false) + ? new LockScope(internalHandle, lockScope.InternalLock, lockScope.Keepalive, connection: null) : null; } finally @@ -52,7 +52,7 @@ public IDisposable TryAcquire(int timeoutMillis, ISqlSynchronizatio { var keepalive = new KeepaliveHelper(connection); keepalive.Start(); - result = new LockScope(internalHandle, internalLock, keepalive, ownsKeepalive: true); + result = new LockScope(internalHandle, internalLock, keepalive, connection); } } finally @@ -78,7 +78,7 @@ public async Task TryAcquireAsync(int timeoutMillis, I { var internalHandle = await lockScope.InternalLock.TryAcquireAsync(timeoutMillis, strategy, cancellationToken, contextHandle: lockScope.InternalHandle).ConfigureAwait(false); return internalHandle != null - ? new LockScope(internalHandle, lockScope.InternalLock, lockScope.Keepalive, ownsKeepalive: false) + ? new LockScope(internalHandle, lockScope.InternalLock, lockScope.Keepalive, connection: null) : null; } finally @@ -99,7 +99,7 @@ public async Task TryAcquireAsync(int timeoutMillis, I { var keepalive = new KeepaliveHelper(connection); keepalive.Start(); - result = new LockScope(internalHandle, internalLock, keepalive, ownsKeepalive: true); + result = new LockScope(internalHandle, internalLock, keepalive, connection); } } finally @@ -109,22 +109,22 @@ public async Task TryAcquireAsync(int timeoutMillis, I return result; } - + private sealed class LockScope : IDisposable { - private readonly bool ownsKeepalive; private KeepaliveHelper keepalive; + private SqlConnection connection; public LockScope( IDisposable internalHandle, ExternalConnectionOrTransactionSqlDistributedLock internalLock, KeepaliveHelper keepalive, - bool ownsKeepalive) + SqlConnection connection) { this.InternalHandle = internalHandle; this.InternalLock = internalLock; this.keepalive = keepalive; - this.ownsKeepalive = ownsKeepalive; + this.connection = connection; } public IDisposable InternalHandle { get; private set; } @@ -144,9 +144,15 @@ public void Dispose() this.InternalHandle.Dispose(); this.InternalHandle = null; this.InternalLock = null; - if (!this.ownsKeepalive) + if (this.connection != null) + { + // if we own the connection then dispose it + this.connection.Dispose(); + this.connection = null; + } + else { - // if the keepalive is owned by an outer handle, restart it + // otherwise, the keepalive is owned by an outer handle so restart it keepalive.Start(); } } From df69c420514ccb57ac5781566595645383c9813a Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Sat, 3 Feb 2018 16:18:24 -0500 Subject: [PATCH 2/4] Release notes for connection leak fix --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6dd64968..02da40a5 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,8 @@ Most of the time, you'll want to use the default connection strategy. See more d ## Release notes - 1.4.0 - - Added a SQL-based distributed semaphore ([#7](https://github.com/madelson/DistributedLock/issues/7)) + - 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 for investigating this issue! - 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 From 82e5106f92ac13caedd363f261d99ef711207c03 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Sat, 3 Feb 2018 16:22:27 -0500 Subject: [PATCH 3/4] README fixup --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 02da40a5..7ea64fb0 100644 --- a/README.md +++ b/README.md @@ -199,7 +199,7 @@ Most of the time, you'll want to use the default connection strategy. See more d ## Release notes - 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 for investigating this issue! + - 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! - 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 From dbde988ba7c4ced1ac00061dd3af83b8ebefe22b Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Sat, 3 Feb 2018 16:24:09 -0500 Subject: [PATCH 4/4] README fixup --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7ea64fb0..08af6465 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,7 @@ Most of the time, you'll want to use the default connection strategy. See more d ## Release notes - 1.4.0 - - Added a SQL-based distributed semaphore ([#7](https://github.com/madelson/DistributedLock/issues/7)). + - 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! - 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))