From 87a1129f5901a38085a35db98eabfc2250c061cd Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 6 Jun 2025 12:04:53 -0700 Subject: [PATCH 01/28] Add get flow and basic tests. --- .../netfx/src/Microsoft.Data.SqlClient.csproj | 1 + .../Data/ProviderBase/DbConnectionInternal.cs | 2 + .../ConnectionPool/ChannelDbConnectionPool.cs | 351 +++++++++++++++++- .../ChannelDbConnectionPoolTest.cs | 230 ++++++++++-- tools/props/Versions.props | 3 +- 5 files changed, 548 insertions(+), 39 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj index ea4950329e..b00e79ab53 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj @@ -1025,6 +1025,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index b4dfb4f214..3f805b94b2 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -97,6 +97,8 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all #region Properties + internal DateTime CreateTime => _createTime; + internal bool AllowSetConnectionString { get; } internal bool CanBePooled => !IsConnectionDoomed && !_cannotBePooled && !_owningObject.TryGetTarget(out _); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index c4772bb736..ebd1d38e2c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -4,11 +4,16 @@ using System; using System.Collections.Concurrent; using System.Data.Common; +using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Channels; using System.Threading.Tasks; using System.Transactions; using Microsoft.Data.Common; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; +using static Microsoft.Data.SqlClient.ConnectionPool.DbConnectionPoolState; #nullable enable @@ -25,47 +30,125 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool public ConcurrentDictionary AuthenticationContexts => throw new NotImplementedException(); /// - public DbConnectionFactory ConnectionFactory => throw new NotImplementedException(); + public DbConnectionFactory ConnectionFactory => _connectionFactory; /// - public int Count => throw new NotImplementedException(); + public int Count => _numConnections; /// public bool ErrorOccurred => throw new NotImplementedException(); /// - public int Id => throw new NotImplementedException(); + public int Id => _objectID; /// - public DbConnectionPoolIdentity Identity => throw new NotImplementedException(); + public DbConnectionPoolIdentity Identity => _identity; /// - public bool IsRunning => throw new NotImplementedException(); + public bool IsRunning => State is Running; /// public TimeSpan LoadBalanceTimeout => throw new NotImplementedException(); /// - public DbConnectionPoolGroup PoolGroup => throw new NotImplementedException(); + public DbConnectionPoolGroup PoolGroup => _connectionPoolGroup; /// - public DbConnectionPoolGroupOptions PoolGroupOptions => throw new NotImplementedException(); + public DbConnectionPoolGroupOptions PoolGroupOptions => _connectionPoolGroupOptions; /// - public DbConnectionPoolProviderInfo ProviderInfo => throw new NotImplementedException(); + public DbConnectionPoolProviderInfo ProviderInfo => _connectionPoolProviderInfo; /// public DbConnectionPoolState State { - get => throw new NotImplementedException(); - set => throw new NotImplementedException(); + get; + set; } /// public bool UseLoadBalancing => throw new NotImplementedException(); + + private int MaxPoolSize => PoolGroupOptions.MaxPoolSize; #endregion + #region Fields + private readonly DbConnectionPoolIdentity _identity; + + private readonly DbConnectionFactory _connectionFactory; + private readonly DbConnectionPoolGroup _connectionPoolGroup; + private readonly DbConnectionPoolGroupOptions _connectionPoolGroupOptions; + private DbConnectionPoolProviderInfo _connectionPoolProviderInfo; + + /// + /// The private member which carries the set of authenticationcontexts for this pool (based on the user's identity). + /// + private readonly ConcurrentDictionary _pooledDbAuthenticationContexts; + + // Prevents synchronous operations which depend on async operations on managed + // threads from blocking on all available threads, which would stop async tasks + // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance + // between sync and async tasks. + private static SemaphoreSlim SyncOverAsyncSemaphore { get; } = new(Math.Max(1, Environment.ProcessorCount / 2)); + + private static int _objectTypeCount; // EventSource counter + + private readonly int _objectID = Interlocked.Increment(ref _objectTypeCount); + + /// + /// Tracks all connections currently managed by this pool, whether idle or busy. + /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts. + /// + private readonly DbConnectionInternal?[] _connections; + + /// + /// Reader side for the idle connection channel. Contains nulls in order to release waiting attempts after + /// a connection has been physically closed/broken. + /// + private readonly ChannelReader _idleConnectionReader; + private readonly ChannelWriter _idleConnectionWriter; + + // Counts the total number of open connections tracked by the pool. + private volatile int _numConnections; + + // Counts the number of connections currently sitting idle in the pool. + private volatile int _idleCount; + #endregion + + + /// + /// Initializes a new PoolingDataSource. + /// + internal ChannelDbConnectionPool( + DbConnectionFactory connectionFactory, + DbConnectionPoolGroup connectionPoolGroup, + DbConnectionPoolIdentity identity, + DbConnectionPoolProviderInfo connectionPoolProviderInfo) + { + State = Initializing; + + _connectionFactory = connectionFactory; + _connectionPoolGroup = connectionPoolGroup; + _connectionPoolGroupOptions = connectionPoolGroup.PoolGroupOptions; + _connectionPoolProviderInfo = connectionPoolProviderInfo; + _identity = identity; + _pooledDbAuthenticationContexts = new ConcurrentDictionary< + DbConnectionPoolAuthenticationContextKey, + DbConnectionPoolAuthenticationContext>( + concurrencyLevel: 4 * Environment.ProcessorCount, + capacity: 2); + _connections = new DbConnectionInternal[MaxPoolSize]; + + // We enforce Max Pool Size, so no need to to create a bounded channel (which is less efficient) + // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents + // On the producing side, we have connections being released back into the pool (both multiplexing and not) + var idleChannel = Channel.CreateUnbounded(); + _idleConnectionReader = idleChannel.Reader; + _idleConnectionWriter = idleChannel.Writer; + + State = Running; + } #region Methods /// @@ -111,9 +194,253 @@ public void TransactionEnded(Transaction transaction, DbConnectionInternal trans } /// - public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal connection) + public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) { - throw new NotImplementedException(); + if (taskCompletionSource is not null) + { + Task.Run(async () => + { + //TODO: use same timespan everywhere and tick down for queueuing and actual connection opening work + try + { + var connection = await GetInternalConnection(owningObject, userOptions, TimeSpan.FromSeconds(owningObject.ConnectionTimeout), true, CancellationToken.None).ConfigureAwait(false); + taskCompletionSource.SetResult(connection); + } + catch (Exception e) + { + taskCompletionSource.SetException(e); + } + }); + connection = null; + return false; + } + else + { + //TODO: use same timespan everywhere and tick down for queueuing and actual connection opening work + var task = GetInternalConnection(owningObject, userOptions, TimeSpan.FromSeconds(owningObject.ConnectionTimeout), false, CancellationToken.None); + //TODO: move sync over async limit to this spot? + connection = task.GetAwaiter().GetResult(); + return connection is not null; + } + } + + internal async Task GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) + { + DbConnectionInternal? connection = null; + + //TODO: check transacted pool + + connection ??= GetIdleConnection(); + + connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); + + if (connection != null) + { + // TODO: set connection internal state + // PrepareConnection(owningConnection, connection, transaction); + return connection; + } + + // We're at max capacity. Block on the idle channel with a timeout. + // Note that Channels guarantee fair FIFO behavior to callers of ReadAsync (first-come first- + // served), which is crucial to us. + using CancellationTokenSource linkedSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + CancellationToken finalToken = linkedSource.Token; + linkedSource.CancelAfter(timeout); + //TODO: respect remaining time, linkedSource.CancelAfter(timeout.CheckAndGetTimeLeft()); + + try + { + while (true) + { + try + { + if (async) + { + connection = await _idleConnectionReader.ReadAsync(finalToken).ConfigureAwait(false); + } + else + { + SyncOverAsyncSemaphore.Wait(finalToken); + try + { + // If there are no connections in the channel, then this call will block until one is available. + // Because this call uses the managed thread pool, we need to limit the number of + // threads allowed to block here to avoid a deadlock. + ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = + _idleConnectionReader.ReadAsync(finalToken).ConfigureAwait(false).GetAwaiter(); + using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); + + // Cancellation happens through the ReadAsync call, which will complete the task. + awaiter.UnsafeOnCompleted(() => mres.Set()); + mres.Wait(CancellationToken.None); + connection = awaiter.GetResult(); + } + finally + { + SyncOverAsyncSemaphore.Release(); + } + } + + if (connection != null && CheckIdleConnection(connection)) + { + //TODO: set connection internal state + //PrepareConnection(owningConnection, connection, transaction); + return connection; + } + } + catch (OperationCanceledException) + { + cancellationToken.ThrowIfCancellationRequested(); + Debug.Assert(finalToken.IsCancellationRequested); + + throw ADP.PooledOpenTimeout(); + } + catch (ChannelClosedException) + { + //TODO: exceptions from resource file + throw new Exception("The connection pool has been shut down."); + } + + // If we're here, our waiting attempt on the idle connection channel was released with a null + // (or bad connection), or we're in sync mode. Check again if a new idle connection has appeared since we last checked. + connection = GetIdleConnection(); + + // We might have closed a connection in the meantime and no longer be at max capacity + // so try to open a new connection and if that fails, loop again. + connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); + + if (connection != null) + { + //TODO: set connection internal state + //PrepareConnection(owningConnection, connection, transaction); + return connection; + } + } + } + finally + { + //TODO: log error + } + } + + /// + /// Tries to read a connection from the idle connection channel. + /// + /// Returns true if a valid idles connection is found, otherwise returns false. + /// TODO: profile the inlining to see if it's necessary + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private DbConnectionInternal? GetIdleConnection() + { + + while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) + { + if (CheckIdleConnection(connection)) + { + return connection; + } + } + + return null; + } + + /// + /// Checks that the provided connection is live and unexpired and closes it if needed. + /// Decrements the idle count as long as the connection is not null. + /// + /// The connection to be checked. + /// Returns true if the connection is live and unexpired, otherwise returns false. + /// TODO: profile the inlining to see if it's necessary + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool CheckIdleConnection(DbConnectionInternal? connection) + { + if (connection is null) + { + return false; + } + + // Only decrement when the connection has a value. + Interlocked.Decrement(ref _idleCount); + + //TODO: check if the connection is expired + //return CheckConnection(connection); + + return true; + } + + /// + internal Task OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) + { + // As long as we're under max capacity, attempt to increase the connection count and open a new connection. + for (var numConnections = _numConnections; numConnections < MaxPoolSize; numConnections = _numConnections) + { + // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 + if (Interlocked.CompareExchange(ref _numConnections, numConnections + 1, numConnections) != numConnections) + { + continue; + } + + try + { + // We've managed to increase the open counter, open a physical connection. + var startTime = Stopwatch.GetTimestamp(); + + // TODO: This blocks the thread for several network calls! + // This will be unexpected to async callers. + // Our options are limited because DbConnectionInternal doesn't support async open. + // It's better to block this thread and keep throughput high than to queue all of our opens onto a single worker thread. + // Add an async path when this support is added to DbConnectionInternal. + DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( + this, + owningConnection, + _connectionPoolGroup.ConnectionOptions, + _connectionPoolGroup.PoolKey, + userOptions); + + if (newConnection == null) + { + throw ADP.InternalError(ADP.InternalErrorCode.CreateObjectReturnedNull); // CreateObject succeeded, but null object + } + if (!newConnection.CanBePooled) + { + throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); // CreateObject succeeded, but non-poolable object + } + + newConnection.PrePush(null); + + int i; + for (i = 0; i < MaxPoolSize; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], newConnection, null) == null) + { + break; + } + } + + Debug.Assert(i < MaxPoolSize, $"Could not find free slot in {_connections} when opening."); + if (i == MaxPoolSize) + { + //TODO: generic exception? + throw new Exception($"Could not find free slot in {_connections} when opening. Please report a bug."); + } + + return Task.FromResult(newConnection); + } + catch + { + // Physical open failed, decrement the open and busy counter back down. + Interlocked.Decrement(ref _numConnections); + + // In case there's a waiting attempt on the channel, we write a null to the idle connection channel + // to wake it up, so it will try opening (and probably throw immediately) + // Statement order is important since we have synchronous completions on the channel. + _idleConnectionWriter.TryWrite(null); + + throw; + } + } + + return Task.FromResult(null); } #endregion } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 2dcfe476fe..a05a300c48 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -4,8 +4,11 @@ using System; using System.Data.Common; +using System.Threading; using System.Threading.Tasks; using System.Transactions; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; using Xunit; @@ -13,143 +16,318 @@ namespace Microsoft.Data.SqlClient.UnitTests { public class ChannelDbConnectionPoolTest { - private readonly ChannelDbConnectionPool _pool; + private readonly ChannelDbConnectionPool pool; + private readonly DbConnectionPoolGroup dbConnectionPoolGroup; + private readonly DbConnectionPoolGroupOptions poolGroupOptions; + private readonly DbConnectionFactory connectionFactory; + private readonly DbConnectionPoolIdentity identity; + private readonly DbConnectionPoolProviderInfo connectionPoolProviderInfo; public ChannelDbConnectionPoolTest() { - _pool = new ChannelDbConnectionPool(); + // Use a stubbed connection factory to avoid network code + connectionFactory = new ConnectionFactoryStub(); + identity = DbConnectionPoolIdentity.NoIdentity; + connectionPoolProviderInfo = new DbConnectionPoolProviderInfo(); + poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 50, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true + ); + dbConnectionPoolGroup = new DbConnectionPoolGroup( + new DbConnectionOptions("DataSource=localhost;", null), + new DbConnectionPoolKey("TestDataSource"), + poolGroupOptions + ); + pool = new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + identity, + connectionPoolProviderInfo + ); } + [Theory] + [InlineData(1)] + [InlineData(5)] + [InlineData(10)] + public async Task TestGetConnectionFromEmptyPoolSync_ShouldCreateNewConnection(int numConnections) + { + // Act + for (int i = 0; i < numConnections; i++) + { + var internalConnection = await pool.GetInternalConnection( + new SqlConnection(), + new DbConnectionOptions("", null), + TimeSpan.Zero, + async: false, + CancellationToken.None + ); + + // Assert + Assert.NotNull(internalConnection); + } + + + // Assert + Assert.Equal(numConnections, pool.Count); + } + + [Theory] + [InlineData(1)] + [InlineData(5)] + [InlineData(10)] + public async Task TestGetConnectionFromEmptyPoolAsync_ShouldCreateNewConnection(int numConnections) + { + // Act + for (int i = 0; i < numConnections; i++) + { + var internalConnection = await pool.GetInternalConnection( + new SqlConnection(), + new DbConnectionOptions("", null), + TimeSpan.Zero, + async: false, + CancellationToken.None + ); + + // Assert + Assert.NotNull(internalConnection); + } + + + // Assert + Assert.Equal(numConnections, pool.Count); + } + + #region Property Tests + [Fact] public void TestAuthenticationContexts() { - Assert.Throws(() => _ = _pool.AuthenticationContexts); + Assert.Throws(() => _ = pool.AuthenticationContexts); } [Fact] public void TestConnectionFactory() { - Assert.Throws(() => _ = _pool.ConnectionFactory); + Assert.Equal(connectionFactory, pool.ConnectionFactory); } [Fact] public void TestCount() { - Assert.Throws(() => _ = _pool.Count); + Assert.Equal(0, pool.Count); } [Fact] public void TestErrorOccurred() { - Assert.Throws(() => _ = _pool.ErrorOccurred); + Assert.Throws(() => _ = pool.ErrorOccurred); } [Fact] public void TestId() { - Assert.Throws(() => _ = _pool.Id); + Assert.True(pool.Id >= 1); } [Fact] public void TestIdentity() { - Assert.Throws(() => _ = _pool.Identity); + Assert.Equal(identity, pool.Identity); } [Fact] public void TestIsRunning() { - Assert.Throws(() => _ = _pool.IsRunning); + Assert.True(pool.IsRunning); } [Fact] public void TestLoadBalanceTimeout() { - Assert.Throws(() => _ = _pool.LoadBalanceTimeout); + Assert.Throws(() => _ = pool.LoadBalanceTimeout); } [Fact] public void TestPoolGroup() { - Assert.Throws(() => _ = _pool.PoolGroup); + Assert.Equal(dbConnectionPoolGroup, pool.PoolGroup); } [Fact] public void TestPoolGroupOptions() { - Assert.Throws(() => _ = _pool.PoolGroupOptions); + Assert.Equal(poolGroupOptions, pool.PoolGroupOptions); } [Fact] public void TestProviderInfo() { - Assert.Throws(() => _ = _pool.ProviderInfo); + Assert.Equal(connectionPoolProviderInfo, pool.ProviderInfo); } [Fact] public void TestStateGetter() { - Assert.Throws(() => _ = _pool.State); + Assert.Equal(DbConnectionPoolState.Running, pool.State); } [Fact] public void TestStateSetter() { - Assert.Throws(() => _pool.State = DbConnectionPoolState.Running); + pool.State = DbConnectionPoolState.ShuttingDown; + Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); + pool.State = DbConnectionPoolState.Running; + Assert.Equal(DbConnectionPoolState.Running, pool.State); } [Fact] public void TestUseLoadBalancing() { - Assert.Throws(() => _ = _pool.UseLoadBalancing); + Assert.Throws(() => _ = pool.UseLoadBalancing); } + #endregion + + #region Not Implemented Method Tests + [Fact] public void TestClear() { - Assert.Throws(() => _pool.Clear()); + Assert.Throws(() => pool.Clear()); } [Fact] public void TestPutObjectFromTransactedPool() { - Assert.Throws(() => _pool.PutObjectFromTransactedPool(null!)); + Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); } [Fact] public void TestReplaceConnection() { - Assert.Throws(() => _pool.ReplaceConnection(null!, null!, null!)); + Assert.Throws(() => pool.ReplaceConnection(null!, null!, null!)); } [Fact] public void TestReturnInternalConnection() { - Assert.Throws(() => _pool.ReturnInternalConnection(null!, null!)); + Assert.Throws(() => pool.ReturnInternalConnection(null!, null!)); } [Fact] public void TestShutdown() { - Assert.Throws(() => _pool.Shutdown()); + Assert.Throws(() => pool.Shutdown()); } [Fact] public void TestStartup() { - Assert.Throws(() => _pool.Startup()); + Assert.Throws(() => pool.Startup()); } [Fact] public void TestTransactionEnded() { - Assert.Throws(() => _pool.TransactionEnded(null!, null!)); + Assert.Throws(() => pool.TransactionEnded(null!, null!)); } - [Fact] - public void TestTryGetConnection() + #endregion + + #region Test classes + internal class ConnectionFactoryStub : DbConnectionFactory { - Assert.Throws(() => _pool.TryGetConnection(null!, null!, null!, out _)); + protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) + { + return new TestDbConnectionInternal(); + } + + #region Not Implemented Members + public override DbProviderFactory ProviderFactory => throw new NotImplementedException(); + + protected override DbConnectionOptions CreateConnectionOptions(string connectionString, DbConnectionOptions previous) + { + throw new NotImplementedException(); + } + + protected override DbConnectionPoolGroupOptions CreateConnectionPoolGroupOptions(DbConnectionOptions options) + { + throw new NotImplementedException(); + } + + protected override int GetObjectId(DbConnection connection) + { + throw new NotImplementedException(); + } + + internal override DbConnectionPoolGroup GetConnectionPoolGroup(DbConnection connection) + { + throw new NotImplementedException(); + } + + internal override DbConnectionInternal GetInnerConnection(DbConnection connection) + { + throw new NotImplementedException(); + } + + internal override void PermissionDemand(DbConnection outerConnection) + { + throw new NotImplementedException(); + } + + internal override void SetConnectionPoolGroup(DbConnection outerConnection, DbConnectionPoolGroup poolGroup) + { + throw new NotImplementedException(); + } + + internal override void SetInnerConnectionEvent(DbConnection owningObject, DbConnectionInternal to) + { + throw new NotImplementedException(); + } + + internal override bool SetInnerConnectionFrom(DbConnection owningObject, DbConnectionInternal to, DbConnectionInternal from) + { + throw new NotImplementedException(); + } + + internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnectionInternal to) + { + throw new NotImplementedException(); + } + #endregion + } + + internal class TestDbConnectionInternal : DbConnectionInternal + { + #region Not Implemented Members + public override string ServerVersion => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction transaction) + { + throw new NotImplementedException(); + } + + protected override void Activate(Transaction transaction) + { + throw new NotImplementedException(); + } + + protected override void Deactivate() + { + throw new NotImplementedException(); + } + #endregion } + #endregion } } diff --git a/tools/props/Versions.props b/tools/props/Versions.props index d4e2619f3b..679cdcd146 100644 --- a/tools/props/Versions.props +++ b/tools/props/Versions.props @@ -24,11 +24,12 @@ 6.0.2 + 4.3.0 4.5.1 4.5.5 8.0.0 8.0.5 - 4.3.0 + 9.0.4 From 4233f943e8631d8a7b39a5014e807cc9b15295dd Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 6 Jun 2025 15:45:19 -0700 Subject: [PATCH 02/28] Add tests. Clean up diff. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 47 ++---- .../ChannelDbConnectionPoolTest.cs | 135 ++++++++++++++++-- 2 files changed, 133 insertions(+), 49 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ebd1d38e2c..9ccd344d50 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -110,9 +110,6 @@ public DbConnectionPoolState State // Counts the total number of open connections tracked by the pool. private volatile int _numConnections; - - // Counts the number of connections currently sitting idle in the pool. - private volatile int _idleCount; #endregion @@ -140,7 +137,7 @@ internal ChannelDbConnectionPool( _connections = new DbConnectionInternal[MaxPoolSize]; - // We enforce Max Pool Size, so no need to to create a bounded channel (which is less efficient) + // We enforce Max Pool Size, so no need to create a bounded channel (which is less efficient) // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents // On the producing side, we have connections being released back into the pool (both multiplexing and not) var idleChannel = Channel.CreateUnbounded(); @@ -198,6 +195,11 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource and let the caller await it as needed, + // but we need to signal to the provided TaskCompletionSource when the connection is established. + // This pattern has implications for connection open retry logic that are intricate enough to merit + // dedicated work. Task.Run(async () => { //TODO: use same timespan everywhere and tick down for queueuing and actual connection opening work @@ -224,7 +226,7 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) + private async Task GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) { DbConnectionInternal? connection = null; @@ -282,7 +284,8 @@ internal async Task GetInternalConnection(DbConnection own } } - if (connection != null && CheckIdleConnection(connection)) + // TODO: check if connection is still valid + if (connection != null) { //TODO: set connection internal state //PrepareConnection(owningConnection, connection, transaction); @@ -332,42 +335,16 @@ internal async Task GetInternalConnection(DbConnection own [MethodImpl(MethodImplOptions.AggressiveInlining)] private DbConnectionInternal? GetIdleConnection() { - while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { - if (CheckIdleConnection(connection)) - { - return connection; - } + // TODO: check if connection is still valid + // if (CheckIdleConnection(connection)) + return connection; } return null; } - /// - /// Checks that the provided connection is live and unexpired and closes it if needed. - /// Decrements the idle count as long as the connection is not null. - /// - /// The connection to be checked. - /// Returns true if the connection is live and unexpired, otherwise returns false. - /// TODO: profile the inlining to see if it's necessary - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private bool CheckIdleConnection(DbConnectionInternal? connection) - { - if (connection is null) - { - return false; - } - - // Only decrement when the connection has a value. - Interlocked.Decrement(ref _idleCount); - - //TODO: check if the connection is expired - //return CheckConnection(connection); - - return true; - } - /// internal Task OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index a05a300c48..7c180c1280 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -26,7 +26,7 @@ public class ChannelDbConnectionPoolTest public ChannelDbConnectionPoolTest() { // Use a stubbed connection factory to avoid network code - connectionFactory = new ConnectionFactoryStub(); + connectionFactory = new SuccessfulConnectionFactory(); identity = DbConnectionPoolIdentity.NoIdentity; connectionPoolProviderInfo = new DbConnectionPoolProviderInfo(); poolGroupOptions = new DbConnectionPoolGroupOptions( @@ -54,20 +54,21 @@ public ChannelDbConnectionPoolTest() [InlineData(1)] [InlineData(5)] [InlineData(10)] - public async Task TestGetConnectionFromEmptyPoolSync_ShouldCreateNewConnection(int numConnections) + public void TestGetConnectionFromEmptyPoolSync_ShouldCreateNewConnection(int numConnections) { // Act for (int i = 0; i < numConnections; i++) { - var internalConnection = await pool.GetInternalConnection( + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( new SqlConnection(), + taskCompletionSource: null, new DbConnectionOptions("", null), - TimeSpan.Zero, - async: false, - CancellationToken.None + out internalConnection ); // Assert + Assert.True(completed); Assert.NotNull(internalConnection); } @@ -85,16 +86,19 @@ public async Task TestGetConnectionFromEmptyPoolAsync_ShouldCreateNewConnection( // Act for (int i = 0; i < numConnections; i++) { - var internalConnection = await pool.GetInternalConnection( + var tcs = new TaskCompletionSource(); + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( new SqlConnection(), + tcs, new DbConnectionOptions("", null), - TimeSpan.Zero, - async: false, - CancellationToken.None + out internalConnection ); // Assert - Assert.NotNull(internalConnection); + Assert.False(completed); + Assert.Null(internalConnection); + Assert.NotNull(await tcs.Task); } @@ -102,6 +106,9 @@ public async Task TestGetConnectionFromEmptyPoolAsync_ShouldCreateNewConnection( Assert.Equal(numConnections, pool.Count); } + // Test that requests to get connection from the pool fails when max pool size is reached + + #region Property Tests [Fact] @@ -236,15 +243,53 @@ public void TestTransactionEnded() { Assert.Throws(() => pool.TransactionEnded(null!, null!)); } + [Fact] + public void TestGetConnectionFailsWhenMaxPoolSizeReached() + { + // Arrange + for (int i = 0; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + try + { + // Act + DbConnectionInternal extraConnection = null; + var exceeded = pool.TryGetConnection( + //TODO: set timeout to make this faster + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out extraConnection + ); + } catch (Exception ex) + { + // Assert + Assert.IsType(ex); + Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); + } + + // Assert + Assert.Equal(poolGroupOptions.MaxPoolSize, pool.Count); + } #endregion #region Test classes - internal class ConnectionFactoryStub : DbConnectionFactory + internal class SuccessfulConnectionFactory : DbConnectionFactory { protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) { - return new TestDbConnectionInternal(); + return new SuccessfulDbConnectionInternal(); } #region Not Implemented Members @@ -302,7 +347,7 @@ internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnect #endregion } - internal class TestDbConnectionInternal : DbConnectionInternal + internal class SuccessfulDbConnectionInternal : DbConnectionInternal { #region Not Implemented Members public override string ServerVersion => throw new NotImplementedException(); @@ -328,6 +373,68 @@ protected override void Deactivate() } #endregion } + + internal class TimeoutConnectionFactory : DbConnectionFactory + { + protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) + { + return new SuccessfulDbConnectionInternal(); + } + + #region Not Implemented Members + public override DbProviderFactory ProviderFactory => throw new NotImplementedException(); + + protected override DbConnectionOptions CreateConnectionOptions(string connectionString, DbConnectionOptions previous) + { + throw new NotImplementedException(); + } + + protected override DbConnectionPoolGroupOptions CreateConnectionPoolGroupOptions(DbConnectionOptions options) + { + throw new NotImplementedException(); + } + + protected override int GetObjectId(DbConnection connection) + { + throw new NotImplementedException(); + } + + internal override DbConnectionPoolGroup GetConnectionPoolGroup(DbConnection connection) + { + throw new NotImplementedException(); + } + + internal override DbConnectionInternal GetInnerConnection(DbConnection connection) + { + throw new NotImplementedException(); + } + + internal override void PermissionDemand(DbConnection outerConnection) + { + throw new NotImplementedException(); + } + + internal override void SetConnectionPoolGroup(DbConnection outerConnection, DbConnectionPoolGroup poolGroup) + { + throw new NotImplementedException(); + } + + internal override void SetInnerConnectionEvent(DbConnection owningObject, DbConnectionInternal to) + { + throw new NotImplementedException(); + } + + internal override bool SetInnerConnectionFrom(DbConnection owningObject, DbConnectionInternal to, DbConnectionInternal from) + { + throw new NotImplementedException(); + } + + internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnectionInternal to) + { + throw new NotImplementedException(); + } + #endregion + } #endregion } } From fb8c9d0854e3c1665fae7b845425381aaac94778 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 9 Jun 2025 11:24:19 -0700 Subject: [PATCH 03/28] Add additional tests. Refactor to test error behavior. --- .../ChannelDbConnectionPoolTest.cs | 232 +++++++++++------- 1 file changed, 150 insertions(+), 82 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 7c180c1280..1b3512a3e7 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -7,6 +7,7 @@ using System.Threading; using System.Threading.Tasks; using System.Transactions; +using Microsoft.Data.Common; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; @@ -16,17 +17,19 @@ namespace Microsoft.Data.SqlClient.UnitTests { public class ChannelDbConnectionPoolTest { - private readonly ChannelDbConnectionPool pool; - private readonly DbConnectionPoolGroup dbConnectionPoolGroup; - private readonly DbConnectionPoolGroupOptions poolGroupOptions; - private readonly DbConnectionFactory connectionFactory; - private readonly DbConnectionPoolIdentity identity; - private readonly DbConnectionPoolProviderInfo connectionPoolProviderInfo; - - public ChannelDbConnectionPoolTest() + private ChannelDbConnectionPool pool; + private DbConnectionFactory connectionFactory; + private DbConnectionPoolGroup dbConnectionPoolGroup; + private DbConnectionPoolGroupOptions poolGroupOptions; + private DbConnectionPoolIdentity identity; + private DbConnectionPoolProviderInfo connectionPoolProviderInfo; + + private static readonly DbConnectionFactory SuccessfulConnectionFactory = new SuccessfulDbConnectionFactory(); + private static readonly DbConnectionFactory TimeoutConnectionFactory = new TimeoutDbConnectionFactory(); + + private void Setup(DbConnectionFactory connectionFactory) { - // Use a stubbed connection factory to avoid network code - connectionFactory = new SuccessfulConnectionFactory(); + this.connectionFactory = connectionFactory; identity = DbConnectionPoolIdentity.NoIdentity; connectionPoolProviderInfo = new DbConnectionPoolProviderInfo(); poolGroupOptions = new DbConnectionPoolGroupOptions( @@ -54,8 +57,11 @@ public ChannelDbConnectionPoolTest() [InlineData(1)] [InlineData(5)] [InlineData(10)] - public void TestGetConnectionFromEmptyPoolSync_ShouldCreateNewConnection(int numConnections) + public void GetConnectionFromEmptyPoolSync_ShouldCreateNewConnection(int numConnections) { + // Arrange + Setup(SuccessfulConnectionFactory); + // Act for (int i = 0; i < numConnections; i++) { @@ -81,8 +87,11 @@ out internalConnection [InlineData(1)] [InlineData(5)] [InlineData(10)] - public async Task TestGetConnectionFromEmptyPoolAsync_ShouldCreateNewConnection(int numConnections) + public async Task GetConnectionFromEmptyPoolAsync_ShouldCreateNewConnection(int numConnections) { + // Arrange + Setup(SuccessfulConnectionFactory); + // Act for (int i = 0; i < numConnections; i++) { @@ -106,8 +115,106 @@ out internalConnection Assert.Equal(numConnections, pool.Count); } - // Test that requests to get connection from the pool fails when max pool size is reached - + [Fact] + public void GetConnectionRespectsMaxPoolSize() + { + // Arrange + Setup(SuccessfulConnectionFactory); + + for (int i = 0; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + + try + { + // Act + DbConnectionInternal extraConnection = null; + var exceeded = pool.TryGetConnection( + new SqlConnection("Timeout=1"), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out extraConnection + ); + } + catch (Exception ex) + { + // Assert + Assert.IsType(ex); + Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); + } + + // Assert + Assert.Equal(poolGroupOptions.MaxPoolSize, pool.Count); + } + + [Fact] + [ActiveIssue("Requires ReturnInternalConnection to be implemented in ChannelDbConnectionPool")] + public void ConnectionsAreReused() + { + // Arrange + Setup(SuccessfulConnectionFactory); + DbConnectionInternal internalConnection1 = null; + DbConnectionInternal internalConnection2 = null; + + // Act: Get the first connection + var completed1 = pool.TryGetConnection( + new SqlConnection(), + null, + new DbConnectionOptions("", null), + out internalConnection1 + ); + + // Assert: First connection should succeed + Assert.True(completed1); + Assert.NotNull(internalConnection1); + + // Act: Return the first connection to the pool + pool.ReturnInternalConnection(internalConnection1, null); + + // Act: Get the second connection (should reuse the first one) + var completed2 = pool.TryGetConnection( + new SqlConnection(), + null, + new DbConnectionOptions("", null), + out internalConnection2 + ); + + // Assert: Second connection should succeed and reuse the first connection + Assert.True(completed2); + Assert.NotNull(internalConnection2); + Assert.Same(internalConnection1, internalConnection2); + } + + [Fact] + public void GetConnectionWithTimeout_ShouldThrowTimeoutException() + { + // Arrange + Setup(TimeoutConnectionFactory); + DbConnectionInternal internalConnection = null; + + // Act & Assert + var ex = Assert.Throws(() => + { + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + }); + + Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); + } #region Property Tests @@ -243,49 +350,10 @@ public void TestTransactionEnded() { Assert.Throws(() => pool.TransactionEnded(null!, null!)); } - [Fact] - public void TestGetConnectionFailsWhenMaxPoolSizeReached() - { - // Arrange - for (int i = 0; i < poolGroupOptions.MaxPoolSize; i++) - { - DbConnectionInternal internalConnection = null; - var completed = pool.TryGetConnection( - new SqlConnection(), - taskCompletionSource: null, - new DbConnectionOptions("", null), - out internalConnection - ); - - Assert.True(completed); - Assert.NotNull(internalConnection); - } - - try - { - // Act - DbConnectionInternal extraConnection = null; - var exceeded = pool.TryGetConnection( - //TODO: set timeout to make this faster - new SqlConnection(), - taskCompletionSource: null, - new DbConnectionOptions("", null), - out extraConnection - ); - } catch (Exception ex) - { - // Assert - Assert.IsType(ex); - Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); - } - - // Assert - Assert.Equal(poolGroupOptions.MaxPoolSize, pool.Count); - } #endregion #region Test classes - internal class SuccessfulConnectionFactory : DbConnectionFactory + internal class SuccessfulDbConnectionFactory : DbConnectionFactory { protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) { @@ -347,89 +415,89 @@ internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnect #endregion } - internal class SuccessfulDbConnectionInternal : DbConnectionInternal + internal class TimeoutDbConnectionFactory : DbConnectionFactory { - #region Not Implemented Members - public override string ServerVersion => throw new NotImplementedException(); - - public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) { - throw new NotImplementedException(); + throw ADP.PooledOpenTimeout(); } - public override void EnlistTransaction(Transaction transaction) + #region Not Implemented Members + public override DbProviderFactory ProviderFactory => throw new NotImplementedException(); + + protected override DbConnectionOptions CreateConnectionOptions(string connectionString, DbConnectionOptions previous) { throw new NotImplementedException(); } - protected override void Activate(Transaction transaction) + protected override DbConnectionPoolGroupOptions CreateConnectionPoolGroupOptions(DbConnectionOptions options) { throw new NotImplementedException(); } - protected override void Deactivate() + protected override int GetObjectId(DbConnection connection) { throw new NotImplementedException(); } - #endregion - } - internal class TimeoutConnectionFactory : DbConnectionFactory - { - protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) + internal override DbConnectionPoolGroup GetConnectionPoolGroup(DbConnection connection) { - return new SuccessfulDbConnectionInternal(); + throw new NotImplementedException(); } - #region Not Implemented Members - public override DbProviderFactory ProviderFactory => throw new NotImplementedException(); - - protected override DbConnectionOptions CreateConnectionOptions(string connectionString, DbConnectionOptions previous) + internal override DbConnectionInternal GetInnerConnection(DbConnection connection) { throw new NotImplementedException(); } - protected override DbConnectionPoolGroupOptions CreateConnectionPoolGroupOptions(DbConnectionOptions options) + internal override void PermissionDemand(DbConnection outerConnection) { throw new NotImplementedException(); } - protected override int GetObjectId(DbConnection connection) + internal override void SetConnectionPoolGroup(DbConnection outerConnection, DbConnectionPoolGroup poolGroup) { throw new NotImplementedException(); } - internal override DbConnectionPoolGroup GetConnectionPoolGroup(DbConnection connection) + internal override void SetInnerConnectionEvent(DbConnection owningObject, DbConnectionInternal to) { throw new NotImplementedException(); } - internal override DbConnectionInternal GetInnerConnection(DbConnection connection) + internal override bool SetInnerConnectionFrom(DbConnection owningObject, DbConnectionInternal to, DbConnectionInternal from) { throw new NotImplementedException(); } - internal override void PermissionDemand(DbConnection outerConnection) + internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnectionInternal to) { throw new NotImplementedException(); } + #endregion + } - internal override void SetConnectionPoolGroup(DbConnection outerConnection, DbConnectionPoolGroup poolGroup) + internal class SuccessfulDbConnectionInternal : DbConnectionInternal + { + #region Not Implemented Members + public override string ServerVersion => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) { throw new NotImplementedException(); } - internal override void SetInnerConnectionEvent(DbConnection owningObject, DbConnectionInternal to) + public override void EnlistTransaction(Transaction transaction) { throw new NotImplementedException(); } - internal override bool SetInnerConnectionFrom(DbConnection owningObject, DbConnectionInternal to, DbConnectionInternal from) + protected override void Activate(Transaction transaction) { throw new NotImplementedException(); } - internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnectionInternal to) + protected override void Deactivate() { throw new NotImplementedException(); } From 73a379be79b9b79312a3271dda9adc20771c0686 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 9 Jun 2025 12:10:31 -0700 Subject: [PATCH 04/28] Implement return flow and add return/reuse tests. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 185 +++++++++++++++++- .../ChannelDbConnectionPoolTest.cs | 48 +++-- 2 files changed, 207 insertions(+), 26 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9ccd344d50..90c2fc08fa 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -48,7 +48,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool public bool IsRunning => State is Running; /// - public TimeSpan LoadBalanceTimeout => throw new NotImplementedException(); + public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout; /// public DbConnectionPoolGroup PoolGroup => _connectionPoolGroup; @@ -67,7 +67,7 @@ public DbConnectionPoolState State } /// - public bool UseLoadBalancing => throw new NotImplementedException(); + public bool UseLoadBalancing => PoolGroupOptions.UseLoadBalancing; private int MaxPoolSize => PoolGroupOptions.MaxPoolSize; #endregion @@ -169,7 +169,172 @@ public DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConne /// public void ReturnInternalConnection(DbConnectionInternal obj, object owningObject) { - throw new NotImplementedException(); + // Once a connection is closing (which is the state that we're in at + // this point in time) you cannot delegate a transaction to or enlist + // a transaction in it, so we can correctly presume that if there was + // not a delegated or enlisted transaction to start with, that there + // will not be a delegated or enlisted transaction once we leave the + // lock. + + lock (obj) + { + // Calling PrePush prevents the object from being reclaimed + // once we leave the lock, because it sets _pooledCount such + // that it won't appear to be out of the pool. What that + // means, is that we're now responsible for this connection: + // it won't get reclaimed if it gets lost. + obj.PrePush(owningObject); + + // TODO: Consider using a Cer to ensure that we mark the object for reclaimation in the event something bad happens? + } + + if (!CheckConnection(obj)) + { + return; + } + + SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Deactivating.", Id, obj.ObjectID); + obj.DeactivateConnection(); + + bool returnToGeneralPool = false; + bool destroyObject = false; + + if (obj.IsConnectionDoomed || + !obj.CanBePooled || + State is ShuttingDown) + { + // the object is not fit for reuse -- just dispose of it. + destroyObject = true; + } + else + { + returnToGeneralPool = true; + } + + + if (returnToGeneralPool) + { + // Only push the connection into the general pool if we didn't + // already push it onto the transacted pool, put it into stasis, + // or want to destroy it. + Debug.Assert(destroyObject == false); + // Statement order is important since we have synchronous completions on the channel. + + var written = _idleConnectionWriter.TryWrite(obj); + Debug.Assert(written); + } + else if (destroyObject) + { + // Connections that have been marked as no longer + // poolable (e.g. exceeded their connection lifetime) are not, in fact, + // returned to the general pool + CloseConnection(obj); + } + + //------------------------------------------------------------------------------------- + // postcondition + + // ensure that the connection was processed + Debug.Assert(returnToGeneralPool == true || destroyObject == true); + } + + /// + /// Checks that the provided connector is live and unexpired and closes it if needed. + /// + /// + /// Returns true if the connector is live and unexpired, otherwise returns false. + private bool CheckConnection(DbConnectionInternal? connection) + { + // If Clear/ClearAll has been been called since this connector was first opened, + // throw it away. The same if it's broken (in which case CloseConnector is only + // used to update state/perf counter). + //TODO: check clear counter + + // An connector could be broken because of a keepalive that occurred while it was + // idling in the pool + // TODO: Consider removing the pool from the keepalive code. The following branch is simply irrelevant + // if keepalive isn't turned on. + if (connection == null) + { + return false; + } + + if (!connection.IsConnectionAlive()) + { + CloseConnection(connection); + return false; + } + + if (LoadBalanceTimeout != TimeSpan.Zero && DateTime.UtcNow > connection.CreateTime + LoadBalanceTimeout) + { + CloseConnection(connection); + return false; + } + + return true; + } + + /// + /// Closes the provided connector and adjust pool state accordingly. + /// + /// The connector to be closed. + private void CloseConnection(DbConnectionInternal connector) + { + try + { + connector.Dispose(); + } + catch + { + //TODO: log error + } + + // TODO: check clear counter so that we don't clear new connections + + int i; + for (i = 0; i < MaxPoolSize; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], null, connector) == connector) + { + break; + } + } + + // If CloseConnection is being called from within OpenNewConnection (e.g. an error happened during a connection initializer which + // causes the connector to Break, and therefore return the connector), then we haven't yet added the connector to Connections. + // In this case, there's no state to revert here (that's all taken care of in OpenNewConnection), skip it. + if (i == MaxPoolSize) + { + return; + } + + var numConnections = Interlocked.Decrement(ref _numConnections); + Debug.Assert(numConnections >= 0); + + // If a connection has been closed for any reason, we write a null to the idle connection channel to wake up + // a waiter, who will open a new physical connection + // Statement order is important since we have synchronous completions on the channel. + _idleConnectionWriter.TryWrite(null); + } + + private void PrepareConnection(DbConnection owningObject, DbConnectionInternal obj) + { + lock (obj) + { // Protect against Clear and ReclaimEmancipatedObjects, which call IsEmancipated, which is affected by PrePush and PostPop + obj.PostPop(owningObject); + } + try + { + //TODO: pass through transaction + obj.ActivateConnection(null); + } + catch + { + // if Activate throws an exception + // put it back in the pool or have it properly disposed of + this.ReturnInternalConnection(obj, owningObject); + throw; + } } /// @@ -239,7 +404,7 @@ private async Task GetInternalConnection(DbConnection owni if (connection != null) { // TODO: set connection internal state - // PrepareConnection(owningConnection, connection, transaction); + PrepareConnection(owningConnection, connection); return connection; } @@ -285,10 +450,10 @@ private async Task GetInternalConnection(DbConnection owni } // TODO: check if connection is still valid - if (connection != null) + if (connection != null && CheckConnection(connection)) { //TODO: set connection internal state - //PrepareConnection(owningConnection, connection, transaction); + PrepareConnection(owningConnection, connection); return connection; } } @@ -316,7 +481,7 @@ private async Task GetInternalConnection(DbConnection owni if (connection != null) { //TODO: set connection internal state - //PrepareConnection(owningConnection, connection, transaction); + PrepareConnection(owningConnection, connection); return connection; } } @@ -338,8 +503,10 @@ private async Task GetInternalConnection(DbConnection owni while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { // TODO: check if connection is still valid - // if (CheckIdleConnection(connection)) - return connection; + if (CheckConnection(connection)) + { + return connection; + } } return null; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 1b3512a3e7..5604a41e15 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -158,17 +158,17 @@ out extraConnection } [Fact] - [ActiveIssue("Requires ReturnInternalConnection to be implemented in ChannelDbConnectionPool")] public void ConnectionsAreReused() { // Arrange Setup(SuccessfulConnectionFactory); + SqlConnection owningConnection = new SqlConnection(); DbConnectionInternal internalConnection1 = null; DbConnectionInternal internalConnection2 = null; // Act: Get the first connection var completed1 = pool.TryGetConnection( - new SqlConnection(), + owningConnection, null, new DbConnectionOptions("", null), out internalConnection1 @@ -179,11 +179,11 @@ out internalConnection1 Assert.NotNull(internalConnection1); // Act: Return the first connection to the pool - pool.ReturnInternalConnection(internalConnection1, null); + pool.ReturnInternalConnection(internalConnection1, owningConnection); // Act: Get the second connection (should reuse the first one) var completed2 = pool.TryGetConnection( - new SqlConnection(), + owningConnection, null, new DbConnectionOptions("", null), out internalConnection2 @@ -221,78 +221,91 @@ out internalConnection [Fact] public void TestAuthenticationContexts() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => _ = pool.AuthenticationContexts); } [Fact] public void TestConnectionFactory() { + Setup(SuccessfulConnectionFactory); Assert.Equal(connectionFactory, pool.ConnectionFactory); } [Fact] public void TestCount() { + Setup(SuccessfulConnectionFactory); Assert.Equal(0, pool.Count); } [Fact] public void TestErrorOccurred() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => _ = pool.ErrorOccurred); } [Fact] public void TestId() { + Setup(SuccessfulConnectionFactory); Assert.True(pool.Id >= 1); } [Fact] public void TestIdentity() { + Setup(SuccessfulConnectionFactory); Assert.Equal(identity, pool.Identity); } [Fact] public void TestIsRunning() { + Setup(SuccessfulConnectionFactory); Assert.True(pool.IsRunning); } [Fact] public void TestLoadBalanceTimeout() { - Assert.Throws(() => _ = pool.LoadBalanceTimeout); + Setup(SuccessfulConnectionFactory); + Assert.Equal(poolGroupOptions.LoadBalanceTimeout, pool.LoadBalanceTimeout); } [Fact] public void TestPoolGroup() { + Setup(SuccessfulConnectionFactory); Assert.Equal(dbConnectionPoolGroup, pool.PoolGroup); } [Fact] public void TestPoolGroupOptions() { + Setup(SuccessfulConnectionFactory); Assert.Equal(poolGroupOptions, pool.PoolGroupOptions); } [Fact] public void TestProviderInfo() { + Setup(SuccessfulConnectionFactory); Assert.Equal(connectionPoolProviderInfo, pool.ProviderInfo); } [Fact] public void TestStateGetter() { + Setup(SuccessfulConnectionFactory); Assert.Equal(DbConnectionPoolState.Running, pool.State); } [Fact] public void TestStateSetter() { + Setup(SuccessfulConnectionFactory); pool.State = DbConnectionPoolState.ShuttingDown; Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); pool.State = DbConnectionPoolState.Running; @@ -302,7 +315,8 @@ public void TestStateSetter() [Fact] public void TestUseLoadBalancing() { - Assert.Throws(() => _ = pool.UseLoadBalancing); + Setup(SuccessfulConnectionFactory); + Assert.Equal(poolGroupOptions.UseLoadBalancing, pool.UseLoadBalancing); } #endregion @@ -312,42 +326,42 @@ public void TestUseLoadBalancing() [Fact] public void TestClear() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => pool.Clear()); } [Fact] public void TestPutObjectFromTransactedPool() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); } [Fact] public void TestReplaceConnection() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => pool.ReplaceConnection(null!, null!, null!)); } - [Fact] - public void TestReturnInternalConnection() - { - Assert.Throws(() => pool.ReturnInternalConnection(null!, null!)); - } - [Fact] public void TestShutdown() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => pool.Shutdown()); } [Fact] public void TestStartup() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => pool.Startup()); } [Fact] public void TestTransactionEnded() { + Setup(SuccessfulConnectionFactory); Assert.Throws(() => pool.TransactionEnded(null!, null!)); } #endregion @@ -357,7 +371,7 @@ internal class SuccessfulDbConnectionFactory : DbConnectionFactory { protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) { - return new SuccessfulDbConnectionInternal(); + return new StubDbConnectionInternal(); } #region Not Implemented Members @@ -477,7 +491,7 @@ internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnect #endregion } - internal class SuccessfulDbConnectionInternal : DbConnectionInternal + internal class StubDbConnectionInternal : DbConnectionInternal { #region Not Implemented Members public override string ServerVersion => throw new NotImplementedException(); @@ -489,17 +503,17 @@ public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) public override void EnlistTransaction(Transaction transaction) { - throw new NotImplementedException(); + return; } protected override void Activate(Transaction transaction) { - throw new NotImplementedException(); + return; } protected override void Deactivate() { - throw new NotImplementedException(); + return; } #endregion } From 7ac22f67717a4fb27e84d735e7a558ea73612e1f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 9 Jun 2025 15:19:07 -0700 Subject: [PATCH 05/28] Clean up comments and todos. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 144 ++++++++---------- 1 file changed, 63 insertions(+), 81 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 90c2fc08fa..06ac9801ff 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -184,8 +184,6 @@ public void ReturnInternalConnection(DbConnectionInternal obj, object owningObje // means, is that we're now responsible for this connection: // it won't get reclaimed if it gets lost. obj.PrePush(owningObject); - - // TODO: Consider using a Cer to ensure that we mark the object for reclaimation in the event something bad happens? } if (!CheckConnection(obj)) @@ -196,46 +194,17 @@ public void ReturnInternalConnection(DbConnectionInternal obj, object owningObje SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Deactivating.", Id, obj.ObjectID); obj.DeactivateConnection(); - bool returnToGeneralPool = false; - bool destroyObject = false; - if (obj.IsConnectionDoomed || !obj.CanBePooled || State is ShuttingDown) { - // the object is not fit for reuse -- just dispose of it. - destroyObject = true; + CloseConnection(obj); } else { - returnToGeneralPool = true; - } - - - if (returnToGeneralPool) - { - // Only push the connection into the general pool if we didn't - // already push it onto the transacted pool, put it into stasis, - // or want to destroy it. - Debug.Assert(destroyObject == false); - // Statement order is important since we have synchronous completions on the channel. - var written = _idleConnectionWriter.TryWrite(obj); Debug.Assert(written); } - else if (destroyObject) - { - // Connections that have been marked as no longer - // poolable (e.g. exceeded their connection lifetime) are not, in fact, - // returned to the general pool - CloseConnection(obj); - } - - //------------------------------------------------------------------------------------- - // postcondition - - // ensure that the connection was processed - Debug.Assert(returnToGeneralPool == true || destroyObject == true); } /// @@ -245,15 +214,6 @@ public void ReturnInternalConnection(DbConnectionInternal obj, object owningObje /// Returns true if the connector is live and unexpired, otherwise returns false. private bool CheckConnection(DbConnectionInternal? connection) { - // If Clear/ClearAll has been been called since this connector was first opened, - // throw it away. The same if it's broken (in which case CloseConnector is only - // used to update state/perf counter). - //TODO: check clear counter - - // An connector could be broken because of a keepalive that occurred while it was - // idling in the pool - // TODO: Consider removing the pool from the keepalive code. The following branch is simply irrelevant - // if keepalive isn't turned on. if (connection == null) { return false; @@ -277,24 +237,15 @@ private bool CheckConnection(DbConnectionInternal? connection) /// /// Closes the provided connector and adjust pool state accordingly. /// - /// The connector to be closed. - private void CloseConnection(DbConnectionInternal connector) + /// The connector to be closed. + private void CloseConnection(DbConnectionInternal connection) { - try - { - connector.Dispose(); - } - catch - { - //TODO: log error - } - - // TODO: check clear counter so that we don't clear new connections + connection.Dispose(); int i; for (i = 0; i < MaxPoolSize; i++) { - if (Interlocked.CompareExchange(ref _connections[i], null, connector) == connector) + if (Interlocked.CompareExchange(ref _connections[i], null, connection) == connection) { break; } @@ -317,12 +268,18 @@ private void CloseConnection(DbConnectionInternal connector) _idleConnectionWriter.TryWrite(null); } + /// + /// Sets connection state and activates the connection for use. Should always be called after a connection is created or retrieved from the pool. + /// + /// The owning DbConnection instance. + /// The DbConnectionInternal to be activated. private void PrepareConnection(DbConnection owningObject, DbConnectionInternal obj) { lock (obj) { // Protect against Clear and ReclaimEmancipatedObjects, which call IsEmancipated, which is affected by PrePush and PostPop obj.PostPop(owningObject); } + try { //TODO: pass through transaction @@ -358,16 +315,32 @@ public void TransactionEnded(Transaction transaction, DbConnectionInternal trans /// public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) { + // If taskCompletionSource is not null, we are in an async context. if (taskCompletionSource is not null) { - // This is ugly, but async anti-patterns further up the stack necessitate a fresh task to be created. - // Ideally we would just return a Task and let the caller await it as needed, - // but we need to signal to the provided TaskCompletionSource when the connection is established. - // This pattern has implications for connection open retry logic that are intricate enough to merit - // dedicated work. + /* This is ugly, but async anti-patterns above and below us in the stack necessitate a fresh task to be created. + * Ideally we would just return the Task from GetInternalConnection and let the caller await it as needed, + * but instead we need to signal to the provided TaskCompletionSource when the connection is established. + * This pattern has implications for connection open retry logic that are intricate enough to merit + * dedicated work. For now, callers that need to open many connections asynchronously and in parallel *must* + * pre-prevision threads in the managed thread pool to avoid exhaustion and timeouts. + * + * Also note that we don't have access to the cancellation token passed by the caller to the original OpenAsync call. + * This means that we cannot cancel the connection open operation if the caller's token is cancelled. We can only cancel + * based on our own timeout, which is set to the owningObject's ConnectionTimeout. Some + */ Task.Run(async () => { - //TODO: use same timespan everywhere and tick down for queueuing and actual connection opening work + if (taskCompletionSource.Task.IsCompleted) { + // If the task is already completed, we don't need to do anything. + return; + } + + // We're potentially on a new thread, so we need to properly set the ambient transaction. + // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState + // so that we can access it here. Read: area for improvement. + ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + try { var connection = await GetInternalConnection(owningObject, userOptions, TimeSpan.FromSeconds(owningObject.ConnectionTimeout), true, CancellationToken.None).ConfigureAwait(false); @@ -383,41 +356,46 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) { + // First pass + // + // Try to get an idle connection from the channel or open a new one. DbConnectionInternal? connection = null; - //TODO: check transacted pool - connection ??= GetIdleConnection(); connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); if (connection != null) { - // TODO: set connection internal state PrepareConnection(owningConnection, connection); return connection; } + // Second pass + // // We're at max capacity. Block on the idle channel with a timeout. // Note that Channels guarantee fair FIFO behavior to callers of ReadAsync (first-come first- // served), which is crucial to us. using CancellationTokenSource linkedSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); CancellationToken finalToken = linkedSource.Token; linkedSource.CancelAfter(timeout); - //TODO: respect remaining time, linkedSource.CancelAfter(timeout.CheckAndGetTimeLeft()); try { + // Continue looping around until we create/retrieve a connection or the timeout expires. while (true) { try @@ -428,12 +406,15 @@ private async Task GetInternalConnection(DbConnection owni } else { + // If there are no connections in the channel, then ReadAsync will block until one is available. + // Channels doesn't offer a sync API, so running ReadAsync synchronously on this thread may spawn + // additional new async work items in the managed thread pool if there are no items available in the + // channel. We need to ensure that we don't block all available managed threads with these child + // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput + // to the managed threadpool. SyncOverAsyncSemaphore.Wait(finalToken); try { - // If there are no connections in the channel, then this call will block until one is available. - // Because this call uses the managed thread pool, we need to limit the number of - // threads allowed to block here to avoid a deadlock. ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = _idleConnectionReader.ReadAsync(finalToken).ConfigureAwait(false).GetAwaiter(); using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); @@ -449,10 +430,8 @@ private async Task GetInternalConnection(DbConnection owni } } - // TODO: check if connection is still valid if (connection != null && CheckConnection(connection)) { - //TODO: set connection internal state PrepareConnection(owningConnection, connection); return connection; } @@ -470,9 +449,12 @@ private async Task GetInternalConnection(DbConnection owni throw new Exception("The connection pool has been shut down."); } + // Third pass + // + // Try again to get an idle connection or open a new one. // If we're here, our waiting attempt on the idle connection channel was released with a null // (or bad connection), or we're in sync mode. Check again if a new idle connection has appeared since we last checked. - connection = GetIdleConnection(); + connection ??= GetIdleConnection(); // We might have closed a connection in the meantime and no longer be at max capacity // so try to open a new connection and if that fails, loop again. @@ -480,7 +462,6 @@ private async Task GetInternalConnection(DbConnection owni if (connection != null) { - //TODO: set connection internal state PrepareConnection(owningConnection, connection); return connection; } @@ -496,13 +477,12 @@ private async Task GetInternalConnection(DbConnection owni /// Tries to read a connection from the idle connection channel. /// /// Returns true if a valid idles connection is found, otherwise returns false. - /// TODO: profile the inlining to see if it's necessary + /// TODO: profile the inlining to see if it's necessary (comment copied from npgsql implementation) [MethodImpl(MethodImplOptions.AggressiveInlining)] private DbConnectionInternal? GetIdleConnection() { while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { - // TODO: check if connection is still valid if (CheckConnection(connection)) { return connection; @@ -529,11 +509,13 @@ private async Task GetInternalConnection(DbConnection owni // We've managed to increase the open counter, open a physical connection. var startTime = Stopwatch.GetTimestamp(); - // TODO: This blocks the thread for several network calls! - // This will be unexpected to async callers. - // Our options are limited because DbConnectionInternal doesn't support async open. - // It's better to block this thread and keep throughput high than to queue all of our opens onto a single worker thread. - // Add an async path when this support is added to DbConnectionInternal. + /* TODO: This blocks the thread for several network calls! + * When running async, the blocked thread is one allocated from the managed thread pool (due to use of Task.Run above). + * This is why it's critical for async callers to pre-provision threads in the managed thread pool. + * Our options are limited because DbConnectionInternal doesn't support an async open. + * It's better to block this thread and keep throughput high than to queue all of our opens onto a single worker thread. + * Add an async path when this support is added to DbConnectionInternal. + */ DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( this, owningConnection, From 7c759308595996f135f56037767dc4c0e9d0bdf1 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 9 Jun 2025 16:18:03 -0700 Subject: [PATCH 06/28] Add test cases. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 7 - .../ChannelDbConnectionPoolTest.cs | 360 +++++++++++++++++- 2 files changed, 356 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 06ac9801ff..9930307458 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -169,13 +169,6 @@ public DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConne /// public void ReturnInternalConnection(DbConnectionInternal obj, object owningObject) { - // Once a connection is closing (which is the state that we're in at - // this point in time) you cannot delegate a transaction to or enlist - // a transaction in it, so we can correctly presume that if there was - // not a delegated or enlisted transaction to start with, that there - // will not be a delegated or enlisted transaction once we leave the - // lock. - lock (obj) { // Calling PrePush prevents the object from being reclaimed diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 5604a41e15..7f69b3ae6d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Concurrent; +using System.Collections.Specialized; using System.Data.Common; using System.Threading; using System.Threading.Tasks; @@ -57,7 +59,7 @@ private void Setup(DbConnectionFactory connectionFactory) [InlineData(1)] [InlineData(5)] [InlineData(10)] - public void GetConnectionFromEmptyPoolSync_ShouldCreateNewConnection(int numConnections) + public void GetConnectionEmptyPool_ShouldCreateNewConnection(int numConnections) { // Arrange Setup(SuccessfulConnectionFactory); @@ -87,7 +89,7 @@ out internalConnection [InlineData(1)] [InlineData(5)] [InlineData(10)] - public async Task GetConnectionFromEmptyPoolAsync_ShouldCreateNewConnection(int numConnections) + public async Task GetConnectionAsyncEmptyPool_ShouldCreateNewConnection(int numConnections) { // Arrange Setup(SuccessfulConnectionFactory); @@ -116,7 +118,7 @@ out internalConnection } [Fact] - public void GetConnectionRespectsMaxPoolSize() + public void GetConnectionMaxPoolSize_ShouldTimeoutAfterPeriod() { // Arrange Setup(SuccessfulConnectionFactory); @@ -157,6 +159,263 @@ out extraConnection Assert.Equal(poolGroupOptions.MaxPoolSize, pool.Count); } + [Fact] + public async Task GetConnectionAsyncMaxPoolSize_ShouldTimeoutAfterPeriod() + { + // Arrange + Setup(SuccessfulConnectionFactory); + + for (int i = 0; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + + try + { + // Act + TaskCompletionSource taskCompletionSource = new TaskCompletionSource(); + DbConnectionInternal extraConnection = null; + var exceeded = pool.TryGetConnection( + new SqlConnection("Timeout=1"), + taskCompletionSource, + new DbConnectionOptions("", null), + out extraConnection + ); + await taskCompletionSource.Task; + } + catch (Exception ex) + { + // Assert + Assert.IsType(ex); + Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); + } + + // Assert + Assert.Equal(poolGroupOptions.MaxPoolSize, pool.Count); + } + + [Fact] + public async Task GetConnectionMaxPoolSize_ShouldReuseAfterConnectionReleased() + { + // Arrange + Setup(SuccessfulConnectionFactory); + DbConnectionInternal firstConnection = null; + SqlConnection firstOwningConnection = new SqlConnection(); + + pool.TryGetConnection( + firstOwningConnection, + taskCompletionSource: null, + new DbConnectionOptions("", null), + out firstConnection + ); + + for (int i = 1; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + + TaskCompletionSource tcs = new TaskCompletionSource(); + + // Act + var task = Task.Run(() => + { + DbConnectionInternal extraConnection = null; + var exceeded = pool.TryGetConnection( + new SqlConnection(""), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out extraConnection + ); + return extraConnection; + }); + pool.ReturnInternalConnection(firstConnection, firstOwningConnection); + var extraConnection = await task; + + // Assert + Assert.Equal(firstConnection, extraConnection); + } + + [Fact] + public async Task GetConnectionAsyncMaxPoolSize_ShouldReuseAfterConnectionReleased() + { + // Arrange + Setup(SuccessfulConnectionFactory); + DbConnectionInternal firstConnection = null; + SqlConnection firstOwningConnection = new SqlConnection(); + + pool.TryGetConnection( + firstOwningConnection, + taskCompletionSource: null, + new DbConnectionOptions("", null), + out firstConnection + ); + + for (int i = 1; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + + TaskCompletionSource taskCompletionSource = new TaskCompletionSource(); + + // Act + DbConnectionInternal recycledConnection = null; + var exceeded = pool.TryGetConnection( + new SqlConnection(""), + taskCompletionSource, + new DbConnectionOptions("", null), + out recycledConnection + ); + pool.ReturnInternalConnection(firstConnection, firstOwningConnection); + recycledConnection = await taskCompletionSource.Task; + + // Assert + Assert.Equal(firstConnection, recycledConnection); + } + + [Fact] + public async Task GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest() + { + // Arrange + Setup(SuccessfulConnectionFactory); + DbConnectionInternal firstConnection = null; + SqlConnection firstOwningConnection = new SqlConnection(); + + pool.TryGetConnection( + firstOwningConnection, + taskCompletionSource: null, + new DbConnectionOptions("", null), + out firstConnection + ); + + for (int i = 1; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + + // Act + var recycledTask = Task.Run(() => + { + DbConnectionInternal recycledConnection = null; + var exceeded = pool.TryGetConnection( + new SqlConnection(""), + null, + new DbConnectionOptions("", null), + out recycledConnection + ); + return recycledConnection; + }); + var failedTask = Task.Run(() => + { + DbConnectionInternal failedConnection = null; + var exceeded2 = pool.TryGetConnection( + new SqlConnection("Timeout=1"), + null, + new DbConnectionOptions("", null), + out failedConnection + ); + return failedConnection; + }); + + pool.ReturnInternalConnection(firstConnection, firstOwningConnection); + var recycledConnection = await recycledTask; + + // Assert + Assert.Equal(firstConnection, recycledConnection); + await Assert.ThrowsAsync(async () => await failedTask); + } + + [Fact] + public async Task GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest() + { + // Arrange + Setup(SuccessfulConnectionFactory); + DbConnectionInternal firstConnection = null; + SqlConnection firstOwningConnection = new SqlConnection(); + + pool.TryGetConnection( + firstOwningConnection, + taskCompletionSource: null, + new DbConnectionOptions("", null), + out firstConnection + ); + + for (int i = 1; i < poolGroupOptions.MaxPoolSize; i++) + { + DbConnectionInternal internalConnection = null; + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + + Assert.True(completed); + Assert.NotNull(internalConnection); + } + + TaskCompletionSource recycledTaskCompletionSource = new TaskCompletionSource(); + TaskCompletionSource failedCompletionSource = new TaskCompletionSource(); + + // Act + DbConnectionInternal recycledConnection = null; + var exceeded = pool.TryGetConnection( + new SqlConnection(""), + recycledTaskCompletionSource, + new DbConnectionOptions("", null), + out recycledConnection + ); + DbConnectionInternal failedConnection = null; + var exceeded2 = pool.TryGetConnection( + new SqlConnection("Timeout=1"), + failedCompletionSource, + new DbConnectionOptions("", null), + out failedConnection + ); + + pool.ReturnInternalConnection(firstConnection, firstOwningConnection); + recycledConnection = await recycledTaskCompletionSource.Task; + + // Assert + Assert.Equal(firstConnection, recycledConnection); + await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); + } + [Fact] public void ConnectionsAreReused() { @@ -196,7 +455,7 @@ out internalConnection2 } [Fact] - public void GetConnectionWithTimeout_ShouldThrowTimeoutException() + public void GetConnectionTimeout_ShouldThrowTimeoutException() { // Arrange Setup(TimeoutConnectionFactory); @@ -216,6 +475,99 @@ out internalConnection Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); } + [Fact] + public async Task GetConnectionAsyncTimeout_ShouldThrowTimeoutException() + { + // Arrange + Setup(TimeoutConnectionFactory); + DbConnectionInternal internalConnection = null; + TaskCompletionSource taskCompletionSource = new TaskCompletionSource(); + + // Act & Assert + var ex = await Assert.ThrowsAsync(async () => + { + var completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource, + new DbConnectionOptions("", null), + out internalConnection + ); + + await taskCompletionSource.Task; + }); + + Assert.Equal("Timeout expired. The timeout period elapsed prior to obtaining a connection from the pool. This may have occurred because all pooled connections were in use and max pool size was reached.", ex.Message); + } + + [Fact] + public void StressTest() + { + //Arrange + Setup(SuccessfulConnectionFactory); + ConcurrentBag tasks = new ConcurrentBag(); + + + for (int i = 1; i < poolGroupOptions.MaxPoolSize * 3; i++) + { + var t = Task.Run(() => + { + DbConnectionInternal internalConnection = null; + SqlConnection owningObject = new SqlConnection(); + var completed = pool.TryGetConnection( + owningObject, + taskCompletionSource: null, + new DbConnectionOptions("", null), + out internalConnection + ); + if (completed) + { + pool.ReturnInternalConnection(internalConnection, owningObject); + } + + Assert.True(completed); + Assert.NotNull(internalConnection); + }); + tasks.Add(t); + } + + Task.WaitAll(tasks.ToArray()); + Assert.True(pool.Count <= poolGroupOptions.MaxPoolSize, "Pool size exceeded max pool size after stress test."); + } + + [Fact] + public void StressTestAsync() + { + //Arrange + Setup(SuccessfulConnectionFactory); + ConcurrentBag tasks = new ConcurrentBag(); + + + for (int i = 1; i < poolGroupOptions.MaxPoolSize * 3; i++) + { + var t = Task.Run(async () => + { + DbConnectionInternal internalConnection = null; + SqlConnection owningObject = new SqlConnection(); + TaskCompletionSource taskCompletionSource = new TaskCompletionSource(); + var completed = pool.TryGetConnection( + owningObject, + taskCompletionSource, + new DbConnectionOptions("", null), + out internalConnection + ); + internalConnection = await taskCompletionSource.Task; + pool.ReturnInternalConnection(internalConnection, owningObject); + + Assert.NotNull(internalConnection); + }); + tasks.Add(t); + } + + Task.WaitAll(tasks.ToArray()); + Assert.True(pool.Count <= poolGroupOptions.MaxPoolSize, "Pool size exceeded max pool size after stress test."); + } + + #region Property Tests [Fact] From 6dde5b984418a0e919134e4b13b72ef192d647a7 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 12 Jun 2025 10:40:53 -0700 Subject: [PATCH 07/28] Enable init property accessor in netfx. --- .../netfx/src/Microsoft.Data.SqlClient.csproj | 3 +++ .../CompilerServices/IsExternalInit.netfx.cs | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj index b00e79ab53..6d172d90f5 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj @@ -948,6 +948,9 @@ System\IO\StreamExtensions.netfx.cs + + System\Runtime\CompilerServices\IsExternalInit.netfx.cs + diff --git a/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs b/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs new file mode 100644 index 0000000000..0d0181ba6d --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs @@ -0,0 +1,23 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +#if NETFRAMEWORK + +using System.ComponentModel; + + +// This class enables the use of the `init` property accessor in .NET framework. +namespace System.Runtime.CompilerServices +{ + /// + /// Reserved to be used by the compiler for tracking metadata. + /// This class should not be used by developers in source code. + /// + [EditorBrowsable(EditorBrowsableState.Never)] + internal static class IsExternalInit + { + } +} + +#endif From 9999bf7e7fe08c2c17147b663c5c12bb33679996 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 12 Jun 2025 10:41:21 -0700 Subject: [PATCH 08/28] Switch to auto properties. --- .../Data/ProviderBase/DbConnectionInternal.cs | 18 ++--- .../ConnectionPool/ChannelDbConnectionPool.cs | 67 +++++++++++-------- .../ConnectionPool/IDbConnectionPool.cs | 2 +- 3 files changed, 51 insertions(+), 36 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index 3f805b94b2..39f65727c9 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -46,11 +46,6 @@ internal abstract class DbConnectionInternal /// private bool _cannotBePooled; - /// - /// When the connection was created. - /// - private DateTime _createTime; - /// /// [usage must be thread-safe] the transaction that we're enlisted in, either manually or automatically. /// @@ -97,7 +92,14 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all #region Properties - internal DateTime CreateTime => _createTime; + /// + /// When the connection was created. + /// + internal DateTime CreateTime + { + get; + private set; + } internal bool AllowSetConnectionString { get; } @@ -533,7 +535,7 @@ internal void DeactivateConnection() // If we're not already doomed, check the connection's lifetime and // doom it if it's lifetime has elapsed. DateTime now = DateTime.UtcNow; - if (now.Ticks - _createTime.Ticks > Pool.LoadBalanceTimeout.Ticks) + if (now.Ticks - CreateTime.Ticks > Pool.LoadBalanceTimeout.Ticks) { DoNotPoolThisConnection(); } @@ -703,7 +705,7 @@ internal void MakeNonPooledObject(DbConnection owningObject) /// internal void MakePooledConnection(IDbConnectionPool connectionPool) { - _createTime = DateTime.UtcNow; + CreateTime = DateTime.UtcNow; Pool = connectionPool; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9930307458..2249c1c400 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -27,12 +27,21 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool { #region Properties /// - public ConcurrentDictionary AuthenticationContexts => throw new NotImplementedException(); + public ConcurrentDictionary AuthenticationContexts + { + get; + init; + } /// - public DbConnectionFactory ConnectionFactory => _connectionFactory; + public DbConnectionFactory ConnectionFactory + { + get; + init; + } /// + /// Note: maintain _numConnections backing field to enable atomic operations public int Count => _numConnections; /// @@ -42,7 +51,11 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool public int Id => _objectID; /// - public DbConnectionPoolIdentity Identity => _identity; + public DbConnectionPoolIdentity Identity + { + get; + private set; + } /// public bool IsRunning => State is Running; @@ -51,19 +64,31 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout; /// - public DbConnectionPoolGroup PoolGroup => _connectionPoolGroup; + public DbConnectionPoolGroup PoolGroup + { + get; + init; + } /// - public DbConnectionPoolGroupOptions PoolGroupOptions => _connectionPoolGroupOptions; + public DbConnectionPoolGroupOptions PoolGroupOptions + { + get; + init; + } /// - public DbConnectionPoolProviderInfo ProviderInfo => _connectionPoolProviderInfo; + public DbConnectionPoolProviderInfo ProviderInfo + { + get; + init; + } /// public DbConnectionPoolState State { get; - set; + private set; } /// @@ -73,18 +98,6 @@ public DbConnectionPoolState State #endregion #region Fields - private readonly DbConnectionPoolIdentity _identity; - - private readonly DbConnectionFactory _connectionFactory; - private readonly DbConnectionPoolGroup _connectionPoolGroup; - private readonly DbConnectionPoolGroupOptions _connectionPoolGroupOptions; - private DbConnectionPoolProviderInfo _connectionPoolProviderInfo; - - /// - /// The private member which carries the set of authenticationcontexts for this pool (based on the user's identity). - /// - private readonly ConcurrentDictionary _pooledDbAuthenticationContexts; - // Prevents synchronous operations which depend on async operations on managed // threads from blocking on all available threads, which would stop async tasks // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance @@ -124,12 +137,12 @@ internal ChannelDbConnectionPool( { State = Initializing; - _connectionFactory = connectionFactory; - _connectionPoolGroup = connectionPoolGroup; - _connectionPoolGroupOptions = connectionPoolGroup.PoolGroupOptions; - _connectionPoolProviderInfo = connectionPoolProviderInfo; - _identity = identity; - _pooledDbAuthenticationContexts = new ConcurrentDictionary< + ConnectionFactory = connectionFactory; + PoolGroup = connectionPoolGroup; + PoolGroupOptions = connectionPoolGroup.PoolGroupOptions; + ProviderInfo = connectionPoolProviderInfo; + Identity = identity; + AuthenticationContexts = new ConcurrentDictionary< DbConnectionPoolAuthenticationContextKey, DbConnectionPoolAuthenticationContext>( concurrencyLevel: 4 * Environment.ProcessorCount, @@ -512,8 +525,8 @@ private async Task GetInternalConnection(DbConnection owni DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( this, owningConnection, - _connectionPoolGroup.ConnectionOptions, - _connectionPoolGroup.PoolKey, + PoolGroup.ConnectionOptions, + PoolGroup.PoolKey, userOptions); if (newConnection == null) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs index d6647f832e..fba0a2af27 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs @@ -81,7 +81,7 @@ internal interface IDbConnectionPool /// /// The current state of the connection pool. /// - DbConnectionPoolState State { get; set; } + DbConnectionPoolState State { get; } /// /// Indicates whether the connection pool is using load balancing. From 931ac03d2bcc1d83e129e161f4d9be565ffa06be Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 12 Jun 2025 10:47:00 -0700 Subject: [PATCH 09/28] Reorder class members. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 129 +++++++++--------- 1 file changed, 64 insertions(+), 65 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 2249c1c400..ed4bf591b1 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -25,6 +25,68 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// internal sealed class ChannelDbConnectionPool : IDbConnectionPool { + #region Fields + // Prevents synchronous operations which depend on async operations on managed + // threads from blocking on all available threads, which would stop async tasks + // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance + // between sync and async tasks. + private static SemaphoreSlim _syncOverAsyncSemaphore = new(Math.Max(1, Environment.ProcessorCount / 2)); + + private static int _objectTypeCount; // EventSource counter + + private readonly int _objectID = Interlocked.Increment(ref _objectTypeCount); + + /// + /// Tracks all connections currently managed by this pool, whether idle or busy. + /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts. + /// + private readonly DbConnectionInternal?[] _connections; + + /// + /// Reader side for the idle connection channel. Contains nulls in order to release waiting attempts after + /// a connection has been physically closed/broken. + /// + private readonly ChannelReader _idleConnectionReader; + private readonly ChannelWriter _idleConnectionWriter; + + // Counts the total number of open connections tracked by the pool. + private volatile int _numConnections; + #endregion + + /// + /// Initializes a new PoolingDataSource. + /// + internal ChannelDbConnectionPool( + DbConnectionFactory connectionFactory, + DbConnectionPoolGroup connectionPoolGroup, + DbConnectionPoolIdentity identity, + DbConnectionPoolProviderInfo connectionPoolProviderInfo) + { + State = Initializing; + + ConnectionFactory = connectionFactory; + PoolGroup = connectionPoolGroup; + PoolGroupOptions = connectionPoolGroup.PoolGroupOptions; + ProviderInfo = connectionPoolProviderInfo; + Identity = identity; + AuthenticationContexts = new ConcurrentDictionary< + DbConnectionPoolAuthenticationContextKey, + DbConnectionPoolAuthenticationContext>( + concurrencyLevel: 4 * Environment.ProcessorCount, + capacity: 2); + + _connections = new DbConnectionInternal[MaxPoolSize]; + + // We enforce Max Pool Size, so no need to create a bounded channel (which is less efficient) + // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents + // On the producing side, we have connections being released back into the pool (both multiplexing and not) + var idleChannel = Channel.CreateUnbounded(); + _idleConnectionReader = idleChannel.Reader; + _idleConnectionWriter = idleChannel.Writer; + + State = Running; + } + #region Properties /// public ConcurrentDictionary AuthenticationContexts @@ -97,69 +159,6 @@ public DbConnectionPoolState State private int MaxPoolSize => PoolGroupOptions.MaxPoolSize; #endregion - #region Fields - // Prevents synchronous operations which depend on async operations on managed - // threads from blocking on all available threads, which would stop async tasks - // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance - // between sync and async tasks. - private static SemaphoreSlim SyncOverAsyncSemaphore { get; } = new(Math.Max(1, Environment.ProcessorCount / 2)); - - private static int _objectTypeCount; // EventSource counter - - private readonly int _objectID = Interlocked.Increment(ref _objectTypeCount); - - /// - /// Tracks all connections currently managed by this pool, whether idle or busy. - /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts. - /// - private readonly DbConnectionInternal?[] _connections; - - /// - /// Reader side for the idle connection channel. Contains nulls in order to release waiting attempts after - /// a connection has been physically closed/broken. - /// - private readonly ChannelReader _idleConnectionReader; - private readonly ChannelWriter _idleConnectionWriter; - - // Counts the total number of open connections tracked by the pool. - private volatile int _numConnections; - #endregion - - - /// - /// Initializes a new PoolingDataSource. - /// - internal ChannelDbConnectionPool( - DbConnectionFactory connectionFactory, - DbConnectionPoolGroup connectionPoolGroup, - DbConnectionPoolIdentity identity, - DbConnectionPoolProviderInfo connectionPoolProviderInfo) - { - State = Initializing; - - ConnectionFactory = connectionFactory; - PoolGroup = connectionPoolGroup; - PoolGroupOptions = connectionPoolGroup.PoolGroupOptions; - ProviderInfo = connectionPoolProviderInfo; - Identity = identity; - AuthenticationContexts = new ConcurrentDictionary< - DbConnectionPoolAuthenticationContextKey, - DbConnectionPoolAuthenticationContext>( - concurrencyLevel: 4 * Environment.ProcessorCount, - capacity: 2); - - _connections = new DbConnectionInternal[MaxPoolSize]; - - // We enforce Max Pool Size, so no need to create a bounded channel (which is less efficient) - // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents - // On the producing side, we have connections being released back into the pool (both multiplexing and not) - var idleChannel = Channel.CreateUnbounded(); - _idleConnectionReader = idleChannel.Reader; - _idleConnectionWriter = idleChannel.Writer; - - State = Running; - } - #region Methods /// public void Clear() @@ -418,7 +417,7 @@ private async Task GetInternalConnection(DbConnection owni // channel. We need to ensure that we don't block all available managed threads with these child // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput // to the managed threadpool. - SyncOverAsyncSemaphore.Wait(finalToken); + _syncOverAsyncSemaphore.Wait(finalToken); try { ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = @@ -432,7 +431,7 @@ private async Task GetInternalConnection(DbConnection owni } finally { - SyncOverAsyncSemaphore.Release(); + _syncOverAsyncSemaphore.Release(); } } From 7d734f9919704a3068c2b04e589378c3bb7ce0da Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 12 Jun 2025 11:29:31 -0700 Subject: [PATCH 10/28] Address review comments. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 393 +++++++++--------- 1 file changed, 207 insertions(+), 186 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ed4bf591b1..956368aeab 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -208,10 +208,183 @@ public void ReturnInternalConnection(DbConnectionInternal obj, object owningObje else { var written = _idleConnectionWriter.TryWrite(obj); - Debug.Assert(written); + Debug.Assert(written, "Failed to write returning connection to the idle channel."); } } + /// + public void Shutdown() + { + throw new NotImplementedException(); + } + + /// + public void Startup() + { + throw new NotImplementedException(); + } + + /// + public void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject) + { + throw new NotImplementedException(); + } + + /// + public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) + { + // If taskCompletionSource is not null, we are in an async context. + if (taskCompletionSource is not null) + { + + // Early exit if the task is already completed. + if (taskCompletionSource.Task.IsCompleted) + { + connection = null; + return false; + } + + /* + * This is ugly, but async anti-patterns above and below us in the stack necessitate a fresh task to be + * created. Ideally we would just return the Task from GetInternalConnection and let the caller await + * it as needed, but instead we need to signal to the provided TaskCompletionSource when the connection + * is established. This pattern has implications for connection open retry logic that are intricate + * enough to merit dedicated work. For now, callers that need to open many connections asynchronously + * and in parallel *must* pre-prevision threads in the managed thread pool to avoid exhaustion and + * timeouts. + * + * Also note that we don't have access to the cancellation token passed by the caller to the original + * OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token + * is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's + * ConnectionTimeout. + */ + Task.Run(async () => + { + if (taskCompletionSource.Task.IsCompleted) + { + return; + } + + // We're potentially on a new thread, so we need to properly set the ambient transaction. + // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState + // so that we can access it here. Read: area for improvement. + ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + + try + { + var connection = await GetInternalConnection( + owningObject, + userOptions, + TimeSpan.FromSeconds(owningObject.ConnectionTimeout), + async: true, + CancellationToken.None + ).ConfigureAwait(false); + taskCompletionSource.SetResult(connection); + } + catch (Exception e) + { + taskCompletionSource.SetException(e); + } + }); + + connection = null; + return false; + } + else + { + var task = GetInternalConnection( + owningObject, + userOptions, + TimeSpan.FromSeconds(owningObject.ConnectionTimeout), + async: false, + CancellationToken.None); + + // When running synchronously, we are guaranteed that the task is already completed. + // We don't need to guard the managed threadpool at this spot because we pass the async flag as false + // to GetInternalConnection, which means it will not use Task.Run or any async-await logic that would + // schedule tasks on the managed threadpool. + connection = task.ConfigureAwait(false).GetAwaiter().GetResult(); + return connection is not null; + } + } + + /// + internal Task OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) + { + // As long as we're under max capacity, attempt to increase the connection count and open a new connection. + for (var numConnections = _numConnections; numConnections < MaxPoolSize; numConnections = _numConnections) + { + // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 + if (Interlocked.CompareExchange(ref _numConnections, numConnections + 1, numConnections) != numConnections) + { + continue; + } + + try + { + // We've managed to increase the open counter, open a physical connection. + var startTime = Stopwatch.GetTimestamp(); + + /* TODO: This blocks the thread for several network calls! + * When running async, the blocked thread is one allocated from the managed thread pool (due to use of Task.Run above). + * This is why it's critical for async callers to pre-provision threads in the managed thread pool. + * Our options are limited because DbConnectionInternal doesn't support an async open. + * It's better to block this thread and keep throughput high than to queue all of our opens onto a single worker thread. + * Add an async path when this support is added to DbConnectionInternal. + */ + DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( + this, + owningConnection, + PoolGroup.ConnectionOptions, + PoolGroup.PoolKey, + userOptions); + + if (newConnection == null) + { + throw ADP.InternalError(ADP.InternalErrorCode.CreateObjectReturnedNull); + } + if (!newConnection.CanBePooled) + { + throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); + } + + newConnection.PrePush(null); + + int i; + for (i = 0; i < MaxPoolSize; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], newConnection, null) == null) + { + break; + } + } + + Debug.Assert(i < MaxPoolSize, $"Could not find free slot in {_connections} when opening."); + if (i == MaxPoolSize) + { + //TODO: generic exception? + throw new Exception($"Could not find free slot in {_connections} when opening. Please report a bug."); + } + + return Task.FromResult(newConnection); + } + catch + { + // Physical open failed, decrement the open and busy counter back down. + Interlocked.Decrement(ref _numConnections); + + // In case there's a waiting attempt on the channel, we write a null to the idle connection channel + // to wake it up, so it will try opening (and probably throw immediately) + // Statement order is important since we have synchronous completions on the channel. + _idleConnectionWriter.TryWrite(null); + + throw; + } + } + + return Task.FromResult(null); + } + /// /// Checks that the provided connector is live and unexpired and closes it if needed. /// @@ -274,102 +447,20 @@ private void CloseConnection(DbConnectionInternal connection) } /// - /// Sets connection state and activates the connection for use. Should always be called after a connection is created or retrieved from the pool. + /// Tries to read a connection from the idle connection channel. /// - /// The owning DbConnection instance. - /// The DbConnectionInternal to be activated. - private void PrepareConnection(DbConnection owningObject, DbConnectionInternal obj) - { - lock (obj) - { // Protect against Clear and ReclaimEmancipatedObjects, which call IsEmancipated, which is affected by PrePush and PostPop - obj.PostPop(owningObject); - } - - try - { - //TODO: pass through transaction - obj.ActivateConnection(null); - } - catch - { - // if Activate throws an exception - // put it back in the pool or have it properly disposed of - this.ReturnInternalConnection(obj, owningObject); - throw; - } - } - - /// - public void Shutdown() - { - throw new NotImplementedException(); - } - - /// - public void Startup() - { - throw new NotImplementedException(); - } - - /// - public void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject) - { - throw new NotImplementedException(); - } - - /// - public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) + /// Returns true if a valid idles connection is found, otherwise returns false. + private DbConnectionInternal? GetIdleConnection() { - // If taskCompletionSource is not null, we are in an async context. - if (taskCompletionSource is not null) + while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { - /* This is ugly, but async anti-patterns above and below us in the stack necessitate a fresh task to be created. - * Ideally we would just return the Task from GetInternalConnection and let the caller await it as needed, - * but instead we need to signal to the provided TaskCompletionSource when the connection is established. - * This pattern has implications for connection open retry logic that are intricate enough to merit - * dedicated work. For now, callers that need to open many connections asynchronously and in parallel *must* - * pre-prevision threads in the managed thread pool to avoid exhaustion and timeouts. - * - * Also note that we don't have access to the cancellation token passed by the caller to the original OpenAsync call. - * This means that we cannot cancel the connection open operation if the caller's token is cancelled. We can only cancel - * based on our own timeout, which is set to the owningObject's ConnectionTimeout. Some - */ - Task.Run(async () => + if (CheckConnection(connection)) { - if (taskCompletionSource.Task.IsCompleted) { - // If the task is already completed, we don't need to do anything. - return; - } - - // We're potentially on a new thread, so we need to properly set the ambient transaction. - // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState - // so that we can access it here. Read: area for improvement. - ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); - - try - { - var connection = await GetInternalConnection(owningObject, userOptions, TimeSpan.FromSeconds(owningObject.ConnectionTimeout), true, CancellationToken.None).ConfigureAwait(false); - taskCompletionSource.SetResult(connection); - } - catch (Exception e) - { - taskCompletionSource.SetException(e); - } - }); - connection = null; - return false; + return connection; + } } - else - { - var task = GetInternalConnection(owningObject, userOptions, TimeSpan.FromSeconds(owningObject.ConnectionTimeout), false, CancellationToken.None); - // When running synchronously, we are guaranteed that the task is already completed. - // We don't need to guard the managed threadpool at this spot because we pass the async flag as false - // to GetInternalConnection, which means it will not use Task.Run or any async-await logic that would - // schedule tasks on the managed threadpool. - connection = task.ConfigureAwait(false).GetAwaiter().GetResult(); - return connection is not null; - } + return null; } private async Task GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) @@ -377,13 +468,11 @@ private async Task GetInternalConnection(DbConnection owni // First pass // // Try to get an idle connection from the channel or open a new one. - DbConnectionInternal? connection = null; - - connection ??= GetIdleConnection(); + DbConnectionInternal? connection = GetIdleConnection(); connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); - if (connection != null) + if (connection is not null) { PrepareConnection(owningConnection, connection); return connection; @@ -401,7 +490,7 @@ private async Task GetInternalConnection(DbConnection owni try { // Continue looping around until we create/retrieve a connection or the timeout expires. - while (true) + while (true && !finalToken.IsCancellationRequested) { try { @@ -435,7 +524,7 @@ private async Task GetInternalConnection(DbConnection owni } } - if (connection != null && CheckConnection(connection)) + if (connection is not null && CheckConnection(connection)) { PrepareConnection(owningConnection, connection); return connection; @@ -465,113 +554,45 @@ private async Task GetInternalConnection(DbConnection owni // so try to open a new connection and if that fails, loop again. connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); - if (connection != null) + if (connection is not null) { PrepareConnection(owningConnection, connection); return connection; } } + + throw ADP.PooledOpenTimeout(); } finally { - //TODO: log error + //TODO: metrics } } /// - /// Tries to read a connection from the idle connection channel. + /// Sets connection state and activates the connection for use. Should always be called after a connection is created or retrieved from the pool. /// - /// Returns true if a valid idles connection is found, otherwise returns false. - /// TODO: profile the inlining to see if it's necessary (comment copied from npgsql implementation) - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private DbConnectionInternal? GetIdleConnection() + /// The owning DbConnection instance. + /// The DbConnectionInternal to be activated. + private void PrepareConnection(DbConnection owningObject, DbConnectionInternal obj) { - while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) - { - if (CheckConnection(connection)) - { - return connection; - } + lock (obj) + { // Protect against Clear and ReclaimEmancipatedObjects, which call IsEmancipated, which is affected by PrePush and PostPop + obj.PostPop(owningObject); } - return null; - } - - /// - internal Task OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) - { - // As long as we're under max capacity, attempt to increase the connection count and open a new connection. - for (var numConnections = _numConnections; numConnections < MaxPoolSize; numConnections = _numConnections) + try { - // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 - if (Interlocked.CompareExchange(ref _numConnections, numConnections + 1, numConnections) != numConnections) - { - continue; - } - - try - { - // We've managed to increase the open counter, open a physical connection. - var startTime = Stopwatch.GetTimestamp(); - - /* TODO: This blocks the thread for several network calls! - * When running async, the blocked thread is one allocated from the managed thread pool (due to use of Task.Run above). - * This is why it's critical for async callers to pre-provision threads in the managed thread pool. - * Our options are limited because DbConnectionInternal doesn't support an async open. - * It's better to block this thread and keep throughput high than to queue all of our opens onto a single worker thread. - * Add an async path when this support is added to DbConnectionInternal. - */ - DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( - this, - owningConnection, - PoolGroup.ConnectionOptions, - PoolGroup.PoolKey, - userOptions); - - if (newConnection == null) - { - throw ADP.InternalError(ADP.InternalErrorCode.CreateObjectReturnedNull); // CreateObject succeeded, but null object - } - if (!newConnection.CanBePooled) - { - throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); // CreateObject succeeded, but non-poolable object - } - - newConnection.PrePush(null); - - int i; - for (i = 0; i < MaxPoolSize; i++) - { - if (Interlocked.CompareExchange(ref _connections[i], newConnection, null) == null) - { - break; - } - } - - Debug.Assert(i < MaxPoolSize, $"Could not find free slot in {_connections} when opening."); - if (i == MaxPoolSize) - { - //TODO: generic exception? - throw new Exception($"Could not find free slot in {_connections} when opening. Please report a bug."); - } - - return Task.FromResult(newConnection); - } - catch - { - // Physical open failed, decrement the open and busy counter back down. - Interlocked.Decrement(ref _numConnections); - - // In case there's a waiting attempt on the channel, we write a null to the idle connection channel - // to wake it up, so it will try opening (and probably throw immediately) - // Statement order is important since we have synchronous completions on the channel. - _idleConnectionWriter.TryWrite(null); - - throw; - } + //TODO: pass through transaction + obj.ActivateConnection(null); + } + catch + { + // if Activate throws an exception + // put it back in the pool or have it properly disposed of + this.ReturnInternalConnection(obj, owningObject); + throw; } - - return Task.FromResult(null); } #endregion } From ec27e63d812546afde0f67474f438a301bb2680f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 12 Jun 2025 11:40:02 -0700 Subject: [PATCH 11/28] Fix channels package resolution. --- .../netfx/src/Microsoft.Data.SqlClient.csproj | 2 +- tools/specs/Microsoft.Data.SqlClient.nuspec | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj index c883a498d7..78c589435e 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj @@ -1015,7 +1015,7 @@ - + diff --git a/tools/specs/Microsoft.Data.SqlClient.nuspec b/tools/specs/Microsoft.Data.SqlClient.nuspec index 0f029b4c19..4199ec0097 100644 --- a/tools/specs/Microsoft.Data.SqlClient.nuspec +++ b/tools/specs/Microsoft.Data.SqlClient.nuspec @@ -40,6 +40,7 @@ + From c4d5808e67794cc3e6dd32177fb0be7a59d1dbc5 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 16 Jun 2025 15:47:46 -0700 Subject: [PATCH 12/28] Review changes. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 240 +++++++++--------- .../ConnectionPool/DbConnectionPoolState.cs | 1 - .../WaitHandleDbConnectionPool.cs | 6 - 3 files changed, 126 insertions(+), 121 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 956368aeab..896c3699a0 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -32,9 +32,12 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool // between sync and async tasks. private static SemaphoreSlim _syncOverAsyncSemaphore = new(Math.Max(1, Environment.ProcessorCount / 2)); - private static int _objectTypeCount; // EventSource counter + /// + /// Tracks the number of instances of this class. Used to generate unique IDs for each instance. + /// + private static int _instanceCount; - private readonly int _objectID = Interlocked.Increment(ref _objectTypeCount); + private readonly int _instanceID = Interlocked.Increment(ref _instanceCount); /// /// Tracks all connections currently managed by this pool, whether idle or busy. @@ -62,18 +65,12 @@ internal ChannelDbConnectionPool( DbConnectionPoolIdentity identity, DbConnectionPoolProviderInfo connectionPoolProviderInfo) { - State = Initializing; - ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; PoolGroupOptions = connectionPoolGroup.PoolGroupOptions; ProviderInfo = connectionPoolProviderInfo; Identity = identity; - AuthenticationContexts = new ConcurrentDictionary< - DbConnectionPoolAuthenticationContextKey, - DbConnectionPoolAuthenticationContext>( - concurrencyLevel: 4 * Environment.ProcessorCount, - capacity: 2); + AuthenticationContexts = new(); _connections = new DbConnectionInternal[MaxPoolSize]; @@ -110,7 +107,7 @@ public DbConnectionFactory ConnectionFactory public bool ErrorOccurred => throw new NotImplementedException(); /// - public int Id => _objectID; + public int Id => _instanceID; /// public DbConnectionPoolIdentity Identity @@ -120,7 +117,7 @@ public DbConnectionPoolIdentity Identity } /// - public bool IsRunning => State is Running; + public bool IsRunning => State == Running; /// public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout; @@ -167,7 +164,7 @@ public void Clear() } /// - public void PutObjectFromTransactedPool(DbConnectionInternal obj) + public void PutObjectFromTransactedPool(DbConnectionInternal connection) { throw new NotImplementedException(); } @@ -179,35 +176,35 @@ public DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConne } /// - public void ReturnInternalConnection(DbConnectionInternal obj, object owningObject) + public void ReturnInternalConnection(DbConnectionInternal connection, object owningObject) { - lock (obj) + lock (connection) { // Calling PrePush prevents the object from being reclaimed // once we leave the lock, because it sets _pooledCount such // that it won't appear to be out of the pool. What that // means, is that we're now responsible for this connection: // it won't get reclaimed if it gets lost. - obj.PrePush(owningObject); + connection.PrePush(owningObject); } - if (!CheckConnection(obj)) + if (!CheckConnection(connection)) { return; } - SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Deactivating.", Id, obj.ObjectID); - obj.DeactivateConnection(); + SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Deactivating.", Id, connection.ObjectID); + connection.DeactivateConnection(); - if (obj.IsConnectionDoomed || - !obj.CanBePooled || - State is ShuttingDown) + if (connection.IsConnectionDoomed || + !connection.CanBePooled || + State == ShuttingDown) { - CloseConnection(obj); + CloseConnection(connection); } else { - var written = _idleConnectionWriter.TryWrite(obj); + var written = _idleConnectionWriter.TryWrite(connection); Debug.Assert(written, "Failed to write returning connection to the idle channel."); } } @@ -233,71 +230,15 @@ public void TransactionEnded(Transaction transaction, DbConnectionInternal trans /// public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) { - // If taskCompletionSource is not null, we are in an async context. - if (taskCompletionSource is not null) - { - - // Early exit if the task is already completed. - if (taskCompletionSource.Task.IsCompleted) - { - connection = null; - return false; - } - - /* - * This is ugly, but async anti-patterns above and below us in the stack necessitate a fresh task to be - * created. Ideally we would just return the Task from GetInternalConnection and let the caller await - * it as needed, but instead we need to signal to the provided TaskCompletionSource when the connection - * is established. This pattern has implications for connection open retry logic that are intricate - * enough to merit dedicated work. For now, callers that need to open many connections asynchronously - * and in parallel *must* pre-prevision threads in the managed thread pool to avoid exhaustion and - * timeouts. - * - * Also note that we don't have access to the cancellation token passed by the caller to the original - * OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token - * is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's - * ConnectionTimeout. - */ - Task.Run(async () => - { - if (taskCompletionSource.Task.IsCompleted) - { - return; - } - - // We're potentially on a new thread, so we need to properly set the ambient transaction. - // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState - // so that we can access it here. Read: area for improvement. - ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); - - try - { - var connection = await GetInternalConnection( - owningObject, - userOptions, - TimeSpan.FromSeconds(owningObject.ConnectionTimeout), - async: true, - CancellationToken.None - ).ConfigureAwait(false); - taskCompletionSource.SetResult(connection); - } - catch (Exception e) - { - taskCompletionSource.SetException(e); - } - }); - - connection = null; - return false; - } - else + // If taskCompletionSource is null, we are in a sync context. + if (taskCompletionSource is null) { var task = GetInternalConnection( - owningObject, - userOptions, - TimeSpan.FromSeconds(owningObject.ConnectionTimeout), - async: false, - CancellationToken.None); + owningObject, + userOptions, + TimeSpan.FromSeconds(owningObject.ConnectionTimeout), + async: false, + CancellationToken.None); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -306,6 +247,59 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource + { + if (taskCompletionSource.Task.IsCompleted) + { + return; + } + + // We're potentially on a new thread, so we need to properly set the ambient transaction. + // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState + // so that we can access it here. Read: area for improvement. + ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + + try + { + var connection = await GetInternalConnection( + owningObject, + userOptions, + TimeSpan.FromSeconds(owningObject.ConnectionTimeout), + async: true, + CancellationToken.None + ).ConfigureAwait(false); + taskCompletionSource.SetResult(connection); + } + catch (Exception e) + { + taskCompletionSource.SetException(e); + } + }); + + connection = null; + return false; } /// @@ -314,6 +308,10 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource - /// Checks that the provided connector is live and unexpired and closes it if needed. + /// Checks that the provided connection is live and unexpired and closes it if needed. /// /// - /// Returns true if the connector is live and unexpired, otherwise returns false. - private bool CheckConnection(DbConnectionInternal? connection) + /// Returns true if the connection is live and unexpired, otherwise returns false. + private bool CheckConnection(DbConnectionInternal connection) { if (connection == null) { @@ -413,26 +412,27 @@ private bool CheckConnection(DbConnectionInternal? connection) } /// - /// Closes the provided connector and adjust pool state accordingly. + /// Closes the provided connection and removes it from the pool. /// - /// The connector to be closed. + /// The connection to be closed. private void CloseConnection(DbConnectionInternal connection) { connection.Dispose(); - int i; - for (i = 0; i < MaxPoolSize; i++) + bool found = false; + for (int i = 0; i < _connections.Length; i++) { if (Interlocked.CompareExchange(ref _connections[i], null, connection) == connection) { + found = true; break; } } // If CloseConnection is being called from within OpenNewConnection (e.g. an error happened during a connection initializer which - // causes the connector to Break, and therefore return the connector), then we haven't yet added the connector to Connections. + // causes the connection to Break, and therefore return the connection), then we haven't yet added the connection to Connections. // In this case, there's no state to revert here (that's all taken care of in OpenNewConnection), skip it. - if (i == MaxPoolSize) + if (!found) { return; } @@ -440,21 +440,22 @@ private void CloseConnection(DbConnectionInternal connection) var numConnections = Interlocked.Decrement(ref _numConnections); Debug.Assert(numConnections >= 0); - // If a connection has been closed for any reason, we write a null to the idle connection channel to wake up - // a waiter, who will open a new physical connection - // Statement order is important since we have synchronous completions on the channel. + // This connection was tracked by the pool, so closing it has opened a free spot in the pool. + // Write a null to the idle connection channel to wake up a waiter, who can now open a new + // connection. Statement order is important since we have synchronous completions on the channel. _idleConnectionWriter.TryWrite(null); } /// /// Tries to read a connection from the idle connection channel. /// - /// Returns true if a valid idles connection is found, otherwise returns false. + /// Returns true if a valid idle connection is found, otherwise returns false. private DbConnectionInternal? GetIdleConnection() { + // The channel may contain nulls. Read until we find a non-null connection or exhaust the channel. while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { - if (CheckConnection(connection)) + if (connection is not null && CheckConnection(connection)) { return connection; } @@ -463,6 +464,15 @@ private void CloseConnection(DbConnectionInternal connection) return null; } + /// + /// Gets an internal connection from the pool, either by retrieving an idle connection or opening a new one. + /// + /// The DbConnection that will own this internal connection + /// The user options to set on the internal connection + /// After this TimeSpan, the operation will timeout if a connection is not retrieved. + /// A boolean indicating whether the operation should be asynchronous. + /// A token to observe while waiting for the task to complete. + /// Returns a DbConnectionInternal that is retrieved from the pool. private async Task GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) { // First pass @@ -573,24 +583,26 @@ private async Task GetInternalConnection(DbConnection owni /// Sets connection state and activates the connection for use. Should always be called after a connection is created or retrieved from the pool. /// /// The owning DbConnection instance. - /// The DbConnectionInternal to be activated. - private void PrepareConnection(DbConnection owningObject, DbConnectionInternal obj) + /// The DbConnectionInternal to be activated. + private void PrepareConnection(DbConnection owningObject, DbConnectionInternal connection) { - lock (obj) - { // Protect against Clear and ReclaimEmancipatedObjects, which call IsEmancipated, which is affected by PrePush and PostPop - obj.PostPop(owningObject); + lock (connection) + { + // Protect against Clear which calls IsEmancipated, which is affected by PrePush and PostPop + connection.PostPop(owningObject); } try { //TODO: pass through transaction - obj.ActivateConnection(null); + connection.ActivateConnection(null); } catch { - // if Activate throws an exception - // put it back in the pool or have it properly disposed of - this.ReturnInternalConnection(obj, owningObject); + // At this point, the connection is "out of the pool" (the call to postpop). If we hit a transient + // error anywhere along the way when enlisting the connection in the transaction, we need to get + // the connection back into the pool so that it isn't leaked. + this.ReturnInternalConnection(connection, owningObject); throw; } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs index 1790e38a57..ca4353f2c6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolState.cs @@ -6,7 +6,6 @@ namespace Microsoft.Data.SqlClient.ConnectionPool { internal enum DbConnectionPoolState { - Initializing, Running, ShuttingDown, } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index 77d2b3bae1..d1917293bf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -451,8 +451,6 @@ internal WaitHandleDbConnectionPool( throw ADP.InternalError(ADP.InternalErrorCode.AttemptingToPoolOnRestrictedToken); } - State = Initializing; - lock (s_random) { // Random.Next is not thread-safe @@ -854,10 +852,6 @@ private void DeactivateObject(DbConnectionInternal obj) } else { - // NOTE: constructor should ensure that current state cannot be State.Initializing, so it can only - // be State.Running or State.ShuttingDown - Debug.Assert(State is Running or ShuttingDown); - lock (obj) { // A connection with a delegated transaction cannot currently From 0451b5046c8cd6930d6bfcc26512de11548e5702 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 20 Jun 2025 13:22:04 -0700 Subject: [PATCH 13/28] Set CreateTime in constructor. This makes sense becuase CreateTime cannot be modified and should be set regardless of pooling status. --- .../src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index 39f65727c9..1c6ed4a239 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -88,6 +88,7 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all AllowSetConnectionString = allowSetConnectionString; ShouldHidePassword = hidePassword; State = state; + CreateTime = DateTime.UtcNow; } #region Properties @@ -98,7 +99,7 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all internal DateTime CreateTime { get; - private set; + init; } internal bool AllowSetConnectionString { get; } @@ -705,7 +706,6 @@ internal void MakeNonPooledObject(DbConnection owningObject) /// internal void MakePooledConnection(IDbConnectionPool connectionPool) { - CreateTime = DateTime.UtcNow; Pool = connectionPool; } From 3f79f990cbc6208e37394188207170b166084884 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 20 Jun 2025 13:31:34 -0700 Subject: [PATCH 14/28] Naming --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 896c3699a0..557daf89ce 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -26,7 +26,7 @@ namespace Microsoft.Data.SqlClient.ConnectionPool internal sealed class ChannelDbConnectionPool : IDbConnectionPool { #region Fields - // Prevents synchronous operations which depend on async operations on managed + // Limits synchronous operations which depend on async operations on managed // threads from blocking on all available threads, which would stop async tasks // from being scheduled and cause deadlocks. Use ProcessorCount/2 as a balance // between sync and async tasks. @@ -37,7 +37,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// private static int _instanceCount; - private readonly int _instanceID = Interlocked.Increment(ref _instanceCount); + private readonly int _instanceId = Interlocked.Increment(ref _instanceCount); /// /// Tracks all connections currently managed by this pool, whether idle or busy. @@ -107,7 +107,7 @@ public DbConnectionFactory ConnectionFactory public bool ErrorOccurred => throw new NotImplementedException(); /// - public int Id => _instanceID; + public int Id => _instanceId; /// public DbConnectionPoolIdentity Identity From 2d260bb8770175bd0ac1abbaf06f6f617970a658 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Fri, 20 Jun 2025 13:57:14 -0700 Subject: [PATCH 15/28] Fix potential connection leak when setting TaskCompletionSource result. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 557daf89ce..ab23f8993b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -280,10 +280,11 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource Date: Tue, 24 Jun 2025 13:46:55 -0700 Subject: [PATCH 16/28] Fix package reference for central package management. --- src/Directory.Packages.props | 1 + .../netfx/ref/Microsoft.Data.SqlClient.csproj | 1 + .../src/Microsoft.Data.SqlClient.csproj | 3 ++- tools/specs/Microsoft.Data.SqlClient.nuspec | 3 ++- 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 7d94425123..b5148713c5 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -10,6 +10,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj index 6b507b5a0a..bb2fe8ee4d 100644 --- a/src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.csproj @@ -46,6 +46,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj index 24f1bb0a4e..ddd79c9430 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj @@ -5,9 +5,10 @@ - + + diff --git a/tools/specs/Microsoft.Data.SqlClient.nuspec b/tools/specs/Microsoft.Data.SqlClient.nuspec index 4199ec0097..ad04728342 100644 --- a/tools/specs/Microsoft.Data.SqlClient.nuspec +++ b/tools/specs/Microsoft.Data.SqlClient.nuspec @@ -40,7 +40,7 @@ - + @@ -77,6 +77,7 @@ + From a440e10625fc40450d54e136f42755b2b5b9c0d0 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 24 Jun 2025 13:47:20 -0700 Subject: [PATCH 17/28] Fix formatting. Refactor get connection loop. --- .../Data/ProviderBase/DbConnectionInternal.cs | 6 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 302 ++++++++---------- .../ConnectionPool/IDbConnectionPool.cs | 4 +- .../ChannelDbConnectionPoolTest.cs | 3 - 4 files changed, 138 insertions(+), 177 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index 1c6ed4a239..aeb8fe1147 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -96,11 +96,7 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all /// /// When the connection was created. /// - internal DateTime CreateTime - { - get; - init; - } + internal DateTime CreateTime { get; init; } internal bool AllowSetConnectionString { get; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ab23f8993b..ee0f267d66 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -5,6 +5,7 @@ using System.Collections.Concurrent; using System.Data.Common; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; @@ -86,18 +87,12 @@ internal ChannelDbConnectionPool( #region Properties /// - public ConcurrentDictionary AuthenticationContexts - { - get; - init; - } + public ConcurrentDictionary< + DbConnectionPoolAuthenticationContextKey, + DbConnectionPoolAuthenticationContext> AuthenticationContexts { get; init; } /// - public DbConnectionFactory ConnectionFactory - { - get; - init; - } + public DbConnectionFactory ConnectionFactory { get; init; } /// /// Note: maintain _numConnections backing field to enable atomic operations @@ -110,11 +105,7 @@ public DbConnectionFactory ConnectionFactory public int Id => _instanceId; /// - public DbConnectionPoolIdentity Identity - { - get; - private set; - } + public DbConnectionPoolIdentity Identity { get; init; } /// public bool IsRunning => State == Running; @@ -123,32 +114,16 @@ public DbConnectionPoolIdentity Identity public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout; /// - public DbConnectionPoolGroup PoolGroup - { - get; - init; - } + public DbConnectionPoolGroup PoolGroup { get; init; } /// - public DbConnectionPoolGroupOptions PoolGroupOptions - { - get; - init; - } + public DbConnectionPoolGroupOptions PoolGroupOptions { get; init; } /// - public DbConnectionPoolProviderInfo ProviderInfo - { - get; - init; - } + public DbConnectionPoolProviderInfo ProviderInfo { get; init; } /// - public DbConnectionPoolState State - { - get; - private set; - } + public DbConnectionPoolState State { get; private set; } /// public bool UseLoadBalancing => PoolGroupOptions.UseLoadBalancing; @@ -170,7 +145,10 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) } /// - public DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConnectionOptions userOptions, DbConnectionInternal oldConnection) + public DbConnectionInternal ReplaceConnection( + DbConnection owningObject, + DbConnectionOptions userOptions, + DbConnectionInternal oldConnection) { throw new NotImplementedException(); } @@ -188,19 +166,22 @@ public void ReturnInternalConnection(DbConnectionInternal connection, object own connection.PrePush(owningObject); } - if (!CheckConnection(connection)) + if (!IsLiveConnection(connection)) { return; } - SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Deactivating.", Id, connection.ObjectID); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Deactivating.", + Id, + connection.ObjectID); connection.DeactivateConnection(); if (connection.IsConnectionDoomed || !connection.CanBePooled || State == ShuttingDown) { - CloseConnection(connection); + RemoveConnection(connection); } else { @@ -228,17 +209,23 @@ public void TransactionEnded(Transaction transaction, DbConnectionInternal trans } /// - public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection) + public bool TryGetConnection( + DbConnection owningObject, + TaskCompletionSource taskCompletionSource, + DbConnectionOptions userOptions, + out DbConnectionInternal? connection) { + var timeout = TimeSpan.FromSeconds(owningObject.ConnectionTimeout); + using CancellationTokenSource cancellationTokenSource = new(timeout); + // If taskCompletionSource is null, we are in a sync context. if (taskCompletionSource is null) { var task = GetInternalConnection( owningObject, userOptions, - TimeSpan.FromSeconds(owningObject.ConnectionTimeout), async: false, - CancellationToken.None); + cancellationTokenSource.Token); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -254,21 +241,19 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource { if (taskCompletionSource.Task.IsCompleted) @@ -287,9 +272,8 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource - internal Task OpenNewInternalConnection(DbConnection? owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) + internal Task OpenNewInternalConnection( + DbConnection? owningConnection, + DbConnectionOptions userOptions, + bool async, + CancellationToken cancellationToken) { // As long as we're under max capacity, attempt to increase the connection count and open a new connection. for (var numConnections = _numConnections; numConnections < MaxPoolSize; numConnections = _numConnections) @@ -402,22 +390,17 @@ public bool TryGetConnection(DbConnection owningObject, TaskCompletionSource /// /// Returns true if the connection is live and unexpired, otherwise returns false. - private bool CheckConnection(DbConnectionInternal connection) + private bool IsLiveConnection(DbConnectionInternal connection) { - if (connection == null) - { - return false; - } - if (!connection.IsConnectionAlive()) { - CloseConnection(connection); + RemoveConnection(connection); return false; } if (LoadBalanceTimeout != TimeSpan.Zero && DateTime.UtcNow > connection.CreateTime + LoadBalanceTimeout) { - CloseConnection(connection); + RemoveConnection(connection); return false; } @@ -428,10 +411,10 @@ private bool CheckConnection(DbConnectionInternal connection) /// Closes the provided connection and removes it from the pool. /// /// The connection to be closed. - private void CloseConnection(DbConnectionInternal connection) + private void RemoveConnection(DbConnectionInternal connection) { - connection.Dispose(); - + // Remove the connection from the pool first to ensure any exception thrown during disposal doesn't leave + // the pool in an inconsistent state. bool found = false; for (int i = 0; i < _connections.Length; i++) { @@ -442,9 +425,12 @@ private void CloseConnection(DbConnectionInternal connection) } } - // If CloseConnection is being called from within OpenNewConnection (e.g. an error happened during a connection initializer which - // causes the connection to Break, and therefore return the connection), then we haven't yet added the connection to Connections. - // In this case, there's no state to revert here (that's all taken care of in OpenNewConnection), skip it. + connection.Dispose(); + + // If CloseConnection is being called from within OpenNewConnection (e.g. an error happened during a + // connection initializer which causes the connection to Break, and therefore return the connection), then + // we haven't yet added the connection to Connections. In this case, there's no state to revert here + // (that's all taken care of in OpenNewConnection), skip it. if (!found) { return; @@ -462,13 +448,13 @@ private void CloseConnection(DbConnectionInternal connection) /// /// Tries to read a connection from the idle connection channel. /// - /// Returns true if a valid idle connection is found, otherwise returns false. + /// A connection from the idle channel, or null if the channel is empty. private DbConnectionInternal? GetIdleConnection() { // The channel may contain nulls. Read until we find a non-null connection or exhaust the channel. while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { - if (connection is not null && CheckConnection(connection)) + if (connection is not null && IsLiveConnection(connection)) { return connection; } @@ -482,118 +468,98 @@ private void CloseConnection(DbConnectionInternal connection) /// /// The DbConnection that will own this internal connection /// The user options to set on the internal connection - /// After this TimeSpan, the operation will timeout if a connection is not retrieved. /// A boolean indicating whether the operation should be asynchronous. /// A token to observe while waiting for the task to complete. /// Returns a DbConnectionInternal that is retrieved from the pool. - private async Task GetInternalConnection(DbConnection owningConnection, DbConnectionOptions userOptions, TimeSpan timeout, bool async, CancellationToken cancellationToken) + private async Task GetInternalConnection( + DbConnection owningConnection, + DbConnectionOptions userOptions, + bool async, + CancellationToken cancellationToken) { - // First pass - // - // Try to get an idle connection from the channel or open a new one. - DbConnectionInternal? connection = GetIdleConnection(); + DbConnectionInternal? connection = null; - connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); - - if (connection is not null) + // Continue looping until we create or retrieve a connection + do { - PrepareConnection(owningConnection, connection); - return connection; - } - - // Second pass - // - // We're at max capacity. Block on the idle channel with a timeout. - // Note that Channels guarantee fair FIFO behavior to callers of ReadAsync (first-come first- - // served), which is crucial to us. - using CancellationTokenSource linkedSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - CancellationToken finalToken = linkedSource.Token; - linkedSource.CancelAfter(timeout); - - try - { - // Continue looping around until we create/retrieve a connection or the timeout expires. - while (true && !finalToken.IsCancellationRequested) + try { - try - { - if (async) - { - connection = await _idleConnectionReader.ReadAsync(finalToken).ConfigureAwait(false); - } - else - { - // If there are no connections in the channel, then ReadAsync will block until one is available. - // Channels doesn't offer a sync API, so running ReadAsync synchronously on this thread may spawn - // additional new async work items in the managed thread pool if there are no items available in the - // channel. We need to ensure that we don't block all available managed threads with these child - // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput - // to the managed threadpool. - _syncOverAsyncSemaphore.Wait(finalToken); - try - { - ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = - _idleConnectionReader.ReadAsync(finalToken).ConfigureAwait(false).GetAwaiter(); - using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); - - // Cancellation happens through the ReadAsync call, which will complete the task. - awaiter.UnsafeOnCompleted(() => mres.Set()); - mres.Wait(CancellationToken.None); - connection = awaiter.GetResult(); - } - finally - { - _syncOverAsyncSemaphore.Release(); - } - } + // Optimistically try to get an idle connection from the channel + // Doesn't wait if the channel is empty, just returns null. + connection ??= GetIdleConnection(); - if (connection is not null && CheckConnection(connection)) - { - PrepareConnection(owningConnection, connection); - return connection; - } - } - catch (OperationCanceledException) - { - cancellationToken.ThrowIfCancellationRequested(); - Debug.Assert(finalToken.IsCancellationRequested); - throw ADP.PooledOpenTimeout(); - } - catch (ChannelClosedException) + // We didn't find an idle connection, try to open a new one. + connection ??= await OpenNewInternalConnection( + owningConnection, + userOptions, + async, + cancellationToken).ConfigureAwait(false); + + // We're at max capacity. Block on the idle channel with a timeout. + // Note that Channels guarantee fair FIFO behavior to callers of ReadAsync (first-come first- + // served), which is crucial to us. + if (async) { - //TODO: exceptions from resource file - throw new Exception("The connection pool has been shut down."); + connection ??= await _idleConnectionReader.ReadAsync(cancellationToken).ConfigureAwait(false); } - - // Third pass - // - // Try again to get an idle connection or open a new one. - // If we're here, our waiting attempt on the idle connection channel was released with a null - // (or bad connection), or we're in sync mode. Check again if a new idle connection has appeared since we last checked. - connection ??= GetIdleConnection(); - - // We might have closed a connection in the meantime and no longer be at max capacity - // so try to open a new connection and if that fails, loop again. - connection ??= await OpenNewInternalConnection(owningConnection, userOptions, timeout, async, cancellationToken).ConfigureAwait(false); - - if (connection is not null) + else { - PrepareConnection(owningConnection, connection); - return connection; + connection ??= ReadChannelSyncOverAsync(cancellationToken); } } + catch (OperationCanceledException) + { + throw ADP.PooledOpenTimeout(); + } + catch (ChannelClosedException) + { + //TODO: exceptions from resource file + throw new Exception("The connection pool has been shut down."); + } + } + while (connection is null || !IsLiveConnection(connection)); - throw ADP.PooledOpenTimeout(); + PrepareConnection(owningConnection, connection); + return connection; + } + + /// + /// Performs a blocking synchronous read from the idle connection channel. + /// + /// Cancels the read operation. + /// The connection read from the channel. + private DbConnectionInternal? ReadChannelSyncOverAsync(CancellationToken cancellationToken) + { + // If there are no connections in the channel, then ReadAsync will block until one is available. + // Channels doesn't offer a sync API, so running ReadAsync synchronously on this thread may spawn + // additional new async work items in the managed thread pool if there are no items available in the + // channel. We need to ensure that we don't block all available managed threads with these child + // tasks or we could deadlock. Prefer to block the current user-owned thread, and limit throughput + // to the managed threadpool. + + _syncOverAsyncSemaphore.Wait(cancellationToken); + try + { + ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter awaiter = + _idleConnectionReader.ReadAsync(cancellationToken).ConfigureAwait(false).GetAwaiter(); + using ManualResetEventSlim mres = new ManualResetEventSlim(false, 0); + + // Cancellation happens through the ReadAsync call, which will complete the task. + // Even a failed task will complete and set the ManualResetEventSlim. + awaiter.UnsafeOnCompleted(() => mres.Set()); + mres.Wait(CancellationToken.None); + return awaiter.GetResult(); } finally { - //TODO: metrics + _syncOverAsyncSemaphore.Release(); } } /// - /// Sets connection state and activates the connection for use. Should always be called after a connection is created or retrieved from the pool. + /// Sets connection state and activates the connection for use. Should always be called after a connection is + /// created or retrieved from the pool. /// /// The owning DbConnection instance. /// The DbConnectionInternal to be activated. @@ -619,6 +585,6 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c throw; } } - #endregion +#endregion } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs index fba0a2af27..8d8482e59f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs @@ -10,6 +10,8 @@ using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; +#nullable enable + namespace Microsoft.Data.SqlClient.ConnectionPool { /// @@ -104,7 +106,7 @@ internal interface IDbConnectionPool /// The user options to use if a new connection must be opened. /// The retrieved connection will be passed out via this parameter. /// True if a connection was set in the out parameter, otherwise returns false. - bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal connection); + bool TryGetConnection(DbConnection owningObject, TaskCompletionSource taskCompletionSource, DbConnectionOptions userOptions, out DbConnectionInternal? connection); /// /// Replaces the internal connection currently associated with owningObject with a new internal connection from the pool. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 7f69b3ae6d..8b049ec876 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -658,9 +658,6 @@ public void TestStateGetter() public void TestStateSetter() { Setup(SuccessfulConnectionFactory); - pool.State = DbConnectionPoolState.ShuttingDown; - Assert.Equal(DbConnectionPoolState.ShuttingDown, pool.State); - pool.State = DbConnectionPoolState.Running; Assert.Equal(DbConnectionPoolState.Running, pool.State); } From 456ea1fa724495729043fef865b4320f005e18ea Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 26 Jun 2025 09:24:38 -0700 Subject: [PATCH 18/28] Move concurrent data structures out to dedicated class. --- .../BoundedConcurrentDictionary.cs | 43 +++++++++++ .../ConnectionPool/ChannelDbConnectionPool.cs | 73 ++++--------------- 2 files changed, 57 insertions(+), 59 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs new file mode 100644 index 0000000000..45069e5721 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs @@ -0,0 +1,43 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Concurrent; +using System.Threading; +using System.Threading.Tasks; + +namespace Microsoft.Data.SqlClient.ConnectionPool +{ + internal class BoundedConcurrentDictionary + { + private readonly ConcurrentDictionary items; + private readonly SemaphoreSlim _semaphore; + private readonly int _capacity; + + public BoundedConcurrentDictionary(int capacity) + { + _capacity = capacity; + _semaphore = new(_capacity); + items = new ConcurrentDictionary(); + } + + public bool TryReserve() => _semaphore.Wait(0); + + public void ReleaseReservation() => _semaphore.Release(); + + public bool TryAdd(T item) => items.TryAdd(item, 0); + + public bool TryRemove(T item) + { + if (items.TryRemove(item, out _)) + { + ReleaseReservation(); + return true; + } + return false; + } + + public int Count => _capacity - _semaphore.CurrentCount; + } +} diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index ee0f267d66..47b0ba0ba0 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -44,7 +44,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// Tracks all connections currently managed by this pool, whether idle or busy. /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts. /// - private readonly DbConnectionInternal?[] _connections; + private readonly BoundedConcurrentDictionary _connections; /// /// Reader side for the idle connection channel. Contains nulls in order to release waiting attempts after @@ -52,9 +52,6 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// private readonly ChannelReader _idleConnectionReader; private readonly ChannelWriter _idleConnectionWriter; - - // Counts the total number of open connections tracked by the pool. - private volatile int _numConnections; #endregion /// @@ -73,7 +70,7 @@ internal ChannelDbConnectionPool( Identity = identity; AuthenticationContexts = new(); - _connections = new DbConnectionInternal[MaxPoolSize]; + _connections = new(MaxPoolSize); // We enforce Max Pool Size, so no need to create a bounded channel (which is less efficient) // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents @@ -96,7 +93,7 @@ public ConcurrentDictionary< /// /// Note: maintain _numConnections backing field to enable atomic operations - public int Count => _numConnections; + public int Count => _connections.Count; /// public bool ErrorOccurred => throw new NotImplementedException(); @@ -306,19 +303,8 @@ public bool TryGetConnection( bool async, CancellationToken cancellationToken) { - // As long as we're under max capacity, attempt to increase the connection count and open a new connection. - for (var numConnections = _numConnections; numConnections < MaxPoolSize; numConnections = _numConnections) + if(_connections.TryReserve()) { - // Try to reserve a spot in the pool by incrementing _numConnections. - // If _numConnections changed underneath us, then another thread already reserved a spot. - // Cycle back through the check above to reset our expected numConnections and to make sure we don't go - // over MaxPoolSize. - // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 - if (Interlocked.CompareExchange(ref _numConnections, numConnections + 1, numConnections) != numConnections) - { - continue; - } - try { // We've managed to increase the open counter, open a physical connection. @@ -350,20 +336,10 @@ public bool TryGetConnection( newConnection.PrePush(null); - int i; - for (i = 0; i < MaxPoolSize; i++) - { - if (Interlocked.CompareExchange(ref _connections[i], newConnection, null) == null) - { - break; - } - } - - Debug.Assert(i < MaxPoolSize, $"Could not find free slot in {_connections} when opening."); - if (i == MaxPoolSize) - { - //TODO: generic exception? - throw new Exception($"Could not find free slot in {_connections} when opening. Please report a bug."); + if (!_connections.TryAdd(newConnection) { + // If we failed to add the connection to the pool, it means that another thread has already + // added it. We need to decrement the open counter. + throw ADP.InternalError(ADP.InternalErrorCode.PooledObjectInPoolMoreThanOnce); } return Task.FromResult(newConnection); @@ -371,7 +347,7 @@ public bool TryGetConnection( catch { // Physical open failed, decrement the open and busy counter back down. - Interlocked.Decrement(ref _numConnections); + _connections.ReleaseReservation(); // In case there's a waiting attempt on the channel, we write a null to the idle connection channel // to wake it up, so it will try opening (and probably throw immediately) @@ -413,36 +389,15 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// The connection to be closed. private void RemoveConnection(DbConnectionInternal connection) { - // Remove the connection from the pool first to ensure any exception thrown during disposal doesn't leave - // the pool in an inconsistent state. - bool found = false; - for (int i = 0; i < _connections.Length; i++) + if(_connections.TryRemove(connection)) { - if (Interlocked.CompareExchange(ref _connections[i], null, connection) == connection) - { - found = true; - break; - } + // This connection was tracked by the pool, so closing it has opened a free spot in the pool. + // Write a null to the idle connection channel to wake up a waiter, who can now open a new + // connection. Statement order is important since we have synchronous completions on the channel. + _idleConnectionWriter.TryWrite(null); } connection.Dispose(); - - // If CloseConnection is being called from within OpenNewConnection (e.g. an error happened during a - // connection initializer which causes the connection to Break, and therefore return the connection), then - // we haven't yet added the connection to Connections. In this case, there's no state to revert here - // (that's all taken care of in OpenNewConnection), skip it. - if (!found) - { - return; - } - - var numConnections = Interlocked.Decrement(ref _numConnections); - Debug.Assert(numConnections >= 0); - - // This connection was tracked by the pool, so closing it has opened a free spot in the pool. - // Write a null to the idle connection channel to wake up a waiter, who can now open a new - // connection. Statement order is important since we have synchronous completions on the channel. - _idleConnectionWriter.TryWrite(null); } /// From 46dd516445c1b4661ea9d3da00ee901be9ec2165 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 26 Jun 2025 16:30:41 -0700 Subject: [PATCH 19/28] Refactor data structure to remove generics, revert to previous logic. --- .../src/Microsoft.Data.SqlClient.csproj | 3 + .../netfx/src/Microsoft.Data.SqlClient.csproj | 3 + .../BoundedConcurrentDictionary.cs | 43 ------ .../ConnectionPool/ChannelDbConnectionPool.cs | 56 +++---- .../ConnectionPool/ConnectionPoolSlots.cs | 138 ++++++++++++++++++ 5 files changed, 174 insertions(+), 69 deletions(-) delete mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs diff --git a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj index c49c118323..610478d3a7 100644 --- a/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj @@ -105,6 +105,9 @@ Microsoft\Data\SqlClient\ConnectionPool\ChannelDbConnectionPool.cs + + Microsoft\Data\SqlClient\ConnectionPool\ConnectionPoolSlots.cs + Microsoft\Data\SqlClient\ConnectionPool\DbConnectionPoolAuthenticationContext.cs diff --git a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj index 78c589435e..8b80b7a685 100644 --- a/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj @@ -291,6 +291,9 @@ Microsoft\Data\SqlClient\ConnectionPool\ChannelDbConnectionPool.cs + + Microsoft\Data\SqlClient\ConnectionPool\ConnectionPoolSlots.cs + Microsoft\Data\SqlClient\ConnectionPool\DbConnectionPoolAuthenticationContext.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs deleted file mode 100644 index 45069e5721..0000000000 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BoundedConcurrentDictionary.cs +++ /dev/null @@ -1,43 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using System.Collections.Concurrent; -using System.Threading; -using System.Threading.Tasks; - -namespace Microsoft.Data.SqlClient.ConnectionPool -{ - internal class BoundedConcurrentDictionary - { - private readonly ConcurrentDictionary items; - private readonly SemaphoreSlim _semaphore; - private readonly int _capacity; - - public BoundedConcurrentDictionary(int capacity) - { - _capacity = capacity; - _semaphore = new(_capacity); - items = new ConcurrentDictionary(); - } - - public bool TryReserve() => _semaphore.Wait(0); - - public void ReleaseReservation() => _semaphore.Release(); - - public bool TryAdd(T item) => items.TryAdd(item, 0); - - public bool TryRemove(T item) - { - if (items.TryRemove(item, out _)) - { - ReleaseReservation(); - return true; - } - return false; - } - - public int Count => _capacity - _semaphore.CurrentCount; - } -} diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 47b0ba0ba0..407c552945 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -44,7 +44,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// Tracks all connections currently managed by this pool, whether idle or busy. /// Only updated rarely - when physical connections are opened/closed - but is read in perf-sensitive contexts. /// - private readonly BoundedConcurrentDictionary _connections; + private readonly ConnectionPoolSlots _connectionSlots; /// /// Reader side for the idle connection channel. Contains nulls in order to release waiting attempts after @@ -70,7 +70,7 @@ internal ChannelDbConnectionPool( Identity = identity; AuthenticationContexts = new(); - _connections = new(MaxPoolSize); + _connectionSlots = new(MaxPoolSize); // We enforce Max Pool Size, so no need to create a bounded channel (which is less efficient) // On the consuming side, we have the multiplexing write loop but also non-multiplexing Rents @@ -92,8 +92,7 @@ public ConcurrentDictionary< public DbConnectionFactory ConnectionFactory { get; init; } /// - /// Note: maintain _numConnections backing field to enable atomic operations - public int Count => _connections.Count; + public int Count => _connectionSlots.ReservationCount; /// public bool ErrorOccurred => throw new NotImplementedException(); @@ -296,28 +295,37 @@ public bool TryGetConnection( return false; } - /// - internal Task OpenNewInternalConnection( + /// + /// Opens a new internal connection to the database. + /// + /// The owning connection. + /// The options for the connection. + /// Whether to open the connection asynchronously. + /// The cancellation token to cancel the operation. + /// A task representing the asynchronous operation, with a result of the new internal connection. + /// InvalidOperationException - when the newly created connection is invalid or already in the pool. + private Task OpenNewInternalConnection( DbConnection? owningConnection, DbConnectionOptions userOptions, bool async, CancellationToken cancellationToken) { - if(_connections.TryReserve()) + // Opening a connection can be a slow operation and we don't want to hold a lock for the duration. + // Instead, we reserve a connection slot prior to attempting to open a new connection and release the slot + // in case of an exception. + if (_connectionSlots.TryReserve()) { try { - // We've managed to increase the open counter, open a physical connection. var startTime = Stopwatch.GetTimestamp(); - /* TODO: This blocks the thread for several network calls! - * When running async, the blocked thread is one allocated from the managed thread pool (due to - * use of Task.Run in TryGetConnection). This is why it's critical for async callers to - * pre-provision threads in the managed thread pool. Our options are limited because - * DbConnectionInternal doesn't support an async open. It's better to block this thread and keep - * throughput high than to queue all of our opens onto a single worker thread. Add an async path - * when this support is added to DbConnectionInternal. - */ + // TODO: This blocks the thread for several network calls! + // When running async, the blocked thread is one allocated from the managed thread pool (due to + // use of Task.Run in TryGetConnection). This is why it's critical for async callers to + // pre-provision threads in the managed thread pool. Our options are limited because + // DbConnectionInternal doesn't support an async open. It's better to block this thread and keep + // throughput high than to queue all of our opens onto a single worker thread. Add an async path + // when this support is added to DbConnectionInternal. DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( this, owningConnection, @@ -325,6 +333,7 @@ public bool TryGetConnection( PoolGroup.PoolKey, userOptions); + // We don't expect these conditions to happen, but we need to check them to be thorough. if (newConnection == null) { throw ADP.InternalError(ADP.InternalErrorCode.CreateObjectReturnedNull); @@ -336,24 +345,18 @@ public bool TryGetConnection( newConnection.PrePush(null); - if (!_connections.TryAdd(newConnection) { - // If we failed to add the connection to the pool, it means that another thread has already - // added it. We need to decrement the open counter. - throw ADP.InternalError(ADP.InternalErrorCode.PooledObjectInPoolMoreThanOnce); - } + _connectionSlots.Add(newConnection); return Task.FromResult(newConnection); } catch { - // Physical open failed, decrement the open and busy counter back down. - _connections.ReleaseReservation(); + _connectionSlots.ReleaseReservation(); // In case there's a waiting attempt on the channel, we write a null to the idle connection channel // to wake it up, so it will try opening (and probably throw immediately) - // Statement order is important since we have synchronous completions on the channel. _idleConnectionWriter.TryWrite(null); - + throw; } } @@ -389,8 +392,9 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// The connection to be closed. private void RemoveConnection(DbConnectionInternal connection) { - if(_connections.TryRemove(connection)) + if(_connectionSlots.TryRemove(connection)) { + _connectionSlots.ReleaseReservation(); // This connection was tracked by the pool, so closing it has opened a free spot in the pool. // Write a null to the idle connection channel to wake up a waiter, who can now open a new // connection. Statement order is important since we have synchronous completions on the channel. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs new file mode 100644 index 0000000000..6e4feac370 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs @@ -0,0 +1,138 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Diagnostics; +using System.Threading; +using Microsoft.Data.ProviderBase; + +#nullable enable + +namespace Microsoft.Data.SqlClient.ConnectionPool +{ + /// + /// A thread-safe array that allows reservations. + /// A reservation *must* be made before adding a connection to the collection. + /// Exceptions *must* be handled by the caller when trying to add connections + /// and the caller *must* release the reservation. + /// + /// + /// + /// ConnectionPoolSlots slots = new ConnectionPoolSlots(100); + /// + /// if (slots.TryReserve()) + /// { + /// try { + /// var connection = OpenConnection(); + /// slots.Add(connection); + /// catch (InvalidOperationException ex) + /// { + /// slots.ReleaseReservation(); + /// throw; + /// } + /// } + /// + /// if (slots.TryRemove()) + /// { + /// slots.ReleaseReservation(); + /// } + /// + /// + /// + internal class ConnectionPoolSlots + { + private readonly DbConnectionInternal?[] _connections; + private readonly int _capacity; + private volatile int _reservations; + + /// + /// Constructs a ConnectionPoolSlots instance with the given capacity. + /// + /// The capacity of the collection. + public ConnectionPoolSlots(int capacity) + { + _capacity = capacity; + _reservations = 0; + _connections = new DbConnectionInternal?[capacity]; + } + + /// + /// Attempts to reserve a spot in the collection. + /// + /// True if a reservation was successfully obtained. + public bool TryReserve() + { + for (var expected = _reservations; expected < _capacity; expected = _reservations) + { + // Try to reserve a spot in the collection by incrementing _reservations. + // If _reservations changed underneath us, then another thread already reserved the spot we were trying to take. + // Cycle back through the check above to reset expected and to make sure we don't go + // over capacity. + // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 + if (Interlocked.CompareExchange(ref _reservations, expected + 1, expected) != expected) + { + continue; + } + + return true; + } + return false; + } + + /// + /// Releases a reservation that was previously obtained. + /// Must be called after removing an connection from the collection or if an exception occurs. + /// + public void ReleaseReservation() + { + Interlocked.Decrement(ref _reservations); + Debug.Assert(_reservations >= 0, "Released a reservation that wasn't held"); + } + + /// + /// Adds a connection to the collection. Can only be called after a reservation has been made. + /// + /// The connection to add to the collection. + /// + /// Thrown when unable to find an empty slot. + /// This can occur if a reservation is not taken before adding a connection. + /// + public void Add(DbConnectionInternal connection) + { + int i; + for (i = 0; i < _capacity; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], connection, null) == null) + { + return; + } + } + + throw new InvalidOperationException("Couldn't find an empty slot."); + } + + /// + /// Removes a connection from the collection. + /// + /// The connection to remove from the collection. + /// True if the connection was found and removed; otherwise, false. + public bool TryRemove(DbConnectionInternal connection) + { + for (int i = 0; i < _connections.Length; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], null, connection) == connection) + { + return true; + } + } + + return false; + } + + /// + /// Gets the total number of reservations. + /// + public int ReservationCount => _reservations; + } +} From 7613b2cabacc11ec4649cd9111a7d48bd93a9649 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 26 Jun 2025 17:00:20 -0700 Subject: [PATCH 20/28] Cleanup --- .../ref/Microsoft.Data.SqlClient.csproj | 5 ++ .../ConnectionPool/ConnectionPoolSlots.cs | 72 +++++++++---------- .../CompilerServices/IsExternalInit.netfx.cs | 2 +- 3 files changed, 42 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj index 60eff24c1c..c02c44829c 100644 --- a/src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/netcore/ref/Microsoft.Data.SqlClient.csproj @@ -45,6 +45,11 @@ + + + + + diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs index 6e4feac370..83db00fad6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs @@ -40,7 +40,7 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// /// /// - internal class ConnectionPoolSlots + internal sealed class ConnectionPoolSlots { private readonly DbConnectionInternal?[] _connections; private readonly int _capacity; @@ -50,7 +50,7 @@ internal class ConnectionPoolSlots /// Constructs a ConnectionPoolSlots instance with the given capacity. /// /// The capacity of the collection. - public ConnectionPoolSlots(int capacity) + internal ConnectionPoolSlots(int capacity) { _capacity = capacity; _reservations = 0; @@ -58,37 +58,9 @@ public ConnectionPoolSlots(int capacity) } /// - /// Attempts to reserve a spot in the collection. - /// - /// True if a reservation was successfully obtained. - public bool TryReserve() - { - for (var expected = _reservations; expected < _capacity; expected = _reservations) - { - // Try to reserve a spot in the collection by incrementing _reservations. - // If _reservations changed underneath us, then another thread already reserved the spot we were trying to take. - // Cycle back through the check above to reset expected and to make sure we don't go - // over capacity. - // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 - if (Interlocked.CompareExchange(ref _reservations, expected + 1, expected) != expected) - { - continue; - } - - return true; - } - return false; - } - - /// - /// Releases a reservation that was previously obtained. - /// Must be called after removing an connection from the collection or if an exception occurs. + /// Gets the total number of reservations. /// - public void ReleaseReservation() - { - Interlocked.Decrement(ref _reservations); - Debug.Assert(_reservations >= 0, "Released a reservation that wasn't held"); - } + internal int ReservationCount => _reservations; /// /// Adds a connection to the collection. Can only be called after a reservation has been made. @@ -98,7 +70,7 @@ public void ReleaseReservation() /// Thrown when unable to find an empty slot. /// This can occur if a reservation is not taken before adding a connection. /// - public void Add(DbConnectionInternal connection) + internal void Add(DbConnectionInternal connection) { int i; for (i = 0; i < _capacity; i++) @@ -112,12 +84,22 @@ public void Add(DbConnectionInternal connection) throw new InvalidOperationException("Couldn't find an empty slot."); } + /// + /// Releases a reservation that was previously obtained. + /// Must be called after removing a connection from the collection or if an exception occurs. + /// + internal void ReleaseReservation() + { + Interlocked.Decrement(ref _reservations); + Debug.Assert(_reservations >= 0, "Released a reservation that wasn't held"); + } + /// /// Removes a connection from the collection. /// /// The connection to remove from the collection. /// True if the connection was found and removed; otherwise, false. - public bool TryRemove(DbConnectionInternal connection) + internal bool TryRemove(DbConnectionInternal connection) { for (int i = 0; i < _connections.Length; i++) { @@ -131,8 +113,26 @@ public bool TryRemove(DbConnectionInternal connection) } /// - /// Gets the total number of reservations. + /// Attempts to reserve a spot in the collection. /// - public int ReservationCount => _reservations; + /// True if a reservation was successfully obtained. + internal bool TryReserve() + { + for (var expected = _reservations; expected < _capacity; expected = _reservations) + { + // Try to reserve a spot in the collection by incrementing _reservations. + // If _reservations changed underneath us, then another thread already reserved the spot we were trying to take. + // Cycle back through the check above to reset expected and to make sure we don't go + // over capacity. + // Note that we purposefully don't use SpinWait for this: https://github.com/dotnet/coreclr/pull/21437 + if (Interlocked.CompareExchange(ref _reservations, expected + 1, expected) != expected) + { + continue; + } + + return true; + } + return false; + } } } diff --git a/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs b/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs index 0d0181ba6d..530d1b4096 100644 --- a/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs +++ b/src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs @@ -6,7 +6,7 @@ using System.ComponentModel; - +// The `init` accessor was introduced in C# 9.0 and is not natively supported in .NET Framework. // This class enables the use of the `init` property accessor in .NET framework. namespace System.Runtime.CompilerServices { From f7b9465a9d957dd417d90409ba02387debc48206 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 26 Jun 2025 17:51:55 -0700 Subject: [PATCH 21/28] Fix timeout exception handling for async path. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 21 +++++++++---------- .../ChannelDbConnectionPoolTest.cs | 7 ------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 407c552945..12e86a412b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -212,7 +212,6 @@ public bool TryGetConnection( out DbConnectionInternal? connection) { var timeout = TimeSpan.FromSeconds(owningObject.ConnectionTimeout); - using CancellationTokenSource cancellationTokenSource = new(timeout); // If taskCompletionSource is null, we are in a sync context. if (taskCompletionSource is null) @@ -221,7 +220,7 @@ public bool TryGetConnection( owningObject, userOptions, async: false, - cancellationTokenSource.Token); + timeout); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -269,21 +268,19 @@ public bool TryGetConnection( owningObject, userOptions, async: true, - cancellationTokenSource.Token + timeout ).ConfigureAwait(false); taskCompletionSource.SetResult(connection); } - catch (InvalidOperationException) + catch (Exception e) { - // We were able to get a connection, but the task was cancelled out from under us. - // This can happen if the caller's CancellationToken is cancelled while we're waiting for a connection. if (connection != null) { + // We were able to get a connection, but the task was cancelled out from under us. + // This can happen if the caller's CancellationToken is cancelled while we're waiting for a connection. this.ReturnInternalConnection(connection, owningObject); } - } - catch (Exception e) - { + // It's possible to fail to set an exception on the TaskCompletionSource if the task is already // completed. In that case, this exception will be swallowed because nobody directly awaits this // task. @@ -428,15 +425,17 @@ private void RemoveConnection(DbConnectionInternal connection) /// The DbConnection that will own this internal connection /// The user options to set on the internal connection /// A boolean indicating whether the operation should be asynchronous. - /// A token to observe while waiting for the task to complete. + /// The timeout for the operation. /// Returns a DbConnectionInternal that is retrieved from the pool. private async Task GetInternalConnection( DbConnection owningConnection, DbConnectionOptions userOptions, bool async, - CancellationToken cancellationToken) + TimeSpan timeout) { DbConnectionInternal? connection = null; + using CancellationTokenSource cancellationTokenSource = new(timeout); + CancellationToken cancellationToken = cancellationTokenSource.Token; // Continue looping until we create or retrieve a connection do diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 8b049ec876..4aafae66a6 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -570,13 +570,6 @@ out internalConnection #region Property Tests - [Fact] - public void TestAuthenticationContexts() - { - Setup(SuccessfulConnectionFactory); - Assert.Throws(() => _ = pool.AuthenticationContexts); - } - [Fact] public void TestConnectionFactory() { From 67d04879ee965aecc93564d1b15da9efcae175ff Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 30 Jun 2025 10:49:15 -0700 Subject: [PATCH 22/28] Review comments. Fix merge conflict. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 19 ++++++---- .../ChannelDbConnectionPoolTest.cs | 36 +++++++++++++++++-- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 12e86a412b..baebfc6ed1 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -259,32 +259,37 @@ public bool TryGetConnection( // We're potentially on a new thread, so we need to properly set the ambient transaction. // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState // so that we can access it here. Read: area for improvement. - ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + // TODO: ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); DbConnectionInternal? connection = null; try { connection = await GetInternalConnection( - owningObject, - userOptions, + owningObject, + userOptions, async: true, timeout ).ConfigureAwait(false); - taskCompletionSource.SetResult(connection); + + if (!taskCompletionSource.TrySetResult(connection)) + { + // We were able to get a connection, but the task was cancelled out from under us. + // This can happen if the caller's CancellationToken is cancelled while we're waiting for a connection. + // Check the success to avoid an unnecessary exception. + this.ReturnInternalConnection(connection, owningObject); + } } catch (Exception e) { if (connection != null) { - // We were able to get a connection, but the task was cancelled out from under us. - // This can happen if the caller's CancellationToken is cancelled while we're waiting for a connection. this.ReturnInternalConnection(connection, owningObject); } // It's possible to fail to set an exception on the TaskCompletionSource if the task is already // completed. In that case, this exception will be swallowed because nobody directly awaits this // task. - taskCompletionSource.SetException(e); + taskCompletionSource.TrySetException(e); } }); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index 4aafae66a6..cb83313bba 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -711,7 +711,13 @@ public void TestTransactionEnded() #region Test classes internal class SuccessfulDbConnectionFactory : DbConnectionFactory { - protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) + protected override DbConnectionInternal CreateConnection( + DbConnectionOptions options, + DbConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + DbConnectionOptions userOptions) { return new StubDbConnectionInternal(); } @@ -768,12 +774,28 @@ internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnect { throw new NotImplementedException(); } + + internal override DbConnectionPoolProviderInfo CreateConnectionPoolProviderInfo(DbConnectionOptions connectionOptions) + { + throw new NotImplementedException(); + } + + internal override DbConnectionPoolGroupProviderInfo CreateConnectionPoolGroupProviderInfo(DbConnectionOptions connectionOptions) + { + throw new NotImplementedException(); + } #endregion } internal class TimeoutDbConnectionFactory : DbConnectionFactory { - protected override DbConnectionInternal CreateConnection(DbConnectionOptions options, DbConnectionPoolKey poolKey, object poolGroupProviderInfo, IDbConnectionPool pool, DbConnection owningConnection) + protected override DbConnectionInternal CreateConnection( + DbConnectionOptions options, + DbConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + DbConnectionOptions userOptions) { throw ADP.PooledOpenTimeout(); } @@ -830,6 +852,16 @@ internal override void SetInnerConnectionTo(DbConnection owningObject, DbConnect { throw new NotImplementedException(); } + + internal override DbConnectionPoolProviderInfo CreateConnectionPoolProviderInfo(DbConnectionOptions connectionOptions) + { + throw new NotImplementedException(); + } + + internal override DbConnectionPoolGroupProviderInfo CreateConnectionPoolGroupProviderInfo(DbConnectionOptions connectionOptions) + { + throw new NotImplementedException(); + } #endregion } From 7377e852502d4dbb039ac8ea0f2b465c4a21bc6c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 30 Jun 2025 11:21:31 -0700 Subject: [PATCH 23/28] Change object reference to full type. Fix tab. --- .../src/Microsoft.Data.SqlClient.csproj | 2 +- .../Data/ProviderBase/DbConnectionInternal.cs | 2 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 34 ++++++++++++------- .../ConnectionPool/IDbConnectionPool.cs | 2 +- .../WaitHandleDbConnectionPool.cs | 2 +- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj index a956b7a00c..9c9ae1db80 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj @@ -20,7 +20,7 @@ - + diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index aeb8fe1147..5fbd9495ab 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -756,7 +756,7 @@ internal virtual void PrepareForReplaceConnection() // By default, there is no preparation required } - internal void PrePush(object expectedOwner) + internal void PrePush(DbConnection expectedOwner) { // Called by IDbConnectionPool when we're about to be put into it's pool, we take this // opportunity to ensure ownership and pool counts are legit. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index baebfc6ed1..72c7367adf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -150,17 +150,14 @@ public DbConnectionInternal ReplaceConnection( } /// - public void ReturnInternalConnection(DbConnectionInternal connection, object owningObject) + public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection owningObject) { - lock (connection) - { - // Calling PrePush prevents the object from being reclaimed - // once we leave the lock, because it sets _pooledCount such - // that it won't appear to be out of the pool. What that - // means, is that we're now responsible for this connection: - // it won't get reclaimed if it gets lost. - connection.PrePush(owningObject); - } + // Calling PrePush prevents the object from being reclaimed + // once we leave the lock, because it sets _pooledCount such + // that it won't appear to be out of the pool. What that + // means, is that we're now responsible for this connection: + // it won't get reclaimed if it gets lost. + ValidateOwnershipAndPoolingState(owningObject, connection); if (!IsLiveConnection(connection)) { @@ -345,7 +342,7 @@ public bool TryGetConnection( throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); } - newConnection.PrePush(null); + ValidateOwnershipAndPoolingState(null, newConnection); _connectionSlots.Add(newConnection); @@ -548,6 +545,19 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c throw; } } -#endregion + + /// + /// Validates that the connection is owned by the provided DbConnection and that it is in a valid state to be returned to the pool. + /// + /// The owning DbConnection instance. + /// The DbConnectionInternal to be validated. + private void ValidateOwnershipAndPoolingState(DbConnection? owningObject, DbConnectionInternal connection) + { + lock (connection) + { + connection.PrePush(owningObject); + } + } + #endregion } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs index 8d8482e59f..b6bf23ffc5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs @@ -122,7 +122,7 @@ internal interface IDbConnectionPool /// /// The internal connection to return to the pool. /// The connection that currently owns this internal connection. Used to verify ownership. - void ReturnInternalConnection(DbConnectionInternal obj, object owningObject); + void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject); /// /// Puts an internal connection from a transacted pool back into the general pool. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index d1917293bf..453696fb67 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -1655,7 +1655,7 @@ private void PutNewObject(DbConnectionInternal obj) } - public void ReturnInternalConnection(DbConnectionInternal obj, object owningObject) + public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject) { Debug.Assert(obj != null, "null obj?"); From 901ba47c7c1a4ace5efc326b6c0b353cb700ed81 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 30 Jun 2025 11:25:33 -0700 Subject: [PATCH 24/28] cleanup --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 72c7367adf..cd06232379 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -5,7 +5,6 @@ using System.Collections.Concurrent; using System.Data.Common; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; @@ -157,7 +156,7 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti // that it won't appear to be out of the pool. What that // means, is that we're now responsible for this connection: // it won't get reclaimed if it gets lost. - ValidateOwnershipAndPoolingState(owningObject, connection); + ValidateOwnershipAndSetPoolingState(owningObject, connection); if (!IsLiveConnection(connection)) { @@ -342,7 +341,7 @@ public bool TryGetConnection( throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); } - ValidateOwnershipAndPoolingState(null, newConnection); + ValidateOwnershipAndSetPoolingState(null, newConnection); _connectionSlots.Add(newConnection); @@ -551,7 +550,7 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c /// /// The owning DbConnection instance. /// The DbConnectionInternal to be validated. - private void ValidateOwnershipAndPoolingState(DbConnection? owningObject, DbConnectionInternal connection) + private void ValidateOwnershipAndSetPoolingState(DbConnection? owningObject, DbConnectionInternal connection) { lock (connection) { From 142f0ca1dfd67b83bd386f9070bff3ba8d632da4 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 30 Jun 2025 11:38:13 -0700 Subject: [PATCH 25/28] Add link to github issue for async pathways. --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index cd06232379..6d2dbd7a40 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -317,6 +317,7 @@ public bool TryGetConnection( { var startTime = Stopwatch.GetTimestamp(); + // https://github.com/dotnet/SqlClient/issues/3459 // TODO: This blocks the thread for several network calls! // When running async, the blocked thread is one allocated from the managed thread pool (due to // use of Task.Run in TryGetConnection). This is why it's critical for async callers to From 57696049e35b1e10778021f8b00c1ca8ce1c94ae Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 1 Jul 2025 14:05:09 -0700 Subject: [PATCH 26/28] Remove side effects from IsLiveConnection. Fix exception handling and connection disposal when opening new connection. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 67 ++++++++++++------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 6d2dbd7a40..8d7d329cf8 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; using System.Collections.Concurrent; +using System.Collections.ObjectModel; using System.Data.Common; using System.Diagnostics; using System.Runtime.CompilerServices; @@ -149,17 +150,18 @@ public DbConnectionInternal ReplaceConnection( } /// - public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection owningObject) + public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection? owningObject) { // Calling PrePush prevents the object from being reclaimed // once we leave the lock, because it sets _pooledCount such // that it won't appear to be out of the pool. What that // means, is that we're now responsible for this connection: // it won't get reclaimed if it gets lost. - ValidateOwnershipAndSetPoolingState(owningObject, connection); + ValidateOwnershipAndSetPoolingState(connection, owningObject); if (!IsLiveConnection(connection)) { + RemoveConnection(connection); return; } @@ -313,6 +315,7 @@ public bool TryGetConnection( // in case of an exception. if (_connectionSlots.TryReserve()) { + DbConnectionInternal? newConnection = null; try { var startTime = Stopwatch.GetTimestamp(); @@ -325,7 +328,7 @@ public bool TryGetConnection( // DbConnectionInternal doesn't support an async open. It's better to block this thread and keep // throughput high than to queue all of our opens onto a single worker thread. Add an async path // when this support is added to DbConnectionInternal. - DbConnectionInternal? newConnection = ConnectionFactory.CreatePooledConnection( + newConnection = ConnectionFactory.CreatePooledConnection( this, owningConnection, PoolGroup.ConnectionOptions, @@ -342,7 +345,7 @@ public bool TryGetConnection( throw ADP.InternalError(ADP.InternalErrorCode.NewObjectCannotBePooled); } - ValidateOwnershipAndSetPoolingState(null, newConnection); + ValidateOwnershipAndSetPoolingState(newConnection, null); _connectionSlots.Add(newConnection); @@ -350,12 +353,14 @@ public bool TryGetConnection( } catch { - _connectionSlots.ReleaseReservation(); + // If we failed to open a new connection, we need to reset any state we modified. + // Clear the reservation we made, dispose of the connection if it was created, and wake up any waiting + // attempts on the idle connection channel. + if (newConnection is not null) + { + RemoveConnection(newConnection, trackedInSlots: false); + } - // In case there's a waiting attempt on the channel, we write a null to the idle connection channel - // to wake it up, so it will try opening (and probably throw immediately) - _idleConnectionWriter.TryWrite(null); - throw; } } @@ -372,13 +377,11 @@ private bool IsLiveConnection(DbConnectionInternal connection) { if (!connection.IsConnectionAlive()) { - RemoveConnection(connection); return false; } if (LoadBalanceTimeout != TimeSpan.Zero && DateTime.UtcNow > connection.CreateTime + LoadBalanceTimeout) { - RemoveConnection(connection); return false; } @@ -389,16 +392,19 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// Closes the provided connection and removes it from the pool. /// /// The connection to be closed. - private void RemoveConnection(DbConnectionInternal connection) + /// Indicates whether the connection is currently pooled. May be false + /// if we just created the connection and it's not tracked in the pool slots. + private void RemoveConnection(DbConnectionInternal connection, bool trackedInSlots = true) { - if(_connectionSlots.TryRemove(connection)) - { - _connectionSlots.ReleaseReservation(); - // This connection was tracked by the pool, so closing it has opened a free spot in the pool. - // Write a null to the idle connection channel to wake up a waiter, who can now open a new - // connection. Statement order is important since we have synchronous completions on the channel. - _idleConnectionWriter.TryWrite(null); + if (trackedInSlots) { + _connectionSlots.TryRemove(connection); } + + _connectionSlots.ReleaseReservation(); + // Closing a connection opens a free spot in the pool. + // Write a null to the idle connection channel to wake up a waiter, who can now open a new + // connection. Statement order is important since we have synchronous completions on the channel. + _idleConnectionWriter.TryWrite(null); connection.Dispose(); } @@ -412,10 +418,18 @@ private void RemoveConnection(DbConnectionInternal connection) // The channel may contain nulls. Read until we find a non-null connection or exhaust the channel. while (_idleConnectionReader.TryRead(out DbConnectionInternal? connection)) { - if (connection is not null && IsLiveConnection(connection)) + if (connection is null) { - return connection; + continue; } + + if (!IsLiveConnection(connection)) + { + RemoveConnection(connection); + continue; + } + + return connection; } return null; @@ -477,8 +491,15 @@ private async Task GetInternalConnection( //TODO: exceptions from resource file throw new Exception("The connection pool has been shut down."); } + + if (connection is not null && !IsLiveConnection(connection)) + { + // If the connection is not live, we need to remove it from the pool and try again. + RemoveConnection(connection); + connection = null; + } } - while (connection is null || !IsLiveConnection(connection)); + while (connection is null); PrepareConnection(owningConnection, connection); return connection; @@ -551,7 +572,7 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c /// /// The owning DbConnection instance. /// The DbConnectionInternal to be validated. - private void ValidateOwnershipAndSetPoolingState(DbConnection? owningObject, DbConnectionInternal connection) + private void ValidateOwnershipAndSetPoolingState(DbConnectionInternal connection, DbConnection? owningObject) { lock (connection) { From bc8b09422dc2941e23440faf4110ded8ccf39a35 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 7 Jul 2025 14:12:44 -0700 Subject: [PATCH 27/28] Minor review feedback. Make tests more reliable. --- .../Data/ProviderBase/DbConnectionInternal.cs | 2 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 44 +++++++++---------- .../ConnectionPool/ConnectionPoolSlots.cs | 15 ++++--- .../ChannelDbConnectionPoolTest.cs | 15 ++++++- 4 files changed, 42 insertions(+), 34 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs index 5fbd9495ab..1ed777b093 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -96,7 +96,7 @@ internal DbConnectionInternal(ConnectionState state, bool hidePassword, bool all /// /// When the connection was created. /// - internal DateTime CreateTime { get; init; } + internal DateTime CreateTime { get; } internal bool AllowSetConnectionString { get; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 8d7d329cf8..4e9b8ec353 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -86,10 +86,10 @@ internal ChannelDbConnectionPool( /// public ConcurrentDictionary< DbConnectionPoolAuthenticationContextKey, - DbConnectionPoolAuthenticationContext> AuthenticationContexts { get; init; } + DbConnectionPoolAuthenticationContext> AuthenticationContexts { get; } /// - public DbConnectionFactory ConnectionFactory { get; init; } + public DbConnectionFactory ConnectionFactory { get; } /// public int Count => _connectionSlots.ReservationCount; @@ -101,7 +101,7 @@ public ConcurrentDictionary< public int Id => _instanceId; /// - public DbConnectionPoolIdentity Identity { get; init; } + public DbConnectionPoolIdentity Identity { get; } /// public bool IsRunning => State == Running; @@ -110,13 +110,13 @@ public ConcurrentDictionary< public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout; /// - public DbConnectionPoolGroup PoolGroup { get; init; } + public DbConnectionPoolGroup PoolGroup { get; } /// - public DbConnectionPoolGroupOptions PoolGroupOptions { get; init; } + public DbConnectionPoolGroupOptions PoolGroupOptions { get; } /// - public DbConnectionPoolProviderInfo ProviderInfo { get; init; } + public DbConnectionPoolProviderInfo ProviderInfo { get; } /// public DbConnectionPoolState State { get; private set; } @@ -152,11 +152,6 @@ public DbConnectionInternal ReplaceConnection( /// public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection? owningObject) { - // Calling PrePush prevents the object from being reclaimed - // once we leave the lock, because it sets _pooledCount such - // that it won't appear to be out of the pool. What that - // means, is that we're now responsible for this connection: - // it won't get reclaimed if it gets lost. ValidateOwnershipAndSetPoolingState(connection, owningObject); if (!IsLiveConnection(connection)) @@ -274,14 +269,14 @@ public bool TryGetConnection( // We were able to get a connection, but the task was cancelled out from under us. // This can happen if the caller's CancellationToken is cancelled while we're waiting for a connection. // Check the success to avoid an unnecessary exception. - this.ReturnInternalConnection(connection, owningObject); + ReturnInternalConnection(connection, owningObject); } } catch (Exception e) { if (connection != null) { - this.ReturnInternalConnection(connection, owningObject); + ReturnInternalConnection(connection, owningObject); } // It's possible to fail to set an exception on the TaskCompletionSource if the task is already @@ -392,13 +387,9 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// Closes the provided connection and removes it from the pool. /// /// The connection to be closed. - /// Indicates whether the connection is currently pooled. May be false - /// if we just created the connection and it's not tracked in the pool slots. - private void RemoveConnection(DbConnectionInternal connection, bool trackedInSlots = true) + private void RemoveConnection(DbConnectionInternal connection) { - if (trackedInSlots) { - _connectionSlots.TryRemove(connection); - } + _connectionSlots.TryRemove(connection); _connectionSlots.ReleaseReservation(); // Closing a connection opens a free spot in the pool. @@ -463,16 +454,16 @@ private async Task GetInternalConnection( connection ??= GetIdleConnection(); - // We didn't find an idle connection, try to open a new one. + // If we didn't find an idle connection, try to open a new one. connection ??= await OpenNewInternalConnection( owningConnection, userOptions, async, cancellationToken).ConfigureAwait(false); - // We're at max capacity. Block on the idle channel with a timeout. - // Note that Channels guarantee fair FIFO behavior to callers of ReadAsync (first-come first- - // served), which is crucial to us. + // If we're at max capacity and couldn't open a connection. Block on the idle channel with a + // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync + // (first-come, first-served), which is crucial to us. if (async) { connection ??= await _idleConnectionReader.ReadAsync(cancellationToken).ConfigureAwait(false); @@ -562,7 +553,7 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c // At this point, the connection is "out of the pool" (the call to postpop). If we hit a transient // error anywhere along the way when enlisting the connection in the transaction, we need to get // the connection back into the pool so that it isn't leaked. - this.ReturnInternalConnection(connection, owningObject); + ReturnInternalConnection(connection, owningObject); throw; } } @@ -576,6 +567,11 @@ private void ValidateOwnershipAndSetPoolingState(DbConnectionInternal connection { lock (connection) { + // Calling PrePush prevents the object from being reclaimed + // once we leave the lock, because it sets _pooledCount such + // that it won't appear to be out of the pool. What that + // means, is that we're now responsible for this connection: + // it won't get reclaimed if it gets lost. connection.PrePush(owningObject); } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs index 83db00fad6..24b3389c68 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs @@ -12,7 +12,7 @@ namespace Microsoft.Data.SqlClient.ConnectionPool { /// - /// A thread-safe array that allows reservations. + /// A thread-safe collection with a fixed capacity that allows reservations. /// A reservation *must* be made before adding a connection to the collection. /// Exceptions *must* be handled by the caller when trying to add connections /// and the caller *must* release the reservation. @@ -26,6 +26,7 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// try { /// var connection = OpenConnection(); /// slots.Add(connection); + /// } /// catch (InvalidOperationException ex) /// { /// slots.ReleaseReservation(); @@ -43,18 +44,18 @@ namespace Microsoft.Data.SqlClient.ConnectionPool internal sealed class ConnectionPoolSlots { private readonly DbConnectionInternal?[] _connections; - private readonly int _capacity; + private readonly uint _capacity; private volatile int _reservations; /// - /// Constructs a ConnectionPoolSlots instance with the given capacity. + /// Constructs a ConnectionPoolSlots instance with the given fixed capacity. /// - /// The capacity of the collection. - internal ConnectionPoolSlots(int capacity) + /// The fixed capacity of the collection. + internal ConnectionPoolSlots(uint fixedCapacity) { - _capacity = capacity; + _capacity = fixedCapacity; _reservations = 0; - _connections = new DbConnectionInternal?[capacity]; + _connections = new DbConnectionInternal?[fixedCapacity]; } /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs index cb83313bba..6e3e328aaa 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -328,11 +328,18 @@ out internalConnection Assert.NotNull(internalConnection); } + // Use ManualResetEventSlim to synchronize the tasks + // and force the request queueing order. + using ManualResetEventSlim mresQueueOrder = new ManualResetEventSlim(); + using CountdownEvent allRequestsQueued = new CountdownEvent(2); + // Act var recycledTask = Task.Run(() => { DbConnectionInternal recycledConnection = null; - var exceeded = pool.TryGetConnection( + mresQueueOrder.Set(); + allRequestsQueued.Signal(); + pool.TryGetConnection( new SqlConnection(""), null, new DbConnectionOptions("", null), @@ -343,7 +350,10 @@ out recycledConnection var failedTask = Task.Run(() => { DbConnectionInternal failedConnection = null; - var exceeded2 = pool.TryGetConnection( + // Force this request to be second in the queue. + mresQueueOrder.Wait(); + allRequestsQueued.Signal(); + pool.TryGetConnection( new SqlConnection("Timeout=1"), null, new DbConnectionOptions("", null), @@ -352,6 +362,7 @@ out failedConnection return failedConnection; }); + allRequestsQueued.Wait(); pool.ReturnInternalConnection(firstConnection, firstOwningConnection); var recycledConnection = await recycledTask; From 92528d3de6131beca211cbefbd7e00f96d3ef03a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 7 Jul 2025 15:09:11 -0700 Subject: [PATCH 28/28] Fix compilation --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 4e9b8ec353..912d09da18 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -69,6 +69,7 @@ internal ChannelDbConnectionPool( ProviderInfo = connectionPoolProviderInfo; Identity = identity; AuthenticationContexts = new(); + MaxPoolSize = Convert.ToUInt32(PoolGroupOptions.MaxPoolSize); _connectionSlots = new(MaxPoolSize); @@ -124,7 +125,7 @@ public ConcurrentDictionary< /// public bool UseLoadBalancing => PoolGroupOptions.UseLoadBalancing; - private int MaxPoolSize => PoolGroupOptions.MaxPoolSize; + private uint MaxPoolSize { get; } #endregion #region Methods @@ -353,7 +354,7 @@ public bool TryGetConnection( // attempts on the idle connection channel. if (newConnection is not null) { - RemoveConnection(newConnection, trackedInSlots: false); + RemoveConnection(newConnection); } throw;