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 0b5becc0fa..3c6347da54 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 @@ -322,7 +322,99 @@ public DbConnectionInternal ReplaceConnection( DbConnectionInternal oldConnection, TimeoutTimer timeout) { - throw new NotImplementedException(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, replacing connection.", Id); + + // First, prefer to get an idle connection from the pool. + // If one is available, we can avoid the cost of creating a new connection. + DbConnectionInternal? newConnection = GetIdleConnection(); + + if (newConnection is not null) + { + // TODO: Full transaction enlistment support (Story 2). + PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); + oldConnection.DeactivateConnection(); + RemoveConnection(oldConnection); + } + else + { + _errorState?.ThrowIfActive(); + + // Unlike OpenNewInternalConnection, this direct create intentionally bypasses + // _connectionCreationRateLimiter. This mirrors the behavior in WaitHandleDbConnectionPool.ReplaceConnection. + try + { + newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) + { + // A failed physical open means the server is unreachable, so enter the blocking + // period exactly as OpenNewInternalConnection and WaitHandleDbConnectionPool.CreateObject + // do: subsequent opens fast-fail until the period expires. Activation failures in the + // try below are intentionally excluded -- the server proved reachable -- matching the + // WaitHandle pool, where PrepareConnection runs outside CreateObject's error-state catch. + // We exclude OperationCanceledException (caller-side timeout/cancellation, not a physical + // failure) and only enter while Running, mirroring OpenNewInternalConnection. + if (State == Running) + { + _errorState?.Enter(ex); + } + + throw; + } + + try + { + newConnection.ClearGeneration = _clearGeneration; + + lock (newConnection) + { + // PostPop requires a lock on the connection. + newConnection.PostPop(owningObject); + } + + // TODO: Full transaction enlistment support (Story 2). + newConnection.ActivateConnection(oldConnection.EnlistedTransaction); + + // Place new into old's slot + bool replaced = _connectionSlots.TryReplace(oldConnection, newConnection); + + if (!replaced) + { + // Should never happen (oldConnection is checked out, so its slot is stable), + // but guard against vending a connection the pool isn't tracking. + throw new InvalidOperationException(StringsHelper.GetString(Strings.SQL_ConnectionPoolReplaceConnectionFailed)); + } + } + catch + { + try + { + newConnection.DeactivateConnection(); + } + catch + { + // Preserve the original failure; best-effort cleanup only. + } + + newConnection.Dispose(); + throw; + } + + // A successful open clears the blocking period, mirroring OpenNewInternalConnection. + _errorState?.Clear(); + + // Only retire the old connection after the replacement is fully activated and we know we won't fail. + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); + } + + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, connection replaced successfully.", Id); + + return newConnection; } /// @@ -1023,10 +1115,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) { @@ -1036,8 +1129,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/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index d63571bd55..c86b30525a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1616,7 +1616,9 @@ public void Open(SqlConnectionOverrides overrides) { statistics = SqlStatistics.StartTimer(Statistics); - if (!(IsProviderRetriable ? TryOpenWithRetry(null, false, overrides) : TryOpen(null, false, overrides))) + if (!(IsProviderRetriable ? + TryOpenWithRetry(retry: null, forceNewConnection: false, overrides: overrides) : + TryOpen(retry: null, forceNewConnection: false, overrides: overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -2252,16 +2254,22 @@ private bool TryOpen(TaskCompletionSource retry, bool forc /// 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. + /// Provide to replace the existing inner connection with a freshly established one (for example, during reconnect after a transient fault); provide when opening 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 (or was 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. + /// + /// may be when the connection is currently open, or when + /// it was previously opened and is now disconnected (the reconnect case handled by + /// DbConnectionClosedPreviouslyOpened and DbConnectionClosedConnecting). Passing + /// on a connection that has never been opened will result in an exception. + /// + /// + /// may be when the connection has never been opened or is + /// currently disconnected. Passing on a connection that is already open will result in an + /// exception. + /// /// internal bool TryOpenInner(TaskCompletionSource retry, bool forceNewConnection) { diff --git a/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs b/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs index 7064a6c19c..75ee7d7b82 100644 --- a/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs +++ b/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs @@ -3147,6 +3147,15 @@ internal static string SQL_ConnectionPoolNoEmptySlot { } } + /// + /// Looks up a localized string similar to Could not replace the connection because it is no longer in the connection pool.. + /// + internal static string SQL_ConnectionPoolReplaceConnectionFailed { + get { + return ResourceManager.GetString("SQL_ConnectionPoolReplaceConnectionFailed", resourceCulture); + } + } + /// /// Looks up a localized string similar to The connection pool has been shut down.. /// diff --git a/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx b/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx index e7f438873f..4d65d00a71 100644 --- a/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx +++ b/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx @@ -2157,6 +2157,9 @@ Could not find an empty slot in the connection pool. + + Could not replace the connection because it is no longer in the connection pool. + The connection pool has been shut down. 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..6df01bc1d9 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -0,0 +1,736 @@ +// 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.Transactions; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Extensions.Time.Testing; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Unit tests for , + /// covering idle reuse, new-connection creation, pool-slot accounting at and below capacity, and the + /// failure paths that keep the old connection available for the caller's reconnect retry loop. + /// + public class ChannelDbConnectionPoolReplaceConnectionTest + { + /// + /// The factory backing every pool built by these tests. xUnit constructs a fresh instance of + /// the test class for each test, so this is not shared state across tests and each test is + /// free to toggle its tunable flags. + /// + private readonly TunableSqlConnectionFactory _factory = new(); + + /// + /// Builds a for the replacement tests. A frozen + /// is injected by default so time-driven background + /// maintenance (idle-timeout pruning, warmup/replenishment, blocking-period expiry) + /// cannot advance and race the assertions. Pass an explicit + /// only when a test needs to drive time forward deterministically. + /// + /// + /// The connection string pins Pool Blocking Period to AlwaysBlock rather than relying on + /// the default (Auto). Auto derives the policy from ADP.IsAzureSqlServerEndpoint(DataSource), + /// which reads the process-wide mutable ADP.s_azureSqlServerEndpoints list. Other tests in + /// this assembly (e.g. ConnectionRoutingTestsAzure) register "localhost" as an Azure endpoint + /// for the duration of their run, and xUnit executes separate collections in parallel. Under + /// Auto that would classify our localhost pool as Azure, skip creating the blocking-period + /// error state entirely, and make every blocking-period assertion below flaky. Pinning the + /// policy makes these tests independent of that global state. + /// + private ChannelDbConnectionPool ConstructPool( + SqlConnectionFactory connectionFactory, + DbConnectionPoolGroupOptions? poolGroupOptions = null, + TimeProvider? timeProvider = 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;Pool Blocking Period=AlwaysBlock;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + return new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo(), + timeProvider: timeProvider ?? new FakeTimeProvider() + ); + } + + #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() + { + // Arrange + var pool = ConstructPool(_factory); + 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); + } + + /// + /// Verifies that after a replacement the old connection is disposed and can no longer + /// be pooled. + /// + [Fact] + public void ReplaceConnection_OldConnectionIsDisposed() + { + // Arrange + var pool = ConstructPool(_factory); + 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) + + /// + /// 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() + { + // Arrange — single connection, no idle connections available + var pool = ConstructPool(_factory); + 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); + } + + /// + /// 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() + { + // 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(_factory, 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.NotSame(conn1, newConnection); + Assert.Equal(3, pool.Count); + } + + #endregion + + #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() + { + // Arrange — use a factory that succeeds initially then fails + var pool = ConstructPool(_factory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Switch to failing mode + _factory.FailOnCreate = true; + + // Act & Assert — exception from factory is propagated + Assert.Throws(() => + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + } + + /// + /// 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. The failed physical open enters the + /// blocking-period error state (mirroring the normal acquire path and the WaitHandle pool), so the + /// caller's retry succeeds only once that period expires. + /// + [Fact] + public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() + { + // Arrange — fill the pool to capacity so a leaked or prematurely released slot would be observable. + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 2, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var fakeTime = new FakeTimeProvider(); + var pool = ConstructPool(_factory, poolGroupOptions, timeProvider: fakeTime); + + 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. + _factory.FailOnCreate = true; + + // Act — replacement fails + Assert.Throws(() => + pool.ReplaceConnection( + owner1, + oldConnection!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // 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 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 failed physical open entered the blocking period, mirroring the normal + // acquire path and the WaitHandle pool, so subsequent opens fast-fail until it expires. + Assert.True(pool.ErrorOccurred); + + // The reconnect retry loop reuses the SAME old connection. Advancing past the blocking + // period fires the exit timer (FakeTimeProvider invokes it synchronously), after which a + // subsequent successful replacement reuses the retained slot and keeps the count unchanged. + _factory.FailOnCreate = false; + fakeTime.Advance(TimeSpan.FromSeconds(5)); + Assert.False(pool.ErrorOccurred); + var newConnection = pool.ReplaceConnection( + owner1, + oldConnection!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); + Assert.Equal(2, pool.Count); + } + + #endregion + + #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() + { + // Arrange + 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)))); + } + + /// + /// Verifies that when activating the replacement connection fails, the newly created + /// connection is disposed (never taking a pool slot) and the old connection is left intact, + /// so the pool's physical connection count is unchanged and nothing is leaked. + /// + [Fact] + public void ReplaceConnection_ActivationFails_NewConnectionDisposed_PoolCountStable() + { + // Arrange + 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 + Assert.Throws(() => + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // Assert — the new connection never took a slot and is disposed on the failure path, + // while the old connection is left in place for the caller's reconnect retry loop, so + // the pool's physical connection count is unchanged (nothing leaked). + Assert.Equal(countBefore, pool.Count); + } + + #endregion + + #region Story 6 — Prefer Idle Connection Reuse + + /// + /// Verifies that when a live idle connection is available, replacement reuses it instead of + /// establishing a new physical connection. The reused connection keeps its own pool slot and + /// the replaced connection's slot is freed, so the pool's physical connection count drops by + /// one and never exceeds the maximum. + /// + [Fact] + public void ReplaceConnection_PrefersIdleOverNewConnection() + { + // Arrange — open two connections, then return one so it becomes an idle connection. + var pool = ConstructPool(_factory); + 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); + + pool.ReturnInternalConnection(conn2!, owner2); + Assert.Equal(1, pool.IdleCount); + Assert.Equal(2, pool.Count); + + // Act — replace conn1. The idle conn2 should be reused rather than creating a new connection. + var newConnection = pool.ReplaceConnection( + owner1, + conn1!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — the replacement is the previously idle connection ... + Assert.Same(conn2, newConnection); + // ... the idle channel was drained ... + Assert.Equal(0, pool.IdleCount); + // ... the replaced connection was disposed ... + Assert.False(conn1!.CanBePooled); + // ... and its slot was freed, so the pool now holds a single physical connection. + Assert.Equal(1, pool.Count); + } + + /// + /// Verifies that reusing an idle connection while the pool is at maximum capacity succeeds and + /// frees the replaced connection's slot, so the pool count never exceeds the maximum. + /// + [Fact] + public void ReplaceConnection_IdleReuse_AtMaxCapacity_FreesOldSlot() + { + // Arrange — fill the pool to max capacity, then return one connection so it is idle. + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 3, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var pool = ConstructPool(_factory, 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); + + pool.ReturnInternalConnection(conn3!, owner3); + Assert.Equal(1, pool.IdleCount); + Assert.Equal(3, pool.Count); + + // Act — replace conn1 while at max capacity; the idle conn3 should be reused. + var newConnection = pool.ReplaceConnection( + owner1, + conn1!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — the idle connection was reused and conn1's slot was freed, dropping below max. + Assert.Same(conn3, newConnection); + Assert.Equal(0, pool.IdleCount); + Assert.Equal(2, pool.Count); + } + + /// + /// Verifies that when activating a reused idle connection fails, the connection is returned to + /// the pool (not leaked or discarded) and the connection being replaced is left untouched, so + /// the caller's reconnect retry loop can try again. + /// + [Fact] + public void ReplaceConnection_IdleReuse_ActivationFails_ReturnedToPool() + { + // Arrange — open two connections, then return one so it becomes an idle connection. + var pool = ConstructPool(_factory); + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + + _factory.FailOnActivate = false; + 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); + + pool.ReturnInternalConnection(conn2!, owner2); + Assert.Equal(1, pool.IdleCount); + Assert.Equal(2, pool.Count); + + // Make the idle-reuse activation fail. + _factory.FailOnActivate = true; + + // Act — ReplaceConnection pulls the idle conn2 and fails to activate it. + Assert.Throws(() => + pool.ReplaceConnection( + owner1, + conn1!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // Assert — the reused connection was returned to the idle pool (not leaked or discarded) ... + Assert.Equal(1, pool.IdleCount); + // ... nothing was removed, so both connections still hold their slots ... + Assert.Equal(2, pool.Count); + // ... and the connection being replaced was left untouched and still healthy. + Assert.False(conn1!.IsConnectionDoomed); + } + + #endregion + + #region Story 7 — New Physical Connection Fallback + + /// + /// 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() + { + // Arrange + var pool = ConstructPool(_factory); + 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 Blocking Period + + /// + /// Verifies that the new-physical-connection branch of + /// respects the pool's blocking period: + /// while the pool is in the blocking-period error state it fast-fails with the cached exception + /// instead of opening another physical connection, and it leaves the old connection intact for + /// the caller's reconnect retry. Idle reuse is intentionally exempt, matching the normal acquire path. + /// + [Fact] + public void ReplaceConnection_NewPhysicalConnection_RespectsBlockingPeriod() + { + // Arrange — ConstructPool pins Pool Blocking Period=AlwaysBlock, so the blocking period is enabled. + var pool = ConstructPool(_factory); + SqlConnection owner = new(); + + // Check out a connection to later replace (creation succeeds). + _factory.FailOnCreate = false; + pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? oldConnection); + Assert.NotNull(oldConnection); + + // Drive the pool into the blocking-period error state with a failed physical create. + _factory.FailOnCreate = true; + var originalException = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + Assert.True(pool.ErrorOccurred); + + // Act & Assert — a replacement that must open a new physical connection (no idle available) + // fast-fails during the blocking period rather than hammering the unhealthy server. + // Flipping the factory back to succeeding proves the create path was never reached: the + // throw can only be the cached exception, which ThrowIfActive rethrows as-is for + // non-SqlException types, so it is the very same instance captured above. + _factory.FailOnCreate = false; + var replaceException = Assert.Throws(() => + pool.ReplaceConnection(owner, oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + Assert.Same(originalException, replaceException); + + // The pool is still blocking and the old connection is untouched, so the caller can retry with it. + Assert.True(pool.ErrorOccurred); + Assert.False(oldConnection!.IsConnectionDoomed); + Assert.Same(pool, oldConnection!.Pool); + } + + /// + /// Verifies that when the new-physical-connection branch of + /// fails to open (server unreachable), + /// the pool enters the blocking-period error state, mirroring the normal acquire path + /// (OpenNewInternalConnection) and the legacy WaitHandle pool's CreateObject. This lets + /// subsequent opens fast-fail instead of hammering the unhealthy server, while the old + /// connection is left intact for the caller's reconnect retry loop. + /// + [Fact] + public void ReplaceConnection_NewPhysicalConnectionFails_EntersBlockingPeriod() + { + // Arrange — ConstructPool pins Pool Blocking Period=AlwaysBlock, so the blocking period is enabled. + var pool = ConstructPool(_factory); + SqlConnection owner = new(); + + // Check out a connection to later replace (creation succeeds), leaving no idle connection + // so the replacement is forced down the new-physical-connection branch. + _factory.FailOnCreate = false; + pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? oldConnection); + Assert.NotNull(oldConnection); + Assert.False(pool.ErrorOccurred); + + // Act — the replacement's physical open fails. + _factory.FailOnCreate = true; + Assert.Throws(() => + pool.ReplaceConnection(owner, oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // Assert — the failed open poisoned the pool into the blocking period, and the old + // connection is left intact so the caller can retry with it. + Assert.True(pool.ErrorOccurred); + Assert.False(oldConnection!.IsConnectionDoomed); + Assert.Same(pool, oldConnection!.Pool); + } + + /// + /// Verifies that when a replacement's physical open succeeds but activation fails, the pool + /// does NOT enter the blocking-period error state. A reachable server that fails activation + /// is not a connectivity failure, so poisoning the pool would be wrong. This mirrors the + /// legacy WaitHandle pool, where PrepareConnection (activation) runs outside CreateObject's + /// error-state catch. + /// + [Fact] + public void ReplaceConnection_ActivationFails_DoesNotEnterBlockingPeriod() + { + // Arrange + var pool = ConstructPool(_factory); + SqlConnection owner = new(); + + _factory.FailOnActivate = false; + pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? oldConnection); + Assert.NotNull(oldConnection); + Assert.False(pool.ErrorOccurred); + + // Act — the replacement opens successfully but fails during activation. + _factory.FailOnActivate = true; + Assert.Throws(() => + pool.ReplaceConnection(owner, oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // Assert — activation failure does not poison the pool: the server proved reachable. + Assert.False(pool.ErrorOccurred); + } + + #endregion + + #region Test Helper Classes + + /// + /// A single tunable connection factory used by all tests in this class. Set + /// to simulate a failed physical open, and + /// to simulate a connection that opens but fails activation. + /// Connections read live at activation time, so it can be + /// toggled after a connection has been created (as idle-reuse tests require). + /// + internal class TunableSqlConnectionFactory : SqlConnectionFactory + { + /// When true, throws instead of returning a connection. + internal bool FailOnCreate { get; set; } + + /// When true, activating any connection from this factory throws. + internal bool FailOnActivate { get; set; } + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (FailOnCreate) + { + throw new InvalidOperationException("Simulated connection failure"); + } + + return new StubDbConnectionInternal(this); + } + } + + /// + /// A minimal stub whose activation behaviour is driven by + /// the that created it. The flag is read live so a + /// test can make activation fail on a connection that was created earlier. + /// + internal class StubDbConnectionInternal : DbConnectionInternal + { + private readonly TunableSqlConnectionFactory? _factory; + + internal StubDbConnectionInternal(TunableSqlConnectionFactory? factory = null) + { + _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 == true) + { + 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 3592a3e54b..442592b9cf 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -858,34 +858,49 @@ public void TestUseLoadBalancing() #endregion - #region Not Implemented Method Tests + #region Replace Connection Tests /// - /// Verifies that remains - /// unimplemented and throws . + /// Verifies that + /// replaces a checked-out connection with a new, distinct connection instance. /// [Fact] - public void TestPutObjectFromTransactedPool() + public void TestReplaceConnection() { // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); + var fakeTime = new FakeTimeProvider(); + var pool = ConstructPool(SuccessfulConnectionFactory, timeProvider: fakeTime); + SqlConnection owner = new(); - // Act & Assert - Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); + 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); } + #endregion + + #region Not Implemented Method Tests + /// - /// Verifies that - /// remains unimplemented and throws . + /// Verifies that remains + /// unimplemented and throws . /// [Fact] - public void TestReplaceConnection() + public void TestPutObjectFromTransactedPool() { // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); // Act & Assert - Assert.Throws(() => pool.ReplaceConnection(null!, null!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); } /// @@ -1615,16 +1630,21 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() /// Verifies that a connection creation failure enters the blocking-period error state when /// blocking is enabled for the pool, and stays out of it when blocking is disabled. The /// blocking policy is driven by the connection string's Pool Blocking Period: - /// Default/Auto enable blocking for a non-Azure host (localhost), AlwaysBlock forces it on, + /// Default/Auto enable blocking for a non-Azure host, AlwaysBlock forces it on, /// and NeverBlock suppresses it. FR-006, FR-007. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds across multiple jobs with Assert Expected:True/Actual:False in <2ms): the - // assertion races the background warmup/replenishment work added in #4452 before the - // pool's error-state transition is observable. Not a defect in this PR. - [Trait("Category", "flaky")] + /// + /// The Auto cases here assert the real Auto => "not an Azure endpoint" => blocking mapping, + /// so they cannot pin Pool Blocking Period to sidestep endpoint classification. They + /// deliberately avoid the data source "localhost": ADPHelper (used by the simulated-server + /// Azure routing tests) temporarily registers "localhost" as an Azure endpoint in the + /// process-wide ADP.s_azureSqlServerEndpoints list. Because a pool decides whether blocking + /// is enabled exactly once, in its constructor, a pool built inside that window would + /// classify localhost as Azure and never block, making these assertions flaky under + /// parallel collection execution. + /// [Theory] - [InlineData("", true)] // Default (unspecified) => Auto => blocks for localhost + [InlineData("", true)] // Default (unspecified) => Auto => blocks for non-Azure host [InlineData("Pool Blocking Period=Auto;", true)] // Auto => blocks for non-Azure host [InlineData("Pool Blocking Period=NeverBlock;", false)] [InlineData("Pool Blocking Period=AlwaysBlock;", true)] @@ -1632,7 +1652,7 @@ public void ErrorOccurred_OnFailure_FollowsBlockingPeriod(string blockingPeriodC { // Arrange var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions($"Data Source=localhost;{blockingPeriodClause}"), + new SqlConnectionOptions($"Data Source=non-azure-test-host;{blockingPeriodClause}"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1658,17 +1678,19 @@ public void ErrorOccurred_OnFailure_FollowsBlockingPeriod(string blockingPeriodC /// Verifies that once the pool enters the blocking period, subsequent synchronous requests /// fail fast with the cached exception without attempting another physical open. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds): races the background warmup/replenishment work added in #4452 before the - // pool's error-state transition is observable. Not a defect in this PR. - [Trait("Category", "flaky")] + /// + /// Pins Pool Blocking Period to AlwaysBlock: this test needs blocking enabled but is not + /// testing endpoint classification, and leaving it on Auto would make it depend on + /// "localhost" not being registered as an Azure endpoint by a concurrently running test + /// (see ErrorOccurred_OnFailure_FollowsBlockingPeriod for the full explanation). + /// [Fact] public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() { // Arrange var factory = new CountingTimeoutConnectionFactory(); var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;"), + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=AlwaysBlock;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1702,16 +1724,16 @@ public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() /// Verifies that clearing the pool while in the blocking-period error state resets the /// externally visible error indicator. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds): races the background warmup/replenishment work added in #4452 before the - // pool's error-state transition is observable. Not a defect in this PR. - [Trait("Category", "flaky")] + /// + /// Pins Pool Blocking Period to AlwaysBlock so entering the error state does not depend on + /// endpoint classification. See ErrorOccurred_OnFailure_FollowsBlockingPeriod. + /// [Fact] public void Clear_InErrorState_ResetsErrorOccurred() { // Arrange var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;"), + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=AlwaysBlock;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1742,10 +1764,10 @@ public void Clear_InErrorState_ResetsErrorOccurred() /// so the test is deterministic and does not wait on /// wall-clock time. FR-006, FR-009. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds): races the background warmup/replenishment work added in #4452 before the - // pool's error-state transition is observable. Not a defect in this PR. - [Trait("Category", "flaky")] + /// + /// Pins Pool Blocking Period to AlwaysBlock so entering the error state does not depend on + /// endpoint classification. See ErrorOccurred_OnFailure_FollowsBlockingPeriod. + /// [Fact] public void Failure_ThenBlockingPeriodExpiry_AllowsSuccessfulCreate() { @@ -1753,7 +1775,7 @@ public void Failure_ThenBlockingPeriodExpiry_AllowsSuccessfulCreate() var factory = new ToggleFailureConnectionFactory(); var fakeTime = new FakeTimeProvider(); var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;"), + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=AlwaysBlock;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -2386,3 +2408,4 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } } + diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs index 7d2ad7347a..0657ed521b 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs @@ -45,7 +45,15 @@ private static ChannelDbConnectionPool ConstructPool( idleTimeout: idleTimeout); var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;"), + // Pool Blocking Period is pinned to AlwaysBlock so the tests that assert the pool + // enters its blocking-period error state do not depend on endpoint classification. + // Under Auto, blocking is enabled only when the data source is not an Azure + // endpoint, and ADPHelper (used by the simulated-server Azure routing tests) + // temporarily registers "localhost" as an Azure endpoint in the process-wide + // ADP.s_azureSqlServerEndpoints list. A pool evaluates blocking exactly once, in its + // constructor, so a pool built inside that window would never block - making those + // assertions flaky under parallel collection execution. + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=AlwaysBlock;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), poolGroupOptions); @@ -334,11 +342,6 @@ public async Task Warmup_RateLimiterSaturated_UserSharesSameLimiter() /// does enter the pool's blocking-period error state. Warmup then stops the pass rather than /// spinning on the persistent failure. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds): the background warmup thread added in #4452 races the assertion so the - // pool's error-state transition is observed late (Assert Expected:True/Actual:False). - // Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public async Task Warmup_AllCreationsFail_AbsorbedAndEntersErrorState() { @@ -370,11 +373,6 @@ public async Task Warmup_AllCreationsFail_AbsorbedAndEntersErrorState() /// exception rather than attempting a fresh on-demand open. The pool remains operational and /// resumes creating on demand once the blocking period expires. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds): the background warmup thread added in #4452 races the assertion so the - // pool's error-state transition is observed late (Assert Expected:True/Actual:False). - // Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public async Task Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() { @@ -400,11 +398,6 @@ public async Task Warmup_Fails_UserRequestFastFailsDuringBlockingPeriod() /// request (rather than warmup itself) keeps the two behaviors independent, and awaiting the /// warmup task makes the stand-down deterministic. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; fails on PR merge - // builds): the background warmup thread added in #4452 races the assertion so the - // pool's error-state transition is observed late (Assert Expected:True/Actual:False). - // Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public async Task Warmup_RespectsErrorState_StandsDownWhileBlocking() { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs index 28e59e7a5d..53390068fb 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs @@ -483,5 +483,181 @@ 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)); + // The occupant of the slot was left untouched, so it is still removable. + Assert.True(poolSlots.TryRemove(existingConnection)); + } + + /// + /// Verifies that replacing a connection with itself is a benign no-op: it reports success, + /// leaves the connection in its slot, and does not change the reservation count. + /// + [Fact] + public void TryReplace_SameConnection_ReturnsTrueAndLeavesConnectionInSlot() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var connection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { })!; + + // Act - replace the connection with itself + var replaced = poolSlots.TryReplace(connection, connection); + + // Assert - the slot still holds the same connection and the count is unchanged + Assert.True(replaced); + Assert.Equal(1, poolSlots.ReservationCount); + Assert.True(poolSlots.TryRemove(connection)); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// 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)); + } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs index 0197d62292..c5d628c618 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs @@ -52,9 +52,19 @@ public void Dispose() /// uses it as its clock so the exit timer can be driven /// deterministically; otherwise the system clock is used. /// + /// + /// The default connection string pins Pool Blocking Period to AlwaysBlock so the tests that + /// assert the pool enters its blocking-period error state do not depend on endpoint + /// classification. Under Auto, blocking is enabled only when the data source is not an Azure + /// endpoint, and ADPHelper (used by the simulated-server Azure routing tests) temporarily + /// registers "localhost" as an Azure endpoint in the process-wide + /// ADP.s_azureSqlServerEndpoints list. A pool resolves whether blocking is enabled exactly + /// once, at construction, so a pool built inside that window would never block - making those + /// assertions flaky under parallel collection execution. + /// private WaitHandleDbConnectionPool CreatePool( SqlConnectionFactory connectionFactory, - string connectionString = "Data Source=localhost;", + string connectionString = "Data Source=localhost;Pool Blocking Period=AlwaysBlock;", TimeProvider? timeProvider = null) { var poolGroupOptions = new DbConnectionPoolGroupOptions( @@ -101,11 +111,6 @@ private static bool TryGetConnectionSync( /// the originating exception is surfaced to the caller and /// becomes true. Guards the CreateObject → wiring. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; failed on a PR merge - // build): the WaitHandle pool's background create/prune thread races the ErrorOccurred - // assertion under load, and this file shares the DbConnectionInternal/SqlConnectionFactory - // changes from #4452. Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public void TryGetConnection_WhenFactoryThrows_EntersBlockingPeriod() { @@ -131,11 +136,6 @@ public void TryGetConnection_WhenFactoryThrows_EntersBlockingPeriod() /// rethrows the original instance; the fast-fail throw returns a clone (to avoid sharing stack /// traces). Guards the error wait-handle → path. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; failed on a PR merge - // build): the WaitHandle pool's background create/prune thread races the assertion under - // load, and this file shares the DbConnectionInternal/SqlConnectionFactory changes from - // #4452. Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public void TryGetConnection_WhileBlocked_FastFailsWithCachedExceptionWithoutInvokingFactory() { @@ -217,11 +217,6 @@ public void TryGetConnection_WhenFactorySucceeds_DoesNotEnterBlockingPeriod() /// an injected , guarding the /// timer-exit → retry → Clear path at the pool level. /// - // Flaky under CI load only (passes locally 3/3 and on main CI; failed on a PR merge - // build): the WaitHandle pool's background create/prune thread races the ErrorOccurred/timer - // assertions under load, and this file shares the DbConnectionInternal/SqlConnectionFactory - // changes from #4452. Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public void TryGetConnection_AfterBlockingPeriodExpires_RetriesFactoryAndRecovers() { @@ -263,11 +258,6 @@ public void TryGetConnection_AfterBlockingPeriodExpires_RetriesFactoryAndRecover /// reset is wired through the pool. Drives timing deterministically with an injected /// . /// - // Flaky under CI load only (passes locally 3/3 and on main CI; failed on a PR merge - // build): the WaitHandle pool's background create/prune thread races the ErrorOccurred/timer - // assertions under load, and this file shares the DbConnectionInternal/SqlConnectionFactory - // changes from #4452. Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public void TryGetConnection_SuccessfulCreate_ResetsBackoffToInitialWait() { @@ -320,11 +310,6 @@ public void TryGetConnection_SuccessfulCreate_ResetsBackoffToInitialWait() /// backoff at the pool level. Drives timing deterministically with an injected /// . /// - // Flaky under CI load only (passes locally 3/3 and on main CI; failed on a PR merge - // build): the WaitHandle pool's background create/prune thread races the ErrorOccurred/timer - // assertions under load, and this file shares the DbConnectionInternal/SqlConnectionFactory - // changes from #4452. Not a defect in this PR. - [Trait("Category", "flaky")] [Fact] public void TryGetConnection_FailingAgainAfterExitTimer_StillDoublesBackoff() {