Merge pull request #220 from madelson/release-2.5

Release 2.5

README.md

@@ -141,6 +141,11 @@ Contributions are welcome! If you are interested in contributing towards a new o
 Setup steps for working with the repository locally are documented [here](docs/Developing%20DistributedLock.md).
 
 ## Release notes
+- 2.5
+	- Add support for creating Postgres locks off `DbDataSource` which is helpful for apps using `NpgsqlMultiHostDataSource`. Thanks @davidngjy for implementing! ([#153](https://github.com/madelson/DistributedLock/issues/153), DistributedLock.Postgres 1.2.0)
+	- Upgrade Npgsql to 8.0.3 to avoid vulnerability. Thanks @Meir017/@davidngjy for implementing! ([#218](https://github.com/madelson/DistributedLock/issues/218), DistributedLock.Postgres 1.2.0)
+	- Fix Postgres race condition with connection keepalive enabled ([#216](https://github.com/madelson/DistributedLock/issues/216), DistributedLock.Core 1.0.7)
+	- Upgrade Microsoft.Data.SqlClient to 5.2.1 to avoid vulnerability ([#210](https://github.com/madelson/DistributedLock/issues/210), DistributedLock.SqlServer 1.0.5)
 - 2.4
 	- Add support for transaction-scoped locking in Postgres using `pg_advisory_xact_lock` which is helpful when using PgBouncer ([#168](https://github.com/madelson/DistributedLock/issues/168), DistributedLock.Postgres 1.1.0)
 	- Improve support for newer versions of StackExchange.Redis, especially when using the default backlog policy ([#162](https://github.com/madelson/DistributedLock/issues/162), DistributedLock.Redis 1.0.3). Thanks [@Bartleby2718](https://github.com/Bartleby2718) for helping with this!

docs/DistributedLock.Postgres.md

@@ -28,7 +28,7 @@ Under the hood, [Postgres advisory locks can be based on either one 64-bit integ
 - Passing an ASCII string with 0-9 characters, which will be mapped to a `long` based on a custom scheme.
 - Passing an arbitrary string with the `allowHashing` option set to `true` which will be hashed to a `long`. Note that hashing will only be used if other methods of interpreting the string fail.
 
-In addition to specifying the `key`, Postgres-based locks allow you to specify either a `connectionString` or an `IDbConnection` as a means of connecting to the database. In most cases, using a `connectionString` is preferred because it allows for the library to efficiently multiplex connections under the hood and eliminates the risk that the passed-in `IDbConnection` gets used in a way that disrupts the locking process. **NOTE that since `IDbConnection` objects are not thread-safe, lock objects constructed with them can only be used by one thread at a time.**
+In addition to specifying the `key`, Postgres-based locks allow you to specify either a `connectionString`, an `IDbConnection`, or a `DbDataSource` as a means of connecting to the database. In most cases, using a `connectionString` is preferred because it allows for the library to efficiently multiplex connections under the hood and, in the case of `IDbConnection`, eliminates the risk that the passed-in `IDbConnection` gets used in a way that disrupts the locking process. **NOTE that since `IDbConnection` objects are not thread-safe, lock objects constructed with them can only be used by one thread at a time.**
 
 ## Options
 

src/Directory.Packages.props

@@ -12,9 +12,9 @@
     <PackageVersion Include="NUnit.Analyzers" Version="4.1.0" />
     <PackageVersion Include="Oracle.ManagedDataAccess.Core" Version="3.21.130" Condition="'$(TargetFramework)' == 'netstandard2.1'" />
     <PackageVersion Include="Oracle.ManagedDataAccess" Version="21.13.0" Condition="'$(TargetFramework)' == 'net462'" />
-    <PackageVersion Include="Npgsql" Version="8.0.2" />
+    <PackageVersion Include="Npgsql" Version="8.0.3" />
     <PackageVersion Include="StackExchange.Redis" Version="2.7.27" />
-    <PackageVersion Include="Microsoft.Data.SqlClient" Version="5.2.0" />
+    <PackageVersion Include="Microsoft.Data.SqlClient" Version="5.2.1" />
     <PackageVersion Include="nunit" Version="3.14.0" />
     <PackageVersion Include="nunit3testadapter" Version="4.5.0" />
     <PackageVersion Include="Microsoft.NET.Test.SDK" Version="17.9.0" />
@@ -27,4 +27,4 @@
     <PackageVersion Include="Microsoft.Bcl.AsyncInterfaces" Version="8.0.0" Condition="'$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462'" />
     <PackageVersion Include="System.ValueTuple" Version="4.5.0" Condition="'$(TargetFramework)' == 'net462'" />
   </ItemGroup>
-</Project>
+</Project>

src/DistributedLock.Core/DistributedLock.Core.csproj

@@ -11,7 +11,7 @@
   </PropertyGroup>
 
   <PropertyGroup>
-    <Version>1.0.6</Version>
+    <Version>1.0.7</Version>
     <AssemblyVersion>1.0.0.0</AssemblyVersion>
     <Authors>Michael Adelson</Authors>
     <Description>Core interfaces and utilities that support the DistributedLock.* family of packages</Description>

src/DistributedLock.Core/Internal/AsyncLock.cs

@@ -31,14 +31,9 @@ public async ValueTask<IDisposable> AcquireAsync(CancellationToken cancellationT
         return acquired ? new Handle(this._semaphore) : null;
     }
 
-    private sealed class Handle : IDisposable
+    private sealed class Handle(SemaphoreSlim semaphore) : IDisposable
     {
-        private SemaphoreSlim? _semaphore;
-
-        public Handle(SemaphoreSlim semaphore)
-        {
-            this._semaphore = semaphore;
-        }
+        private SemaphoreSlim? _semaphore = semaphore;
 
         public void Dispose() => Interlocked.Exchange(ref this._semaphore, null)?.Release();
     }

src/DistributedLock.Core/Internal/Data/ConnectionMonitor.cs

@@ -138,7 +138,7 @@ public IDatabaseConnectionMonitoringHandle GetMonitoringHandle()
 
             var connectionLostTokenSource = new CancellationTokenSource();
             var handle = new MonitoringHandle(this, connectionLostTokenSource.Token);
-            (this._monitoringHandleRegistrations ??= new Dictionary<MonitoringHandle, CancellationTokenSource>())
+            (this._monitoringHandleRegistrations ??= [])
                 .Add(handle, connectionLostTokenSource);
 
             if (!this.StartMonitorWorkerIfNeededNoLock() 
@@ -403,16 +403,10 @@ private async Task<bool> DoKeepaliveAsync(TimeoutValue keepaliveCadence, Cancell
         return true;
     }
 
-    private sealed class MonitoringHandle : IDatabaseConnectionMonitoringHandle
+    private sealed class MonitoringHandle(ConnectionMonitor keepaliveHelper, CancellationToken cancellationToken) : IDatabaseConnectionMonitoringHandle
     {
-        private ConnectionMonitor? _monitor;
-        private readonly CancellationToken _connectionLostToken;
-
-        public MonitoringHandle(ConnectionMonitor keepaliveHelper, CancellationToken cancellationToken)
-        {
-            this._monitor = keepaliveHelper;
-            this._connectionLostToken = cancellationToken;
-        }
+        private ConnectionMonitor? _monitor = keepaliveHelper;
+        private readonly CancellationToken _connectionLostToken = cancellationToken;
 
         public CancellationToken ConnectionLostToken => Volatile.Read(ref this._monitor) != null ? this._connectionLostToken : throw new ObjectDisposedException("handle");
 

src/DistributedLock.Core/Internal/Data/DatabaseConnection.cs

@@ -46,7 +46,13 @@ protected DatabaseConnection(IDbTransaction transaction, bool isExternallyOwned)
 
     public DatabaseCommand CreateCommand()
     {
-        var command = this.InnerConnection.CreateCommand();
+        IDbCommand command;
+        // Because of Npgsql's command recycling (https://github.com/npgsql/npgsql/blob/main/src/Npgsql/NpgsqlConnection.cs#L566),
+        // CreateCommand() is not actually thread-safe. Ideally this would use this.ConnectionMonitor.AcquireConnectionLockAsync
+        // like other operations, but that requires a change to the Core internal API so I'm leaving it for #217. For the current
+        // issue with Npgsql, merely synchronizing access to this method should be good enough, and ConnectionMonitor makes a
+        // fine lock object that isn't being used elsewhere (#216)
+        lock (this.ConnectionMonitor) { command = this.InnerConnection.CreateCommand(); }
         command.Transaction = this._transaction;
         return new DatabaseCommand(command, this);
     }

src/DistributedLock.Core/Internal/ManagedFinalizerQueue.cs

@@ -142,7 +142,7 @@ private Task FinalizeAsync(bool waitForItemFinalization)
                 var itemFinalizerTask = this.TryRemove(kvp.Key, disposeKey: true);
                 if (waitForItemFinalization)
                 {
-                    (itemFinalizerTasks ??= new List<Task>()).Add(itemFinalizerTask);
+                    (itemFinalizerTasks ??= []).Add(itemFinalizerTask);
                 }
             }
         }
@@ -171,16 +171,10 @@ private Task TryRemove(IAsyncDisposable key, bool disposeKey)
         return Task.CompletedTask;
     }
 
-    private sealed class Registration : IDisposable
+    private sealed class Registration(ManagedFinalizerQueue queue, IAsyncDisposable key) : IDisposable
     {
-        private readonly ManagedFinalizerQueue _queue;
-        private IAsyncDisposable? _key;
-
-        public Registration(ManagedFinalizerQueue queue, IAsyncDisposable key)
-        {
-            this._queue = queue;
-            this._key = key;
-        }
+        private readonly ManagedFinalizerQueue _queue = queue;
+        private IAsyncDisposable? _key = key;
 
         public void Dispose()
         {

src/DistributedLock.Core/packages.lock.json

@@ -172,9 +172,9 @@
       },
       "Microsoft.NET.ILLink.Tasks": {
         "type": "Direct",
-        "requested": "[8.0.2, )",
-        "resolved": "8.0.2",
-        "contentHash": "hKTrehpfVzOhAz0mreaTAZgbz0DrMEbWq4n3hAo8Ks6WdxdqQhNPvzOqn9VygKuWf1bmxPdraqzTaXriO/sn0A=="
+        "requested": "[8.0.4, )",
+        "resolved": "8.0.4",
+        "contentHash": "PZb5nfQ+U19nhnmnR9T1jw+LTmozhuG2eeuzuW5A7DqxD/UXW2ucjmNJqnqOuh8rdPzM3MQXoF8AfFCedJdCUw=="
       },
       "Microsoft.SourceLink.GitHub": {
         "type": "Direct",

src/DistributedLock.Postgres/DistributedLock.Postgres.csproj

@@ -1,7 +1,7 @@
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks>netstandard2.0;netstandard2.1;net462</TargetFrameworks>
+    <TargetFrameworks>netstandard2.0;netstandard2.1;net462;net8.0</TargetFrameworks>
     <RootNamespace>Medallion.Threading.Postgres</RootNamespace>
     <GenerateDocumentationFile>True</GenerateDocumentationFile>
     <WarningLevel>4</WarningLevel>
@@ -11,7 +11,7 @@
   </PropertyGroup>
 
   <PropertyGroup>
-    <Version>1.1.0</Version>
+    <Version>1.2.0</Version>
     <AssemblyVersion>1.0.0.0</AssemblyVersion>
     <Authors>Michael Adelson</Authors>
     <Description>Provides a distributed lock implementation based on Postgresql</Description>
@@ -44,18 +44,23 @@
     <DefineConstants>TRACE;DEBUG</DefineConstants>
   </PropertyGroup>
 
+  <ItemGroup Condition="'$(TargetFramework)' == 'net8.0'">
+    <AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Shipped.txt" />
+    <AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Unshipped.txt" />
+  </ItemGroup>
+
   <ItemGroup>
     <PackageReference Include="Npgsql" />
-    <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All"/>
+    <PackageReference Include="Microsoft.SourceLink.GitHub" PrivateAssets="All" />
     <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" PrivateAssets="All" />
   </ItemGroup>
-  
+
   <ItemGroup>
     <ProjectReference Include="..\DistributedLock.Core\DistributedLock.Core.csproj" />
   </ItemGroup>
 
   <ItemGroup>
-    <Using Remove="System.Net.Http"/>
+    <Using Remove="System.Net.Http" />
   </ItemGroup>
 
   <Import Project="..\FixDistributedLockCoreDependencyVersion.targets" />

src/DistributedLock.Postgres/PostgresAdvisoryLockKey.cs

@@ -94,12 +94,12 @@ internal long Key
     public bool Equals(PostgresAdvisoryLockKey that) => this.ToTuple().Equals(that.ToTuple());
 
     /// <summary>
-    /// Implements <see cref="Object.Equals(object)"/>
+    /// Implements <see cref="object.Equals(object)"/>
     /// </summary>
-    public override bool Equals(object obj) => obj is PostgresAdvisoryLockKey that && this.Equals(that);
+    public override bool Equals(object? obj) => obj is PostgresAdvisoryLockKey that && this.Equals(that);
 
     /// <summary>
-    /// Implements <see cref="Object.GetHashCode"/>
+    /// Implements <see cref="object.GetHashCode"/>
     /// </summary>
     public override int GetHashCode() => this.ToTuple().GetHashCode();
 
@@ -116,7 +116,7 @@ internal long Key
 
     /// <summary>
     /// Returns a string representation of the key that can be round-tripped through
-    /// <see cref="PostgresAdvisoryLockKey(String, Boolean)"/>
+    /// <see cref="PostgresAdvisoryLockKey(string, bool)"/>
     /// </summary>
     public override string ToString() => this._keyEncoding switch
     {

src/DistributedLock.Postgres/PostgresConnectionOptionsBuilder.cs

@@ -15,10 +15,10 @@ internal PostgresConnectionOptionsBuilder() { }
 
     /// <summary>
     /// Some Postgres setups have automation in place which aggressively kills idle connections.
-    /// 
-    /// To prevent this, this option sets the cadence at which we run a no-op "keepalive" query on a connection that is holding a lock. 
+    ///
+    /// To prevent this, this option sets the cadence at which we run a no-op "keepalive" query on a connection that is holding a lock.
     /// Note that this still does not guarantee protection for the connection from all conditions where the governor might kill it.
-    /// 
+    ///
     /// Defaults to <see cref="Timeout.InfiniteTimeSpan"/>, which disables keepalive.
     /// </summary>
     public PostgresConnectionOptionsBuilder KeepaliveCadence(TimeSpan keepaliveCadence)
@@ -29,12 +29,12 @@ public PostgresConnectionOptionsBuilder KeepaliveCadence(TimeSpan keepaliveCaden
 
     /// <summary>
     /// Whether the synchronization should use a transaction scope rather than a session scope. Defaults to false.
-    /// 
+    ///
     /// Synchronizing based on a transaction is necessary to do distributed locking with some pgbouncer configurations
-    /// (see https://github.com/madelson/DistributedLock/issues/168#issuecomment-1823277173). It may also be marginally less 
-    /// expensive than using a connection for a single lock because releasing requires only disposing the 
+    /// (see https://github.com/madelson/DistributedLock/issues/168#issuecomment-1823277173). It may also be marginally less
+    /// expensive than using a connection for a single lock because releasing requires only disposing the
     /// underlying <see cref="IDbTransaction"/>.
-    /// 
+    ///
     /// The disadvantage of this strategy is that it is incompatible with <see cref="UseMultiplexing(bool)"/> and therefore
     /// gives up the advantages of that approach.
     /// </summary>
@@ -46,16 +46,16 @@ public PostgresConnectionOptionsBuilder UseTransaction(bool useTransaction = tru
 
     /// <summary>
     /// This mode takes advantage of the fact that while "holding" a lock (or other synchronization primitive)
-    /// a connection is essentially idle. Thus, rather than creating a new connection for each held lock it is 
+    /// a connection is essentially idle. Thus, rather than creating a new connection for each held lock it is
     /// often possible to multiplex a shared connection so that that connection can hold multiple locks at the same time.
-    /// 
+    ///
     /// Multiplexing is on by default.
-    /// 
+    ///
     /// This is implemented in such a way that releasing a lock held on such a connection will never be blocked by an
     /// Acquire() call that is waiting to acquire a lock on that same connection. For this reason, the multiplexing
-    /// strategy is "optimistic": if the lock can't be acquired instantaneously on the shared connection, a new (shareable) 
+    /// strategy is "optimistic": if the lock can't be acquired instantaneously on the shared connection, a new (shareable)
     /// connection will be allocated.
-    /// 
+    ///
     /// This option can improve performance and avoid connection pool starvation in high-load scenarios. It is also
     /// particularly applicable to cases where <see cref="IDistributedLock.TryAcquire(TimeSpan, System.Threading.CancellationToken)"/>
     /// semantics are used with a zero-length timeout.
@@ -66,12 +66,13 @@ public PostgresConnectionOptionsBuilder UseMultiplexing(bool useMultiplexing = t
         return this;
     }
 
-    internal static (TimeoutValue keepaliveCadence, bool useTransaction, bool useMultiplexing) GetOptions(Action<PostgresConnectionOptionsBuilder>? optionsBuilder)
+    internal static (TimeoutValue keepaliveCadence, bool useTransaction, bool useMultiplexing) GetOptions(
+        Action<PostgresConnectionOptionsBuilder>? optionsBuilder)
     {
         PostgresConnectionOptionsBuilder? options;
         if (optionsBuilder != null)
         {
-            options = new PostgresConnectionOptionsBuilder();
+            options = new();
             optionsBuilder(options);
         }
         else
@@ -90,4 +91,4 @@ internal static (TimeoutValue keepaliveCadence, bool useTransaction, bool useMul
 
         return (keepaliveCadence, useTransaction, useMultiplexing);
     }
-}
+}

src/DistributedLock.Postgres/PostgresDatabaseConnection.cs

@@ -2,6 +2,9 @@
 using Medallion.Threading.Internal.Data;
 using Npgsql;
 using System.Data;
+#if NET7_0_OR_GREATER
+using System.Data.Common;
+#endif
 
 namespace Medallion.Threading.Postgres;
 
@@ -17,6 +20,13 @@ public PostgresDatabaseConnection(IDbTransaction transaction)
     {
     }
 
+#if NET7_0_OR_GREATER
+    public PostgresDatabaseConnection(DbDataSource dbDataSource)
+        : base(dbDataSource.CreateConnection(), isExternallyOwned: false)
+    {
+    }
+#endif
+
     public PostgresDatabaseConnection(string connectionString)
         : base(new NpgsqlConnection(connectionString), isExternallyOwned: false)
     {
@@ -64,4 +74,4 @@ public override async Task SleepAsync(TimeSpan sleepTime, CancellationToken canc
             }
         }
     }
-}
+}

src/DistributedLock.Postgres/PostgresDistributedLock.cs

@@ -1,6 +1,9 @@
 using Medallion.Threading.Internal;
 using Medallion.Threading.Internal.Data;
 using System.Data;
+#if NET7_0_OR_GREATER
+using System.Data.Common;
+#endif
 
 namespace Medallion.Threading.Postgres;
 
@@ -29,6 +32,19 @@ public PostgresDistributedLock(PostgresAdvisoryLockKey key, IDbConnection connec
     {
     }
 
+#if NET7_0_OR_GREATER
+    /// <summary>
+    /// Constructs a lock with the given <paramref name="key"/> (effectively the lock name) and <paramref name="dbDataSource"/>,
+    /// and <paramref name="options"/>.
+    /// 
+    /// Not compatible with connection multiplexing.
+    /// </summary>
+    public PostgresDistributedLock(PostgresAdvisoryLockKey key, DbDataSource dbDataSource, Action<PostgresConnectionOptionsBuilder>? options = null)
+        : this(key, CreateInternalLock(key, dbDataSource, options))
+    {
+    }
+#endif
+
     private PostgresDistributedLock(PostgresAdvisoryLockKey key, IDbDistributedLock internalLock)
     {
         this.Key = key;
@@ -61,4 +77,21 @@ internal static IDbDistributedLock CreateInternalLock(PostgresAdvisoryLockKey ke
         if (connection == null) { throw new ArgumentNullException(nameof(connection)); }
         return new DedicatedConnectionOrTransactionDbDistributedLock(key.ToString(), () => new PostgresDatabaseConnection(connection));
     }
-}
+
+#if NET7_0_OR_GREATER
+    internal static IDbDistributedLock CreateInternalLock(PostgresAdvisoryLockKey key, DbDataSource dbDataSource, Action<PostgresConnectionOptionsBuilder>? options)
+    {
+        if (dbDataSource == null) { throw new ArgumentNullException(nameof(dbDataSource)); }
+
+        // Multiplexing is currently incompatible with DbDataSource, so default it to false
+        var originalOptions = options;
+        options = o => { o.UseMultiplexing(false); originalOptions?.Invoke(o); };
+
+        var (keepaliveCadence, useTransaction, useMultiplexing) = PostgresConnectionOptionsBuilder.GetOptions(options);
+
+        return useMultiplexing
+            ? throw new NotSupportedException("Multiplexing is current incompatible with DbDataSource.")
+            : new DedicatedConnectionOrTransactionDbDistributedLock(key.ToString(), () => new PostgresDatabaseConnection(dbDataSource), useTransaction: useTransaction, keepaliveCadence);
+    }
+#endif
+}