From b2f12e468b32c1f0d503a3b2b4961129e7d722c4 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 13:59:40 -0700 Subject: [PATCH 01/11] Remove unused method. --- .../src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs | 5 ----- .../SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs | 1 - .../src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs | 1 - 3 files changed, 7 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 539f79ff47..422897651a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -735,11 +735,6 @@ internal void PostPop(DbConnection newOwner) } } - internal virtual void PrepareForReplaceConnection() - { - // By default, there is no preparation required - } - /// /// Stamps with the current UTC time. The pool calls this from its /// return-to-pool path only when it intends the idle-timeout machinery to act on the value; 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 d2c3b57323..bb7b6a5b27 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 @@ -1203,7 +1203,6 @@ public DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConne { SqlClientDiagnostics.Metrics.SoftConnectRequest(); PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); - oldConnection.PrepareForReplaceConnection(); oldConnection.DeactivateConnection(); oldConnection.Dispose(); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs index 1f90eac202..dcb1b1c8c2 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs @@ -777,7 +777,6 @@ private Task CreateReplaceConnectionContinuation( if (oldConnection?.State == ConnectionState.Open) { - oldConnection.PrepareForReplaceConnection(); oldConnection.Dispose(); } From 3762d3baa13364649d7a90cf34559f72b2e98e2e Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 15:13:03 -0700 Subject: [PATCH 02/11] Remove instance-level ForceNewConnection property. Replace with explicit method parameters. --- .../Data/ProviderBase/DbConnectionClosed.cs | 2 +- .../Data/ProviderBase/DbConnectionInternal.cs | 9 ++- .../Connection/SqlConnectionInternal.cs | 2 +- .../Microsoft/Data/SqlClient/SqlConnection.cs | 64 ++++++++++++------- .../Data/SqlClient/SqlConnectionFactory.cs | 3 +- .../SqlConnectionStateTransitionTests.cs | 6 +- 6 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs index f5ae12f5a1..7d9c865e0a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs @@ -64,7 +64,7 @@ internal override bool TryOpenConnection( SqlConnectionFactory connectionFactory, TaskCompletionSource retry, TimeoutTimer timeout) => - TryOpenConnectionInternal(outerConnection, connectionFactory, retry, timeout); + TryOpenConnectionInternal(outerConnection, connectionFactory, retry, forceNewConnection: false,timeout); /// internal override void ResetConnection() => throw ADP.ClosedConnectionError(); 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 422897651a..907e491949 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs @@ -925,6 +925,7 @@ protected bool TryOpenConnectionInternal( DbConnection outerConnection, SqlConnectionFactory connectionFactory, TaskCompletionSource retry, + bool forceNewConnection, TimeoutTimer timeout) { // ?->Connecting: prevent set_ConnectionString during Open @@ -934,7 +935,13 @@ protected bool TryOpenConnectionInternal( try { connectionFactory.PermissionDemand(outerConnection); - if (!connectionFactory.TryGetConnection(outerConnection, retry, this, timeout, out openConnection)) + if (!connectionFactory.TryGetConnection( + outerConnection, + retry, + this, + timeout, + forceNewConnection, + out openConnection)) { return false; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 8ff3d92428..a70ab35360 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -1969,7 +1969,7 @@ internal override bool TryReplaceConnection( TaskCompletionSource retry, TimeoutTimer timeout) { - return TryOpenConnectionInternal(outerConnection, connectionFactory, retry, timeout); + return TryOpenConnectionInternal(outerConnection, connectionFactory, retry, forceNewConnection: true, timeout); } internal void ValidateConnectionForExecute(SqlCommand command) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 171bff0109..fb4aff3bdf 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1223,8 +1223,6 @@ internal int ReconnectCount get => _reconnectCount; } - internal bool ForceNewConnection { get; set; } - #if NET /// protected override void OnStateChange(StateChangeEventArgs stateChange) @@ -1598,7 +1596,7 @@ public override void Open() => Open(SqlConnectionOverrides.None); private bool TryOpenWithRetry(TaskCompletionSource retry, SqlConnectionOverrides overrides) - => RetryLogicProvider.Execute(this, () => TryOpen(retry, overrides)); + => RetryLogicProvider.Execute(this, () => TryOpen(retry, false, overrides)); /// public void Open(SqlConnectionOverrides overrides) @@ -1618,7 +1616,7 @@ public void Open(SqlConnectionOverrides overrides) { statistics = SqlStatistics.StartTimer(Statistics); - if (!(IsProviderRetriable ? TryOpenWithRetry(null, overrides) : TryOpen(null, overrides))) + if (!(IsProviderRetriable ? TryOpenWithRetry(null, overrides) : TryOpen(null, false, overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -1667,6 +1665,12 @@ private async Task ReconnectAsync(int timeout) { commandTimeoutExpiration = ADP.TimerCurrent() + ADP.TimerFromSeconds(timeout); } + + // Calling close will request a cancellation on this token source, cancelling the reconnection. + // Ideally, however, the reconnect should be directly awaited, making it impossible to have a + // concurrent call to Close. + // TODO: Note that we do not respect the command timeout in this context. Instead, the reconnect gets + // a full connect timeout. Investigate if this should be changed. _reconnectionCancellationSource = cts; CancellationToken ctoken = cts.Token; int retryCount = _connectRetryCount; // take a snapshot: could be changed by modifying the connection string @@ -1684,8 +1688,7 @@ private async Task ReconnectAsync(int timeout) #endif try { - ForceNewConnection = true; - await OpenAsync(ctoken).ConfigureAwait(false); + await ReconnectAsync(ctoken).ConfigureAwait(false); // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. _reconnectCount = unchecked(_reconnectCount + 1); #if DEBUG @@ -1697,7 +1700,6 @@ private async Task ReconnectAsync(int timeout) #if NETFRAMEWORK _impersonateIdentity = null; #endif - ForceNewConnection = false; } SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ReconnectAsync | Info | Reconnection succeeded. Client Connection Id {0} -> {1}", _originalConnectionId, ClientConnectionId); @@ -1907,13 +1909,21 @@ public override Task OpenAsync(CancellationToken cancellationToken) /// public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) => IsProviderRetriable ? - InternalOpenWithRetryAsync(overrides, cancellationToken) : - InternalOpenAsync(overrides, cancellationToken); + InternalOpenWithRetryAsync(overrides, false, cancellationToken) : + InternalOpenAsync(overrides, false, cancellationToken); + + internal Task ReconnectAsync(CancellationToken cancellationToken) + => ReconnectAsync(SqlConnectionOverrides.None, cancellationToken); + + internal Task ReconnectAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) + => IsProviderRetriable ? + InternalOpenWithRetryAsync(overrides, true, cancellationToken) : + InternalOpenAsync(overrides, true, cancellationToken); - private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) - => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, cancellationToken), cancellationToken); + private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) + => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, forceNewConnection, cancellationToken), cancellationToken); - private Task InternalOpenAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) + private Task InternalOpenAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) { long scopeID = SqlClientEventSource.Log.TryPoolerScopeEnterEvent("SqlConnection.InternalOpenAsync | API | Object Id {0}", ObjectID); SqlClientEventSource.Log.TryCorrelationTraceEvent("SqlConnection.InternalOpenAsync | API | Correlation | Object Id {0}, Activity Id {1}", ObjectID, ActivityCorrelator.Current); @@ -1955,7 +1965,7 @@ private Task InternalOpenAsync(SqlConnectionOverrides overrides, CancellationTok try { - completed = TryOpen(completion, overrides); + completed = TryOpen(completion, forceNewConnection, overrides); } catch (Exception e) { @@ -1975,7 +1985,7 @@ private Task InternalOpenAsync(SqlConnectionOverrides overrides, CancellationTok { registration = cancellationToken.Register(s_openAsyncCancel, completion); } - OpenAsyncRetry retry = new OpenAsyncRetry(this, completion, result, overrides, registration); + OpenAsyncRetry retry = new OpenAsyncRetry(this, completion, result, overrides, registration, forceNewConnection); _currentCompletion = new Tuple, Task>(completion, result.Task); completion.Task.ContinueWith(retry.Retry, TaskScheduler.Default); return result.Task; @@ -2088,14 +2098,16 @@ private class OpenAsyncRetry private TaskCompletionSource _result; private SqlConnectionOverrides _overrides; private CancellationTokenRegistration _registration; + private bool _forceNewConnection; - public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, TaskCompletionSource result, SqlConnectionOverrides overrides, CancellationTokenRegistration registration) + public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, TaskCompletionSource result, SqlConnectionOverrides overrides, CancellationTokenRegistration registration, bool forceNewConnection) { _parent = parent; _retry = retry; _result = result; _overrides = overrides; _registration = registration; + _forceNewConnection = false; SqlClientEventSource.Log.TryTraceEvent("SqlConnection.OpenAsyncRetry | Info | Object Id {0}", _parent?.ObjectID); } @@ -2130,7 +2142,7 @@ internal void Retry(Task retryTask) // protect continuation from races with close and cancel lock (_parent.InnerConnection) { - result = _parent.TryOpen(_retry, _overrides); + result = _parent.TryOpen(_retry, _forceNewConnection, _overrides); } if (result) { @@ -2176,7 +2188,7 @@ private void PrepareStatisticsForNewConnection() } } - private bool TryOpen(TaskCompletionSource retry, SqlConnectionOverrides overrides = SqlConnectionOverrides.None) + private bool TryOpen(TaskCompletionSource retry, bool forceNewConnection, SqlConnectionOverrides overrides = SqlConnectionOverrides.None) { bool result = false; @@ -2206,13 +2218,13 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec { if (_impersonateIdentity.User == identity.User) { - result = TryOpenInner(retry); + result = TryOpenInner(retry, forceNewConnection); } else { using (WindowsImpersonationContext context = _impersonateIdentity.Impersonate()) { - result = TryOpenInner(retry); + result = TryOpenInner(retry, forceNewConnection); } } } @@ -2227,10 +2239,10 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec { _lastIdentity = null; } - result = TryOpenInner(retry); + result = TryOpenInner(retry, forceNewConnection); } #else - result = TryOpenInner(retry); + result = TryOpenInner(retry, forceNewConnection); #endif return result; @@ -2240,12 +2252,18 @@ private bool TryOpen(TaskCompletionSource retry, SqlConnec /// Completes the inner open/replace operation and initializes parser state for the active inner connection. /// /// Retry continuation used by async open paths. + /// Provide true to forcibly overwrite the existing connection. Provide false if connecting for the first time. /// when open initialization completed synchronously; otherwise . /// /// The inner connection is snapshotted after the open call so downstream parser access uses a single observed /// instance and does not rely on a second racy read of . + /// + /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never, + /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is + /// currently disconnected. If the connection is currently open, passing false will result in an exception. See SqlConnection state + /// transitions and subclasses for more details. /// - internal bool TryOpenInner(TaskCompletionSource retry) + internal bool TryOpenInner(TaskCompletionSource retry, bool forceNewConnection) { // Create a single TimeoutTimer that represents the overall connection-open budget for this // attempt. The timer is threaded through every layer (inner connection state transitions, @@ -2257,7 +2275,7 @@ internal bool TryOpenInner(TaskCompletionSource retry) TimeoutTimer timeout = TimeoutTimer.StartNew( TimeSpan.FromSeconds(ConnectionOptions?.ConnectTimeout ?? ADP.DefaultConnectionTimeout)); - if (ForceNewConnection) + if (forceNewConnection) { if (!InnerConnection.TryReplaceConnection(this, ConnectionFactory, retry, timeout)) { diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs index dcb1b1c8c2..889a9c014c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs @@ -318,6 +318,7 @@ internal bool TryGetConnection( TaskCompletionSource retry, DbConnectionInternal oldConnection, TimeoutTimer timeout, + bool forceNewConnection, out DbConnectionInternal connection) { Debug.Assert(owningConnection is not null, "null owningConnection?"); @@ -419,7 +420,7 @@ internal bool TryGetConnection( } else { - if (((SqlConnection)owningConnection).ForceNewConnection) + if (forceNewConnection) { Debug.Assert(oldConnection is not DbConnectionClosed, "Force new connection, but there is no old connection"); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs index b1decd2be0..bd47826de5 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs @@ -24,11 +24,10 @@ public class SqlConnectionStateTransitionTests public void TryOpenInner_WhenInnerConnectionChangesAfterTryOpen_ThrowsInvalidOperation() { using SqlConnection connection = new(); - connection.ForceNewConnection = false; bool initialized = connection.SetInnerConnectionFrom(new TestRaceDbConnectionInternal(), DbConnectionClosedNeverOpened.SingletonInstance); Assert.True(initialized); - InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null)); + InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, false)); Assert.NotNull(exception); Assert.Same(DbConnectionClosedPreviouslyOpened.SingletonInstance, connection.InnerConnection); } @@ -40,11 +39,10 @@ public void TryOpenInner_WhenInnerConnectionChangesAfterTryOpen_ThrowsInvalidOpe public void TryOpenInner_WhenInnerConnectionChangesAfterTryReplace_ThrowsInvalidOperation() { using SqlConnection connection = new(); - connection.ForceNewConnection = true; bool initialized = connection.SetInnerConnectionFrom(new TestRaceDbConnectionInternal(), DbConnectionClosedNeverOpened.SingletonInstance); Assert.True(initialized); - InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null)); + InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, true)); Assert.NotNull(exception); Assert.Same(DbConnectionClosedPreviouslyOpened.SingletonInstance, connection.InnerConnection); } From fd6d37071f0155dc3234244c87a0cdcf4151cfce Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 15:30:59 -0700 Subject: [PATCH 03/11] Fix initialization. Add doc comments. --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index fb4aff3bdf..51dc2d9542 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1912,9 +1912,35 @@ public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancel InternalOpenWithRetryAsync(overrides, false, cancellationToken) : InternalOpenAsync(overrides, false, cancellationToken); + /// + /// Re-establishes the underlying physical connection for an already-open , + /// replacing its current (broken) inner connection with a fresh one while preserving the outer + /// object. Used by the connection-resiliency reconnect path. + /// + /// A token that cancels the reconnect attempt. + /// A task that completes when the connection has been re-established. + /// + /// Unlike , this forces a new underlying connection (it must + /// only be called when the connection is already open), so the replacement flows through + /// with forceNewConnection set to . + /// internal Task ReconnectAsync(CancellationToken cancellationToken) => ReconnectAsync(SqlConnectionOverrides.None, cancellationToken); + /// + /// Re-establishes the underlying physical connection for an already-open , + /// replacing its current (broken) inner connection with a fresh one while preserving the outer + /// object. Used by the connection-resiliency reconnect path. + /// + /// Options that relax some of the checks performed when opening the connection. + /// A token that cancels the reconnect attempt. + /// A task that completes when the connection has been re-established. + /// + /// Unlike , this forces a new + /// underlying connection (it must only be called when the connection is already open), so the + /// replacement flows through with forceNewConnection set to + /// . The provider retry logic is applied when . + /// internal Task ReconnectAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) => IsProviderRetriable ? InternalOpenWithRetryAsync(overrides, true, cancellationToken) : @@ -2107,7 +2133,7 @@ public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource Date: Mon, 29 Jun 2026 15:37:17 -0700 Subject: [PATCH 04/11] Remove unnecessary internal API surface. --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 44 ++++--------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 51dc2d9542..419569cbb3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1688,7 +1688,15 @@ private async Task ReconnectAsync(int timeout) #endif try { - await ReconnectAsync(ctoken).ConfigureAwait(false); + if (IsProviderRetriable) + { + await InternalOpenWithRetryAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); + } + else + { + await InternalOpenAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); + } + // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. _reconnectCount = unchecked(_reconnectCount + 1); #if DEBUG @@ -1912,40 +1920,6 @@ public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancel InternalOpenWithRetryAsync(overrides, false, cancellationToken) : InternalOpenAsync(overrides, false, cancellationToken); - /// - /// Re-establishes the underlying physical connection for an already-open , - /// replacing its current (broken) inner connection with a fresh one while preserving the outer - /// object. Used by the connection-resiliency reconnect path. - /// - /// A token that cancels the reconnect attempt. - /// A task that completes when the connection has been re-established. - /// - /// Unlike , this forces a new underlying connection (it must - /// only be called when the connection is already open), so the replacement flows through - /// with forceNewConnection set to . - /// - internal Task ReconnectAsync(CancellationToken cancellationToken) - => ReconnectAsync(SqlConnectionOverrides.None, cancellationToken); - - /// - /// Re-establishes the underlying physical connection for an already-open , - /// replacing its current (broken) inner connection with a fresh one while preserving the outer - /// object. Used by the connection-resiliency reconnect path. - /// - /// Options that relax some of the checks performed when opening the connection. - /// A token that cancels the reconnect attempt. - /// A task that completes when the connection has been re-established. - /// - /// Unlike , this forces a new - /// underlying connection (it must only be called when the connection is already open), so the - /// replacement flows through with forceNewConnection set to - /// . The provider retry logic is applied when . - /// - internal Task ReconnectAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) - => IsProviderRetriable ? - InternalOpenWithRetryAsync(overrides, true, cancellationToken) : - InternalOpenAsync(overrides, true, cancellationToken); - private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, forceNewConnection, cancellationToken), cancellationToken); From a6ce9928ca105bba03dee92ac99091f5205724fd Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 15:51:09 -0700 Subject: [PATCH 05/11] Expose param. --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 419569cbb3..868ef6bc4c 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1595,8 +1595,8 @@ public override void EnlistTransaction(System.Transactions.Transaction transacti public override void Open() => Open(SqlConnectionOverrides.None); - private bool TryOpenWithRetry(TaskCompletionSource retry, SqlConnectionOverrides overrides) - => RetryLogicProvider.Execute(this, () => TryOpen(retry, false, overrides)); + private bool TryOpenWithRetry(TaskCompletionSource retry, bool forceNewConnection,SqlConnectionOverrides overrides) + => RetryLogicProvider.Execute(this, () => TryOpen(retry, forceNewConnection, overrides)); /// public void Open(SqlConnectionOverrides overrides) @@ -1616,7 +1616,7 @@ public void Open(SqlConnectionOverrides overrides) { statistics = SqlStatistics.StartTimer(Statistics); - if (!(IsProviderRetriable ? TryOpenWithRetry(null, overrides) : TryOpen(null, false, overrides))) + if (!(IsProviderRetriable ? TryOpenWithRetry(null, false, overrides) : TryOpen(null, false, overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -1696,7 +1696,7 @@ private async Task ReconnectAsync(int timeout) { await InternalOpenAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); } - + // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. _reconnectCount = unchecked(_reconnectCount + 1); #if DEBUG From c8334df5cd5554b5c26821f0adba49a3408d7108 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 16:08:21 -0700 Subject: [PATCH 06/11] Address broken test and copilot comments. --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 6 +++--- .../SqlConnectionConcurrentOpenTests.cs | 18 +----------------- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 868ef6bc4c..ff2165d64a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1666,7 +1666,7 @@ private async Task ReconnectAsync(int timeout) commandTimeoutExpiration = ADP.TimerCurrent() + ADP.TimerFromSeconds(timeout); } - // Calling close will request a cancellation on this token source, cancelling the reconnection. + // Calling Close will request a cancellation on this token source, cancelling the reconnection. // Ideally, however, the reconnect should be directly awaited, making it impossible to have a // concurrent call to Close. // TODO: Note that we do not respect the command timeout in this context. Instead, the reconnect gets @@ -2258,8 +2258,8 @@ private bool TryOpen(TaskCompletionSource retry, bool forc /// The inner connection is snapshotted after the open call so downstream parser access uses a single observed /// instance and does not rely on a second racy read of . /// - /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never, - /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is + /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never + /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is /// currently disconnected. If the connection is currently open, passing false will result in an exception. See SqlConnection state /// transitions and subclasses for more details. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs index 8d151e61cd..0ad965913e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs @@ -4,7 +4,6 @@ using System; using System.Data; -using System.Reflection; using System.Threading.Tasks; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.Connection; @@ -26,9 +25,6 @@ namespace Microsoft.Data.SqlClient.UnitTests.Microsoft.Data.SqlClient /// public class SqlConnectionConcurrentOpenTests { - private static readonly MethodInfo s_tryOpenInner = typeof(SqlConnection) - .GetMethod("TryOpenInner", BindingFlags.Instance | BindingFlags.NonPublic)!; - private static DbConnectionInternal GetConnectingSingleton() { return DbConnectionClosedConnecting.SingletonInstance; @@ -43,18 +39,6 @@ private static void ForceInnerConnection(SqlConnection connection, DbConnectionI Assert.True(swapped, $"CAS failed: expected current state {current.GetType().Name} but it changed concurrently."); } - private static bool InvokeTryOpenInner(SqlConnection connection, TaskCompletionSource retry) - { - try - { - return (bool)s_tryOpenInner.Invoke(connection, [retry])!; - } - catch (TargetInvocationException tie) when (tie.InnerException != null) - { - throw tie.InnerException; - } - } - [Fact] public void InnerConnection_DbConnectionClosedConnecting_IsNotAssignableToSqlConnectionInternal() { @@ -102,7 +86,7 @@ public void TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState Exception ex = Assert.ThrowsAny(() => { - InvokeTryOpenInner(connection, completedRetry); + connection.TryOpenInner(completedRetry, false); }); Assert.True( From 5815676210a7cddb7efa5acc0cfea159cf2dff5d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 16:12:23 -0700 Subject: [PATCH 07/11] WIP --- .../Data/ProviderBase/DbConnectionClosed.cs | 2 +- .../ConnectionPool/ChannelDbConnectionPool.cs | 107 +++- .../ConnectionPool/ConnectionPoolSlots.cs | 20 + .../ConnectionPool/IDbConnectionPool.cs | 2 +- ...elDbConnectionPoolReplaceConnectionTest.cs | 502 ++++++++++++++++++ .../ChannelDbConnectionPoolTest.cs | 14 +- 6 files changed, 639 insertions(+), 8 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs index 7d9c865e0a..37782dddf3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs @@ -64,7 +64,7 @@ internal override bool TryOpenConnection( SqlConnectionFactory connectionFactory, TaskCompletionSource retry, TimeoutTimer timeout) => - TryOpenConnectionInternal(outerConnection, connectionFactory, retry, forceNewConnection: false,timeout); + TryOpenConnectionInternal(outerConnection, connectionFactory, retry, forceNewConnection: false, timeout); /// internal override void ResetConnection() => throw ADP.ClosedConnectionError(); 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 9169e9717e..584580377b 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 @@ -231,12 +231,109 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) } /// - public DbConnectionInternal ReplaceConnection( + public DbConnectionInternal? ReplaceConnection( DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout) { - throw new NotImplementedException(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, replacing connection.", Id); + + // Acquire a replacement that reuses the broken connection's pool slot so capacity is preserved + // (FR-005). Prefers an idle connection (Story 6); otherwise establishes a new one. Throws (after + // disposing the old connection and freeing its slot) if establishing a new connection fails (FR-006). + DbConnectionInternal? newConnection = GetReplacementConnection(owningObject, oldConnection, timeout); + if (newConnection is null) + { + // No slot could be secured for a new connection; let the caller's retry logic handle it. + return null; + } + + // Activate the replacement under the old connection's ambient transaction (FR-002/FR-003). If + // activation fails, PrepareConnection returns the new connection to the pool and rethrows (FR-007). + // TODO: Full transaction enlistment support (Story 2). + PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); + + // FR-004: Deactivate and dispose the old connection now that the replacement is active. + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); + + // FR-008: Record a soft connect metric since the caller's connection object is reused. + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, connection replaced successfully.", Id); + + return newConnection; + } + + /// + /// Obtains a connection to replace , reusing its pool slot so the + /// pool's connection count is unchanged (FR-005). An idle connection is preferred (Story 6); + /// otherwise a new physical connection is established. + /// + /// The connection that will own the replacement. + /// The broken connection being replaced. + /// The overall timeout budget for establishing a new connection. + /// + /// The replacement connection, already placed in a pool slot, or if a new + /// connection was established but no slot could be secured for it. + /// + /// + /// Propagated when establishing a new connection fails. Before rethrowing, the old connection is + /// deactivated and removed from the pool so its slot is not leaked (FR-006 / Story 4). + /// + private DbConnectionInternal? GetReplacementConnection( + DbConnection owningObject, + DbConnectionInternal oldConnection, + TimeoutTimer timeout) + { + // Story 6: prefer an idle connection over establishing a new physical one. + DbConnectionInternal? idleConnection = GetIdleConnection(); + if (idleConnection is not null) + { + // The idle connection already holds its own slot, so just release the broken connection's + // slot to keep the pool count correct (FR-005 / Story 6). + _connectionSlots.TryRemove(oldConnection); + return idleConnection; + } + + DbConnectionInternal newConnection; + try + { + newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + } + catch + { + // FR-006 / Story 4: Establishing the replacement failed (e.g., the server is still + // unreachable). Deactivate and remove the broken connection so its slot is not leaked, then + // propagate. RemoveConnection disposes it and clears its Pool reference, so the caller's + // eventual Close() follows the non-pooled path and will not touch the freed slot again. + oldConnection.DeactivateConnection(); + RemoveConnection(oldConnection); + throw; + } + + // Stamp the new connection with the current generation so a later Clear() can discard it. (Idle + // connections already carry their birth generation, so only newly established ones are stamped.) + newConnection.ClearGeneration = _clearGeneration; + + // Atomically swap the new connection into the broken connection's slot so the reservation count + // is unchanged (FR-005). + if (!_connectionSlots.TryReplace(oldConnection, newConnection)) + { + // The old slot was already removed (e.g., by a concurrent Clear). Reserve a fresh slot + // instead; if the pool is now full, give up and dispose the orphaned connection. + if (_connectionSlots.Add( + createCallback: () => newConnection, + cleanupCallback: (conn) => conn?.Dispose()) is null) + { + newConnection.Dispose(); + return null; + } + } + + return newConnection; } /// @@ -664,10 +761,11 @@ private async Task GetInternalConnection( /// /// The owning DbConnection instance. /// The DbConnectionInternal to be activated. + /// The transaction to enlist the connection in, or null to activate cleanly. /// /// Thrown when any exception occurs during connection activation. /// - private void PrepareConnection(DbConnection owningObject, DbConnectionInternal connection) + private void PrepareConnection(DbConnection owningObject, DbConnectionInternal connection, Transaction? transaction = null) { lock (connection) { @@ -677,8 +775,7 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c try { - //TODO: pass through transaction - connection.ActivateConnection(null); + connection.ActivateConnection(transaction); } catch { 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 55eb88f02c..c9d268fd29 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 @@ -170,6 +170,26 @@ internal bool TryRemove(DbConnectionInternal connection) return false; } + /// + /// Atomically replaces an existing connection with a new one in the same slot. + /// The reservation count is unchanged because the slot is reused. + /// + /// The connection currently occupying the slot. + /// The connection to place into the slot. + /// True if the old connection was found and replaced; otherwise, false. + internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInternal newConnection) + { + for (int i = 0; i < _connections.Length; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], newConnection, oldConnection) == oldConnection) + { + return true; + } + } + + return false; + } + /// /// Attempts to reserve a spot in the collection. /// 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 2ae8e8ea98..3cec1f2bc8 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 @@ -132,7 +132,7 @@ internal interface IDbConnectionPool /// The internal connection currently associated with the owning object. /// The overall timeout budget for this connection request. /// A reference to the new DbConnectionInternal. - DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); + DbConnectionInternal? ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); /// /// Returns an internal connection to the pool. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs new file mode 100644 index 0000000000..44015a76ea --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -0,0 +1,502 @@ +// 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.Data.Common; +using System.Threading; +using System.Transactions; +using Microsoft.Data.Common; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Data.SqlClient.Tests.Common; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + public class ChannelDbConnectionPoolReplaceConnectionTest + { + private static readonly SqlConnectionFactory SuccessfulConnectionFactory = new SuccessfulSqlConnectionFactory(); + + private ChannelDbConnectionPool ConstructPool( + SqlConnectionFactory connectionFactory, + DbConnectionPoolGroupOptions? poolGroupOptions = null) + { + poolGroupOptions ??= new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 50, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + return new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo() + ); + } + + #region Story 1 — Transparent Replacement + + [Fact] + public void ReplaceConnection_ReturnsNewConnection() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Act + var newConnection = pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); + } + + [Fact] + public void ReplaceConnection_OldConnectionIsDisposed() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Act + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — the old connection should be disposed (not poolable) + Assert.False(oldConnection.CanBePooled); + } + + #endregion + + #region Story 3 — Pool Capacity Preservation (new physical connection path) + + [Fact] + public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() + { + // Arrange — single connection, no idle connections available + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + Assert.Equal(0, pool.IdleCount); + int countBefore = pool.Count; + + // Act + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — slot was reused, count unchanged + Assert.Equal(countBefore, pool.Count); + } + + [Fact] + public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() + { + // Arrange — fill pool to max capacity, no idle connections + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 3, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions); + + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + SqlConnection owner3 = new(); + + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + pool.TryGetConnection(owner3, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn3); + + Assert.Equal(3, pool.Count); + + // Act — replace connection in a full pool + var newConnection = pool.ReplaceConnection( + owner1, + conn1!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — pool count must not exceed max + Assert.NotNull(newConnection); + Assert.Equal(3, pool.Count); + } + + #endregion + + #region Story 4 — Replacement Failure Propagation + + [Fact] + public void ReplaceConnection_CreationFails_ExceptionPropagated() + { + // Arrange — use a factory that succeeds initially then fails + var switchableFactory = new SwitchableSqlConnectionFactory(); + var pool = ConstructPool(switchableFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Switch to failing mode + switchableFactory.ShouldFail = true; + + // Act & Assert — exception from factory is propagated + Assert.Throws(() => + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + } + + [Fact] + public void ReplaceConnection_CreationFails_OldSlotReleased() + { + // Arrange — fill the pool to capacity so a leaked slot would be observable. + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 2, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var switchableFactory = new SwitchableSqlConnectionFactory(); + var pool = ConstructPool(switchableFactory, poolGroupOptions); + + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? oldConnection); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? otherConnection); + + Assert.NotNull(oldConnection); + Assert.Equal(2, pool.Count); + + // Switch to failing mode so the replacement creation throws. + switchableFactory.ShouldFail = true; + + // Act — replacement fails + Assert.Throws(() => + pool.ReplaceConnection( + owner1, + oldConnection!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // Assert — FR-006: the old connection's slot was released (no capacity leak). + Assert.Equal(1, pool.Count); + // Old connection was disposed as part of the failure cleanup. + Assert.False(oldConnection!.CanBePooled); + // FR-006 scenario 3: the pool is not left in an error state. + Assert.False(pool.ErrorOccurred); + + // The freed slot is usable again — a fresh request can succeed. + switchableFactory.ShouldFail = false; + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? replacementForSlot); + Assert.NotNull(replacementForSlot); + Assert.Equal(2, pool.Count); + } + + #endregion + + #region Story 5 — Activation Failure Rollback + + [Fact] + public void ReplaceConnection_ActivationFails_ExceptionPropagated() + { + // Arrange + var factory = new ActivationFailSqlConnectionFactory(); + var pool = ConstructPool(factory); + SqlConnection owner = new(); + + factory.FailOnActivate = false; + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Now make activation fail for the replacement + factory.FailOnActivate = true; + + // Act & Assert + Assert.Throws(() => + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + } + + [Fact] + public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() + { + // Arrange + var factory = new ActivationFailSqlConnectionFactory(); + var pool = ConstructPool(factory); + SqlConnection owner = new(); + + factory.FailOnActivate = false; + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + int countBefore = pool.Count; + + // Make activation fail + factory.FailOnActivate = true; + + // Act + try + { + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + } + catch (InvalidOperationException) + { + // Expected + } + + // Assert — the new connection was returned to pool (not leaked). + // Pool count stays same because the new connection replaced the old one's slot + // and was then returned to idle. + Assert.Equal(countBefore, pool.Count); + } + + #endregion + + #region Story 6 — Prefer Idle Connection + + [Fact] + public void ReplaceConnection_PrefersIdleOverNewConnection() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + + Assert.NotNull(conn1); + Assert.NotNull(conn2); + + // Return conn2 to make it idle + pool.ReturnInternalConnection(conn2, owner2); + Assert.Equal(1, pool.IdleCount); + + // Act — replace conn1; should prefer the idle conn2 + var newConnection = pool.ReplaceConnection( + owner1, + conn1, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — should reuse the idle connection + Assert.NotNull(newConnection); + Assert.Same(conn2, newConnection); + Assert.Equal(0, pool.IdleCount); + } + + [Fact] + public void ReplaceConnection_NoIdleConnection_CreatesNew() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + Assert.NotNull(conn1); + Assert.Equal(0, pool.IdleCount); + + // Act — no idle connections available, should create new + var newConnection = pool.ReplaceConnection( + owner, + conn1, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert + Assert.NotNull(newConnection); + Assert.NotSame(conn1, newConnection); + Assert.Equal(1, pool.Count); + } + + #endregion + + #region Test Helper Classes + + internal class SuccessfulSqlConnectionFactory : SqlConnectionFactory + { + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new StubDbConnectionInternal(); + } + } + + internal class SwitchableSqlConnectionFactory : SqlConnectionFactory + { + internal bool ShouldFail { get; set; } + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (ShouldFail) + { + throw new InvalidOperationException("Simulated connection failure"); + } + return new StubDbConnectionInternal(); + } + } + + internal class ActivationFailSqlConnectionFactory : SqlConnectionFactory + { + internal bool FailOnActivate { get; set; } + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new ActivationFailDbConnectionInternal(this); + } + } + + internal class StubDbConnectionInternal : DbConnectionInternal + { + public override string ServerVersion => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction transaction) + { + return; + } + + protected override void Activate(Transaction transaction) + { + return; + } + + protected override void Deactivate() + { + return; + } + + internal override void ResetConnection() + { + return; + } + } + + internal class ActivationFailDbConnectionInternal : DbConnectionInternal + { + private readonly ActivationFailSqlConnectionFactory _factory; + + internal ActivationFailDbConnectionInternal(ActivationFailSqlConnectionFactory factory) + { + _factory = factory; + } + + public override string ServerVersion => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction transaction) + { + return; + } + + protected override void Activate(Transaction transaction) + { + if (_factory.FailOnActivate) + { + throw new InvalidOperationException("Simulated activation failure"); + } + } + + protected override void Deactivate() + { + return; + } + + internal override void ResetConnection() + { + return; + } + } + + #endregion + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index b79baefbd5..ded7aeda35 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -705,7 +705,19 @@ public void TestPutObjectFromTransactedPool() public void TestReplaceConnection() { var pool = ConstructPool(SuccessfulConnectionFactory); - Assert.Throws(() => pool.ReplaceConnection(null!, null!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + var newConnection = pool.ReplaceConnection(owner, oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); } [Fact] From 0487b8eb01caf498699b7d7a50eb7fb267fa6614 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 7 Jul 2026 18:46:17 -0700 Subject: [PATCH 08/11] Add unit tests. --- ...elDbConnectionPoolReplaceConnectionTest.cs | 41 +++++ .../ConnectionPool/ConnectionPoolSlotsTest.cs | 151 ++++++++++++++++++ 2 files changed, 192 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 44015a76ea..3b66813339 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -47,6 +47,10 @@ private ChannelDbConnectionPool ConstructPool( #region Story 1 — Transparent Replacement + /// + /// Verifies that returns a + /// non-null connection that is a different instance from the one being replaced. + /// [Fact] public void ReplaceConnection_ReturnsNewConnection() { @@ -73,6 +77,10 @@ public void ReplaceConnection_ReturnsNewConnection() Assert.NotSame(oldConnection, newConnection); } + /// + /// Verifies that after a replacement the old connection is disposed and can no longer + /// be pooled. + /// [Fact] public void ReplaceConnection_OldConnectionIsDisposed() { @@ -102,6 +110,10 @@ public void ReplaceConnection_OldConnectionIsDisposed() #region Story 3 — Pool Capacity Preservation (new physical connection path) + /// + /// Verifies that replacing a connection when no idle connections are available reuses + /// the old connection's slot so the pool's total count remains unchanged. + /// [Fact] public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() { @@ -129,6 +141,10 @@ public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() Assert.Equal(countBefore, pool.Count); } + /// + /// Verifies that replacing a connection in a pool that is already filled to its maximum + /// capacity succeeds without exceeding the maximum pool size. + /// [Fact] public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() { @@ -169,6 +185,10 @@ public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() #region Story 4 — Replacement Failure Propagation + /// + /// Verifies that when creating the replacement connection fails, the exception thrown by + /// the connection factory is propagated to the caller. + /// [Fact] public void ReplaceConnection_CreationFails_ExceptionPropagated() { @@ -196,6 +216,11 @@ public void ReplaceConnection_CreationFails_ExceptionPropagated() TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); } + /// + /// Verifies FR-006: when creating the replacement connection fails, the old connection's + /// slot is released (no capacity leak), the old connection is disposed, the pool is not + /// left in an error state, and the freed slot can be reused by a subsequent request. + /// [Fact] public void ReplaceConnection_CreationFails_OldSlotReleased() { @@ -248,6 +273,10 @@ public void ReplaceConnection_CreationFails_OldSlotReleased() #region Story 5 — Activation Failure Rollback + /// + /// Verifies that when activating the replacement connection fails, the exception is + /// propagated to the caller. + /// [Fact] public void ReplaceConnection_ActivationFails_ExceptionPropagated() { @@ -276,6 +305,10 @@ public void ReplaceConnection_ActivationFails_ExceptionPropagated() TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); } + /// + /// Verifies that when activating the replacement connection fails, the newly created + /// connection is returned to the pool rather than leaked, keeping the pool count stable. + /// [Fact] public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() { @@ -320,6 +353,10 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() #region Story 6 — Prefer Idle Connection + /// + /// Verifies that when an idle connection is available, replacement reuses that idle + /// connection instead of creating a new physical connection. + /// [Fact] public void ReplaceConnection_PrefersIdleOverNewConnection() { @@ -350,6 +387,10 @@ public void ReplaceConnection_PrefersIdleOverNewConnection() Assert.Equal(0, pool.IdleCount); } + /// + /// Verifies that when no idle connection is available, replacement creates a new + /// physical connection distinct from the one being replaced. + /// [Fact] public void ReplaceConnection_NoIdleConnection_CreatesNew() { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs index c4f437d981..7636096543 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs @@ -481,5 +481,156 @@ public void Constructor_EdgeCase_CapacityOfOne_WorksCorrectly() Assert.Null(connection2); Assert.Equal(1, poolSlots.ReservationCount); } + + /// + /// Verifies that replacing an existing connection returns and leaves + /// the reservation count unchanged, since the replacement reuses the same slot. + /// + [Fact] + public void TryReplace_ExistingConnection_ReturnsTrueAndKeepsReservationCount() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + var reservationCountBeforeReplace = poolSlots.ReservationCount; + + // Act + var replaced = poolSlots.TryReplace(oldConnection!, newConnection); + + // Assert - the slot is reused, so the reservation count is unchanged + Assert.True(replaced); + Assert.Equal(1, reservationCountBeforeReplace); + Assert.Equal(1, poolSlots.ReservationCount); + } + + /// + /// Verifies that after a successful replace, the new connection occupies the slot (and can + /// be removed) while the old connection is no longer present in the collection. + /// + [Fact] + public void TryReplace_ExistingConnection_NewConnectionOccupiesSlot() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + + // Act + poolSlots.TryReplace(oldConnection!, newConnection); + + // Assert - the new connection now occupies the slot and can be removed, + // while the old connection is no longer present. + Assert.False(poolSlots.TryRemove(oldConnection!)); + Assert.True(poolSlots.TryRemove(newConnection)); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// Verifies that attempting to replace a connection that is not in the collection returns + /// , does not change the reservation count, and does not insert the + /// new connection. + /// + [Fact] + public void TryReplace_NonExistentConnection_ReturnsFalseAndDoesNotAddNewConnection() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var existingConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var missingConnection = new MockDbConnectionInternal(); + var newConnection = new MockDbConnectionInternal(); + var reservationCountBeforeReplace = poolSlots.ReservationCount; + + // Act + var replaced = poolSlots.TryReplace(missingConnection, newConnection); + + // Assert - nothing was replaced and the new connection was not inserted + Assert.False(replaced); + Assert.Equal(1, reservationCountBeforeReplace); + Assert.Equal(1, poolSlots.ReservationCount); + Assert.False(poolSlots.TryRemove(newConnection)); + } + + /// + /// Verifies that replacing a connection in an empty collection returns + /// and leaves the reservation count at zero. + /// + [Fact] + public void TryReplace_EmptyCollection_ReturnsFalse() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = new MockDbConnectionInternal(); + var newConnection = new MockDbConnectionInternal(); + + // Act + var replaced = poolSlots.TryReplace(oldConnection, newConnection); + + // Assert + Assert.False(replaced); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// Verifies that when multiple connections are present, replace swaps only the targeted + /// connection and leaves the others untouched. + /// + [Fact] + public void TryReplace_MultipleConnections_ReplacesOnlyTargetConnection() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var connection1 = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var connection2 = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + + // Act - replace only connection2 + var replaced = poolSlots.TryReplace(connection2!, newConnection); + + // Assert - the untouched connection remains, the target was swapped out + Assert.True(replaced); + Assert.Equal(2, poolSlots.ReservationCount); + Assert.True(poolSlots.TryRemove(connection1!)); + Assert.False(poolSlots.TryRemove(connection2!)); + Assert.True(poolSlots.TryRemove(newConnection)); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// Verifies that replacing the same connection twice succeeds on the first attempt but + /// fails on the second, because the original connection is no longer in the slot. + /// + [Fact] + public void TryReplace_SameConnectionTwice_ReturnsFalseOnSecondAttempt() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + var newerConnection = new MockDbConnectionInternal(); + + // Act + var firstReplace = poolSlots.TryReplace(oldConnection!, newConnection); + var secondReplace = poolSlots.TryReplace(oldConnection!, newerConnection); + + // Assert - the old connection is gone after the first replace, so the second fails + Assert.True(firstReplace); + Assert.False(secondReplace); + Assert.Equal(1, poolSlots.ReservationCount); + Assert.True(poolSlots.TryRemove(newConnection)); + Assert.False(poolSlots.TryRemove(newerConnection)); + } } } From 2f880ce9b084bc1d66f8dd1594711132f1fa299f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 7 Jul 2026 18:50:11 -0700 Subject: [PATCH 09/11] Wording --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 2 +- 1 file changed, 1 insertion(+), 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 4f531d2af7..7919236c8b 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 @@ -298,7 +298,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) DbConnectionInternal? idleConnection = GetIdleConnection(); if (idleConnection is not null) { - // The idle connection already holds its own slot, so just release the broken connection's + // The idle connection already holds its own slot, so release the broken connection's // slot to keep the pool count correct (FR-005 / Story 6). _connectionSlots.TryRemove(oldConnection); return idleConnection; From 7cff4297960ad43f2d15f7200659af54d8d93edb Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 9 Jul 2026 16:50:31 -0700 Subject: [PATCH 10/11] improve error handling --- .../ConnectionPool/ChannelDbConnectionPool.cs | 121 ++++++------------ ...elDbConnectionPoolReplaceConnectionTest.cs | 69 ++++------ 2 files changed, 62 insertions(+), 128 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 7919236c8b..3a4dab7014 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 @@ -245,100 +245,59 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); - // Acquire a replacement that reuses the broken connection's pool slot so capacity is preserved - // (FR-005). Prefers an idle connection (Story 6); otherwise establishes a new one. Throws (after - // disposing the old connection and freeing its slot) if establishing a new connection fails (FR-006). - DbConnectionInternal? newConnection = GetReplacementConnection(owningObject, oldConnection, timeout); - if (newConnection is null) - { - // No slot could be secured for a new connection; let the caller's retry logic handle it. - return null; - } - - // Activate the replacement under the old connection's ambient transaction (FR-002/FR-003). If - // activation fails, PrepareConnection returns the new connection to the pool and rethrows (FR-007). - // TODO: Full transaction enlistment support (Story 2). - PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); - - // FR-004: Deactivate and dispose the old connection now that the replacement is active. - oldConnection.DeactivateConnection(); - oldConnection.Dispose(); + // We replace oldConnection with a brand-new physical connection. oldConnection is left completely + // untouched until the replacement has been successfully activated, so any failure leaves it reusable + // by the caller's reconnect retry loop (SqlConnection.ReconnectAsync). oldConnection is checked out, + // so its pool slot is stable: nothing else can remove or steal it (Clear/Shutdown/Prune only drain + // idle connections). We only touch the slots once the replacement is live. While the new connection + // is activating the pool may briefly hold one physical connection over MaxPoolSize (the dead old one + // plus the new one), but oldConnection is dead anyway and the reserved slot count never exceeds the + // maximum. + // TODO: Prefer reusing an idle connection before establishing a new one + DbConnectionInternal newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); - // FR-008: Record a soft connect metric since the caller's connection object is reused. - SqlClientDiagnostics.Metrics.SoftConnectRequest(); + try + { + // Stamp with the current generation so a later Clear() can discard it. + newConnection.ClearGeneration = _clearGeneration; - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, connection replaced successfully.", Id); + lock (newConnection) + { + // PostPop requires a lock on the connection. + newConnection.PostPop(owningObject); + } - return newConnection; - } + // Activate under the old connection's ambient transaction (FR-002/FR-003). + // TODO: Full transaction enlistment support (Story 2). + newConnection.ActivateConnection(oldConnection.EnlistedTransaction); - /// - /// Obtains a connection to replace , reusing its pool slot so the - /// pool's connection count is unchanged (FR-005). An idle connection is preferred (Story 6); - /// otherwise a new physical connection is established. - /// - /// The connection that will own the replacement. - /// The broken connection being replaced. - /// The overall timeout budget for establishing a new connection. - /// - /// The replacement connection, already placed in a pool slot, or if a new - /// connection was established but no slot could be secured for it. - /// - /// - /// Propagated when establishing a new connection fails. Before rethrowing, the old connection is - /// deactivated and removed from the pool so its slot is not leaked (FR-006 / Story 4). - /// - private DbConnectionInternal? GetReplacementConnection( - DbConnection owningObject, - DbConnectionInternal oldConnection, - TimeoutTimer timeout) - { - // Story 6: prefer an idle connection over establishing a new physical one. - DbConnectionInternal? idleConnection = GetIdleConnection(); - if (idleConnection is not null) - { - // The idle connection already holds its own slot, so release the broken connection's - // slot to keep the pool count correct (FR-005 / Story 6). - _connectionSlots.TryRemove(oldConnection); - return idleConnection; - } + bool replaced = _connectionSlots.TryReplace(oldConnection, newConnection); - DbConnectionInternal newConnection; - try - { - newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + if (!replaced) + { + // This should never happen because oldConnection is checked out and its slot is stable. + // Still, we need to check or we could vend a connection that is not tracked by the pool. + // TODO: error types and localization + throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced."); + } } catch { - // FR-006 / Story 4: Establishing the replacement failed (e.g., the server is still - // unreachable). Deactivate and remove the broken connection so its slot is not leaked, then - // propagate. RemoveConnection disposes it and clears its Pool reference, so the caller's - // eventual Close() follows the non-pooled path and will not touch the freed slot again. - oldConnection.DeactivateConnection(); - RemoveConnection(oldConnection); + // Dispose the brand-new connection; it never took a slot. oldConnection is untouched. + newConnection.DeactivateConnection(); + newConnection.Dispose(); throw; } - // Stamp the new connection with the current generation so a later Clear() can discard it. (Idle - // connections already carry their birth generation, so only newly established ones are stamped.) - newConnection.ClearGeneration = _clearGeneration; + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); - // Atomically swap the new connection into the broken connection's slot so the reservation count - // is unchanged (FR-005). - if (!_connectionSlots.TryReplace(oldConnection, newConnection)) - { - // The old slot was already removed (e.g., by a concurrent Clear). Reserve a fresh slot - // instead; if the pool is now full, give up and dispose the orphaned connection. - if (_connectionSlots.Add( - createCallback: () => newConnection, - cleanupCallback: (conn) => conn?.Dispose()) is null) - { - newConnection.Dispose(); - return null; - } - } + // A hard connect is already recorded by the connection factory. + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, connection replaced successfully.", Id); + return newConnection; } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 3b66813339..0228d6f2db 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -217,14 +217,14 @@ public void ReplaceConnection_CreationFails_ExceptionPropagated() } /// - /// Verifies FR-006: when creating the replacement connection fails, the old connection's - /// slot is released (no capacity leak), the old connection is disposed, the pool is not - /// left in an error state, and the freed slot can be reused by a subsequent request. + /// Verifies that when creating the replacement connection fails, the old connection is left fully + /// intact - it keeps its pool slot and stays poolable - so the caller's reconnect retry loop can reuse + /// it on a subsequent attempt. The pool count is unchanged and the pool is not left in an error state. /// [Fact] - public void ReplaceConnection_CreationFails_OldSlotReleased() + public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() { - // Arrange — fill the pool to capacity so a leaked slot would be observable. + // Arrange — fill the pool to capacity so a leaked or prematurely released slot would be observable. var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -255,17 +255,23 @@ public void ReplaceConnection_CreationFails_OldSlotReleased() oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); - // Assert — FR-006: the old connection's slot was released (no capacity leak). - Assert.Equal(1, pool.Count); - // Old connection was disposed as part of the failure cleanup. - Assert.False(oldConnection!.CanBePooled); - // FR-006 scenario 3: the pool is not left in an error state. + // Assert — the old connection is left intact so the caller can retry with it: its slot is retained + // (no premature release) ... + Assert.Equal(2, pool.Count); + // ... it is not disposed and remains poolable ... + Assert.True(oldConnection!.CanBePooled); + // ... and the pool is not left in an error state. Assert.False(pool.ErrorOccurred); - // The freed slot is usable again — a fresh request can succeed. + // The reconnect retry loop reuses the SAME old connection: a subsequent successful replacement + // succeeds, reusing the retained slot and keeping the pool count unchanged. switchableFactory.ShouldFail = false; - pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? replacementForSlot); - Assert.NotNull(replacementForSlot); + var newConnection = pool.ReplaceConnection( + owner1, + oldConnection!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); Assert.Equal(2, pool.Count); } @@ -351,41 +357,10 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() #endregion - #region Story 6 — Prefer Idle Connection - - /// - /// Verifies that when an idle connection is available, replacement reuses that idle - /// connection instead of creating a new physical connection. - /// - [Fact] - public void ReplaceConnection_PrefersIdleOverNewConnection() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - SqlConnection owner1 = new(); - SqlConnection owner2 = new(); - - pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); - pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); - - Assert.NotNull(conn1); - Assert.NotNull(conn2); - - // Return conn2 to make it idle - pool.ReturnInternalConnection(conn2, owner2); - Assert.Equal(1, pool.IdleCount); + #region Story 6 — New Physical Connection - // Act — replace conn1; should prefer the idle conn2 - var newConnection = pool.ReplaceConnection( - owner1, - conn1, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); - - // Assert — should reuse the idle connection - Assert.NotNull(newConnection); - Assert.Same(conn2, newConnection); - Assert.Equal(0, pool.IdleCount); - } + // NOTE: Preferring an idle connection over a new physical one is being added in a follow-up PR + // (branch dev/automation/replace-conn-idle-path), along with its ReplaceConnection_PrefersIdleOverNewConnection test. /// /// Verifies that when no idle connection is available, replacement creates a new From 0fc0c44eeac3e87929c1d068b3ee06368833a791 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 9 Jul 2026 17:01:54 -0700 Subject: [PATCH 11/11] Address copilot comments. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 4 ++-- .../Data/SqlClient/ConnectionPool/IDbConnectionPool.cs | 2 +- .../ChannelDbConnectionPoolReplaceConnectionTest.cs | 10 ++++++---- 3 files changed, 9 insertions(+), 7 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 3a4dab7014..3c33b4d625 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 @@ -237,7 +237,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) } /// - public DbConnectionInternal? ReplaceConnection( + public DbConnectionInternal ReplaceConnection( DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout) @@ -297,7 +297,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, connection replaced successfully.", Id); - + return newConnection; } 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 3cec1f2bc8..2ae8e8ea98 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 @@ -132,7 +132,7 @@ internal interface IDbConnectionPool /// The internal connection currently associated with the owning object. /// The overall timeout budget for this connection request. /// A reference to the new DbConnectionInternal. - DbConnectionInternal? ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); + DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); /// /// Returns an internal connection to the pool. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 0228d6f2db..8ab4d12a9f 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -4,13 +4,11 @@ using System; using System.Data.Common; -using System.Threading; using System.Transactions; using Microsoft.Data.Common; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; -using Microsoft.Data.SqlClient.Tests.Common; using Xunit; namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool @@ -258,8 +256,12 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() // Assert — the old connection is left intact so the caller can retry with it: its slot is retained // (no premature release) ... Assert.Equal(2, pool.Count); - // ... it is not disposed and remains poolable ... - Assert.True(oldConnection!.CanBePooled); + // ... it is not doomed, so it remains usable for the retry ... + Assert.False(oldConnection!.IsConnectionDoomed); + // ... it is still owned by the same caller (not released back to the pool) ... + Assert.Same(owner1, oldConnection!.Owner); + // ... it keeps its reference to the pool, which is what enables the caller's retry ... + Assert.Same(pool, oldConnection!.Pool); // ... and the pool is not left in an error state. Assert.False(pool.ErrorOccurred);