diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs index 22aa0360ec..f8c37065a2 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs @@ -1337,6 +1337,19 @@ internal static Exception UndefinedPopulationMechanism(string populationMechanis internal static Exception PooledOpenTimeout() => ADP.InvalidOperation(StringsHelper.GetString(Strings.ADP_PooledOpenTimeout)); + /// + /// Builds the pooled-open timeout exception, attaching (the most + /// recent physical connection creation failure observed by the pool) so a timeout caused by + /// repeated connection failures reports the underlying cause rather than only reporting + /// pool exhaustion. Falls back to the parameterless form when there is no such failure. + /// +#nullable enable + internal static Exception PooledOpenTimeout(Exception? inner) + => inner is null + ? PooledOpenTimeout() + : ADP.InvalidOperation(StringsHelper.GetString(Strings.ADP_PooledOpenTimeout), inner); +#nullable restore + internal static Exception NonPooledOpenTimeout() => ADP.TimeoutException(StringsHelper.GetString(Strings.ADP_NonPooledOpenTimeout)); #endregion 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..b9351f235a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -3,8 +3,10 @@ // See the LICENSE file in the project root for more information. using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Collections.ObjectModel; using System.Data.Common; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; @@ -151,6 +153,15 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// requester to start the loop, and reset to 0 by the loop when it drains. /// private int _warmupLoopRunning; + + /// + /// The exception from the most recent failed physical connection open, retained purely so + /// that a subsequent pooled-open timeout can report it as an inner exception. Cleared on the + /// next successful open. Volatile rather than lock-protected: this is a best-effort + /// diagnostic snapshot, and a torn read across concurrent failures would at worst attach a + /// slightly older failure. See GH#3545. + /// + private volatile Exception? _lastConnectionCreateException; #endregion /// @@ -196,6 +207,12 @@ internal ChannelDbConnectionPool( } State = Running; + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Constructed. MinPoolSize={1}, MaxPoolSize={2}", + Id, + MinPoolSize, + MaxPoolSize); } #region Properties @@ -208,7 +225,7 @@ public ConcurrentDictionary< public SqlConnectionFactory ConnectionFactory { get; } /// - public int Count => _connectionSlots.ReservationCount; + public int Count => _connectionSlots.ConnectionCount; /// public int IdleCount => _idleChannel.Count; @@ -216,6 +233,9 @@ public ConcurrentDictionary< /// public bool ErrorOccurred => _errorState?.HasError ?? false; + /// + public Exception? LastConnectionCreateException => _lastConnectionCreateException; + /// public int Id => _instanceId; @@ -250,6 +270,14 @@ public ConcurrentDictionary< private int MinPoolSize => PoolGroupOptions.MinPoolSize; + /// + /// Indicates whether connections may be vended from (and parked in) the + /// . This mirrors automatic transaction enlistment: + /// when enlistment is disabled a connection is never bound to an ambient transaction, so + /// the transacted store must not be consulted. + /// + private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; + /// /// The most recently launched warmup/replenishment loop task, exposed so tests can await a /// warmup pass to a deterministic completion instead of polling pool counters. May be null @@ -313,7 +341,31 @@ public void Clear() /// public void PutObjectFromTransactedPool(DbConnectionInternal connection) { - throw new NotImplementedException(); + Debug.Assert(connection is not null, "null connection?"); + Debug.Assert(connection.EnlistedTransaction is null, "connection is still enlisted?"); + + // Called by the transacted connection pool once it has removed the connection from its + // list. We put the connection back into general circulation. + // + // NOTE: no locking is required here because if we're in this method we can safely + // presume that the caller is the only one using the connection, that all pre-push logic + // has been done, and that all transactions have ended. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Transaction has ended.", + Id, + connection.ObjectID); + + if (State is Running && connection.CanBePooled) + { + connection.ResetConnection(); + PutConnectionInIdleChannel(connection); + } + else + { + // RemoveConnection triggers replenishment, which is the channel pool's equivalent + // of the wait handle pool's QueuePoolCreateRequest. + RemoveConnection(connection); + } } /// @@ -322,14 +374,255 @@ 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) + { + // Carry the old connection's enlistment over to the replacement so that a connection + // replaced mid-transaction stays bound to the same transaction. + 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) + { + // Retain the failure so a caller that ultimately times out waiting for a pooled + // connection can report why creation kept failing. See GH#3545. + _lastConnectionCreateException = ex; + + // 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); + } + + // Carry the old connection's enlistment over to the replacement so that a + // connection replaced mid-transaction stays bound to the same transaction. + 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 + { + newConnection.DeactivateConnection(); + newConnection.Dispose(); + + // The physical connection was opened (and counted by HardConnectRequest in the + // factory) before activation failed, so balance the counter here. The + // connection never occupied a slot, so the pooled gauge is untouched. + SqlClientDiagnostics.Metrics.HardDisconnectRequest(); + throw; + } + + // A successful open clears the blocking period, mirroring OpenNewInternalConnection. + _lastConnectionCreateException = null; + _errorState?.Clear(); + + // Only retire the old connection after the replacement is fully activated and we know we won't fail. + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); + + // The replacement took over the old connection's slot, so the pooled gauge is + // already correct and only the hard-disconnect counter needs balancing. Traced as a + // destroy so the connection's exit is visible in the pooler trace stream. + SqlClientDiagnostics.Metrics.HardDisconnectRequest(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Disposed.", + Id, + oldConnection.ObjectID); + } + + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, connection replaced successfully.", Id); + + return newConnection; } /// public void ReturnInternalConnection(DbConnectionInternal connection, DbConnection owningObject) { + SqlClientDiagnostics.Metrics.SoftDisconnectRequest(); + ValidateOwnershipAndSetPoolingState(connection, owningObject); + DeactivateAndRouteConnection(connection); + } + + /// + /// Deactivates a connection that is already marked as owned by the pool (via + /// ) and routes it to the idle channel, the + /// transacted pool, stasis, or destruction as appropriate. Shared by the normal return path + /// and by emancipated connection reclamation, which has already performed the + /// PrePush itself and must not re-validate ownership. + /// + /// The connection to deactivate and route. + private void DeactivateAndRouteConnection(DbConnectionInternal connection) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Deactivating.", + Id, + connection.ObjectID); + + // Deactivate before inspecting the connection's transaction state. Deactivation is what + // detaches a completed transaction, so reading EnlistedTransaction beforehand could park + // a connection in the transacted pool under a transaction that has already ended. + connection.DeactivateConnection(); + + if (connection.IsConnectionDoomed) + { + // The connection is not fit for reuse -- just dispose of it. + RemoveConnection(connection); + return; + } + + bool returnToGeneralPool = false; + bool destroyConnection = false; + bool rootTxn = false; + + // A connection with a delegated transaction cannot be handed to a different customer + // until the transaction actually completes, so we send it into stasis -- the + // System.Transactions transaction object keeps it owned (not lost) and is certain to + // put it back into the pool. The decision is made under the connection lock so that a + // transaction completing asynchronously on another thread cannot race with us. + lock (connection) + { + if (State is ShuttingDown) + { + if (connection.IsTransactionRoot) + { + // Connections affiliated with a root transaction that happen to live in a + // pool being shut down must be put in stasis so the root transaction isn't + // orphaned with no means to promote itself to a full delegated transaction + // or to Commit/Rollback. + connection.SetInStasis(); + rootTxn = true; + } + else + { + destroyConnection = true; + } + } + else if (connection.IsTransactionRoot && connection.Pool is null) + { + connection.SetInStasis(); + rootTxn = true; + } + else if (connection.CanBePooled) + { + Transaction? transaction = connection.EnlistedTransaction; + if (transaction is not null) + { + // NOTE: we're not locking on State, so its value could change between the + // conditional check above and here. Although perhaps not ideal, this is OK + // because the DelegatedTransactionEnded event will clean up the connection + // appropriately regardless of the pool state. + // + // Transacting connections are held in their own store and are never + // proactively closed (doing so would abort the transaction, which can be + // distributed). Idle-timeout enforcement does not apply here, so we do not + // stamp the returned time when parking the connection in the transacted pool. + TransactedConnectionPool.PutTransactedObject(transaction, connection); + rootTxn = true; + } + else + { + returnToGeneralPool = true; + } + } + else if (connection.IsTransactionRoot) + { + // The connection cannot be pooled but is a transaction root, so we must have + // hit a race condition: either the pool was shut down or the load balancing + // timeout expired and marked the connection as non-poolable while we were + // processing within this lock. Put it in stasis so the root transaction isn't + // orphaned with no means to promote itself to a full delegated transaction or + // to Commit/Rollback. + connection.SetInStasis(); + rootTxn = true; + } + else + { + destroyConnection = true; + } + } + + if (returnToGeneralPool) + { + Debug.Assert(!destroyConnection, "Connection cannot both be pooled and destroyed."); + PutConnectionInIdleChannel(connection); + } + else if (destroyConnection) + { + RemoveConnection(connection); + } + else + { + // The connection was parked in the transacted pool or placed in stasis. Neither + // path returns it to the idle channel, so without this trace the connection simply + // disappears from the pool's trace stream after deactivation. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Held by a transaction; not returned to the general pool.", + Id, + connection.ObjectID); + } + + // Ensure the connection was processed by exactly one of the paths above. + Debug.Assert(rootTxn || returnToGeneralPool || destroyConnection, + "Returned connection was neither pooled, destroyed, nor placed in stasis."); + } + + /// + /// Places a connection that is fit for general reuse into the idle channel, stamping its + /// idle-return time and dropping it if it is no longer live. + /// + /// The connection to make available to other callers. + private void PutConnectionInIdleChannel(DbConnectionInternal connection) + { // Stamp the return time before IsLiveConnection runs so the idle-expiry gate inside it // measures time-in-pool, not time-since-last-return. Without this, a connection whose // checkout exceeded IdleTimeout (e.g. a long-running query) would be wrongly evicted on @@ -349,26 +642,16 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti } SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Deactivating.", + " {0}, Connection {1}, Pushing to general pool.", Id, connection.ObjectID); - connection.DeactivateConnection(); - if (connection.IsConnectionDoomed || - !connection.CanBePooled || - State == ShuttingDown) + if (!_idleChannel.TryWrite(connection)) { + // The channel has been completed (pool is shutting down). Race window + // between the State check by the caller and TryWrite: destroy instead of pooling. RemoveConnection(connection); } - else - { - if (!_idleChannel.TryWrite(connection)) - { - // The channel has been completed (pool is shutting down). Race window - // between the State check above and TryWrite: destroy instead of pooling. - RemoveConnection(connection); - } - } } /// @@ -522,7 +805,21 @@ public void Startup() /// public void TransactionEnded(Transaction transaction, DbConnectionInternal transactedObject) { - throw new NotImplementedException(); + Debug.Assert(transaction is not null, "null transaction?"); + Debug.Assert(transactedObject is not null, "null transactedObject?"); + + // Note: the connection may still be associated with the transaction due to the explicit + // unbinding requirement. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Transaction {1}, Connection {2}, Transaction Completed", + Id, + transaction.GetHashCode(), + transactedObject.ObjectID); + + // If the connection is in the transacted pool, remove it there and return it to general + // circulation. TransactedConnectionPool.TransactionEnded calls back into + // PutObjectFromTransactedPool for us once it has removed the connection from its list. + TransactedConnectionPool.TransactionEnded(transaction, transactedObject); } /// @@ -548,10 +845,12 @@ public bool TryGetConnection( // If taskCompletionSource is null, we are in a sync context. if (taskCompletionSource is null) { + // We're on the caller's thread, so the ambient transaction is directly observable. var task = GetInternalConnection( owningObject, async: false, - timeout); + timeout, + ADP.GetCurrentTransaction()); // When running synchronously, we are guaranteed that the task is already completed. // We don't need to guard the managed threadpool at this spot because we pass the async flag as false @@ -580,6 +879,46 @@ public bool TryGetConnection( // OpenAsync call. This means that we cannot cancel the connection open operation if the caller's token // is cancelled. We can only cancel based on our own timeout, which is set to the owningObject's // ConnectionTimeout. + // The ambient transaction is captured here, on the caller's thread, because + // Transaction.Current does not flow into the Task.Run below: a TransactionScope keeps + // the ambient transaction in thread-static storage unless it was created with + // TransactionScopeAsyncFlowOption.Enabled. We rely on the caller to capture the ambient + // transaction in the TaskCompletionSource's AsyncState, and then hand it to + // GetInternalConnection explicitly. + // + // Note that we deliberately do not assign Transaction.Current on the thread pool + // thread. That assignment writes to thread-static storage which is *not* unwound when + // the ExecutionContext is restored, so it would outlive this open and be observed by + // unrelated work later scheduled onto the same thread pool thread -- including the + // login-time auto-enlistment that non-pooled connections perform against + // Transaction.Current. The WaitHandle pool can get away with assigning it because it + // processes pending opens on a dedicated non-thread-pool thread. + Transaction? ambientTransaction = taskCompletionSource.Task.AsyncState as Transaction; + + // Try to satisfy the request synchronously from the idle channel before paying for a + // thread pool hop. WaitHandleDbConnectionPool makes the same non-blocking, non-creating + // attempt before enqueuing a pending open, so without this an async open against a warm + // pool would always complete asynchronously under this pool but synchronously under the + // other -- a behavioural difference callers can observe. We deliberately do not try to + // *create* a connection here; that can block on the wire and must stay off the caller's + // thread. + // + // Transactional requests are excluded: they must first consult the transacted store for + // a connection already enlisted in the same transaction, which only GetInternalConnection + // does. Taking a plain idle connection here would both miss that affinity and skip + // enlistment. + if (!(HasTransactionAffinity && ambientTransaction is not null)) + { + DbConnectionInternal? idleConnection = GetIdleConnection(); + if (idleConnection is not null) + { + PrepareConnection(owningObject, idleConnection); + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + connection = idleConnection; + return true; + } + } + Task.Run(async () => { if (taskCompletionSource.Task.IsCompleted) @@ -587,10 +926,6 @@ public bool TryGetConnection( return; } - // We're potentially on a new thread, so we need to properly set the ambient transaction. - // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState - // so that we can access it here. Read: area for improvement. - // TODO: ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); DbConnectionInternal? connection = null; try @@ -598,7 +933,8 @@ public bool TryGetConnection( connection = await GetInternalConnection( owningObject, async: true, - timeout + timeout, + ambientTransaction ).ConfigureAwait(false); if (!taskCompletionSource.TrySetResult(connection)) @@ -653,8 +989,17 @@ public bool TryGetConnection( // pool, whose replenishment enters/clears the same error state as user requests. In // practice the warmup loop already stands down before reaching here (its loop condition // checks ErrorOccurred); this covers the narrow race where the state flips in between. + if (ErrorOccurred) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Errors are set.", Id); + } + _errorState?.ThrowIfActive(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Creating new connection.", Id); + try { // Reserve a pool slot up front so we don't pay the rate-limit cost only to @@ -691,6 +1036,9 @@ public bool TryGetConnection( // TODO: When we fail to acquire a lease, surface the lease metadata // (e.g. RateLimitMetadataName.RetryAfter, ReasonPhrase) in the error // path so the user can identify why the lease was denied. + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Rate limiter saturated; deferring creation to the idle wait.", + Id); faulted = false; return null; } @@ -765,20 +1113,48 @@ _connectionCreationRateLimiter is not null && if (connection is not null) { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Added to pool.", + Id, + connection.ObjectID); + + SqlClientDiagnostics.Metrics.EnterPooledConnection(); + // A new connection was added to the pool. If we've grown past MinPoolSize, // start the pruning timer so idle connections can be reclaimed. Pruner?.UpdateTimer(); + // A successful open proves the server is reachable, so a previously recorded + // failure is no longer a useful explanation for a later timeout. See GH#3545. + _lastConnectionCreateException = null; + // A successful creation clears error/backoff state (FR-009). Warmup goes through // this same path and clears the state on success too, mirroring the legacy // WaitHandle pool: a connection that opens proves the server is reachable. _errorState?.Clear(); } + else + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, No connection created; pool is full or creation is rate limited.", + Id); + } return connection; } catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, PoolCreateRequest called CreateConnection which threw an exception: {1}", + Id, + ex); + + // Retain the failure so a caller that ultimately times out waiting for a pooled + // connection can report why creation kept failing. Recorded regardless of whether + // the blocking period is enabled for this pool group, since the timeout can occur + // either way. See GH#3545. + _lastConnectionCreateException = ex; + // Enter the blocking period error state on creation failure if configured. Warmup // goes through this same path (the warmup loop absorbs the rethrow in its own catch), // mirroring the legacy WaitHandle pool, whose replenishment failures also enter the @@ -827,37 +1203,83 @@ private bool IsLiveConnection(DbConnectionInternal connection) idleTimeout != TimeSpan.Zero && _timeProvider.GetUtcNow().UtcDateTime - connection.ReturnedTime > idleTimeout) { + TraceNotLive(connection, "exceeded the connection idle timeout"); return false; } // Broken physical connection if (!connection.IsConnectionAlive()) { + TraceNotLive(connection, "found dead"); return false; } // Connection has been alive longer than the load balance timeout if (LoadBalanceTimeout != TimeSpan.Zero && DateTime.UtcNow > connection.CreateTime + LoadBalanceTimeout) { + TraceNotLive(connection, "exceeded the load balance timeout"); return false; } // Connection was created before the last Clear, so it's stale. if (connection.ClearGeneration != _clearGeneration) { + TraceNotLive(connection, "was created before the last Clear"); return false; } return true; } + /// + /// Emits the trace for a connection that failed the gate. + /// Split out so each rejection reason is reported individually: the caller only sees that + /// the connection was discarded, which on its own does not explain whether the pool is + /// churning because of idle timeout, load balancing, a Clear, or genuine server failures. + /// + /// The connection that failed the liveness gate. + /// Why the connection was rejected, phrased to read as + /// "Connection {id}, {reason} and removed." + private void TraceNotLive(DbConnectionInternal connection, string reason) => + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, {2} and removed.", + Id, + connection.ObjectID, + reason); + /// /// Closes the provided connection and removes it from the pool. /// /// The connection to be closed. private void RemoveConnection(DbConnectionInternal connection) { - _connectionSlots.TryRemove(connection); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Removing from pool.", + Id, + connection.ObjectID); + + // A connection with a delegated transaction cannot be disposed of until the delegated + // transaction has actually completed; disposing it would abort the (possibly + // distributed) transaction. Leave it alone: when the transaction completes it comes + // back through PutObjectFromTransactedPool, which calls us again. + if (connection.IsTxRootWaitingForTxEnd) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Has Delegated Transaction, waiting to Dispose.", + Id, + connection.ObjectID); + return; + } + + if (_connectionSlots.TryRemove(connection)) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Removed from pool.", + Id, + connection.ObjectID); + + SqlClientDiagnostics.Metrics.ExitPooledConnection(); + } // Removing a connection from the pool opens a free slot. // Write a null to the idle connection channel to wake up a waiter, who can now open a new @@ -865,6 +1287,12 @@ private void RemoveConnection(DbConnectionInternal connection) _idleChannel.TryWrite(null); connection.Dispose(); + SqlClientDiagnostics.Metrics.HardDisconnectRequest(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Disposed.", + Id, + connection.ObjectID); // If this removal brought us back to MinPoolSize, disable the pruning timer. Pruner?.UpdateTimer(); @@ -898,6 +1326,11 @@ private void RemoveConnection(DbConnectionInternal connection) continue; } + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Popped from general pool.", + Id, + connection.ObjectID); + return connection; } @@ -911,6 +1344,10 @@ private void RemoveConnection(DbConnectionInternal connection) /// A boolean indicating whether the operation should be asynchronous. /// The overall timeout budget for this connection request. Time spent waiting /// in the pool is deducted from the budget available for physical connection creation. + /// The ambient transaction captured on the caller's thread, or + /// null when the caller is not inside a transaction. It is passed explicitly rather than read + /// from because this method may run on a thread pool thread + /// that the ambient transaction does not flow to. /// Returns a DbConnectionInternal that is retrieved from the pool. /// /// Thrown when an OperationCanceledException is caught, indicating that the timeout period @@ -923,17 +1360,35 @@ private void RemoveConnection(DbConnectionInternal connection) private async Task GetInternalConnection( DbConnection owningConnection, bool async, - TimeoutTimer timeout) + TimeoutTimer timeout, + Transaction? ambientTransaction) { DbConnectionInternal? connection = null; + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Getting connection.", Id); + + // When automatic enlistment is disabled, the connection must never be bound to the + // ambient transaction, so we neither consult the transacted store nor hand the + // transaction to activation. HasTransactionAffinity is derived from the connection + // string's Enlist keyword. + Transaction? transaction = HasTransactionAffinity ? ambientTransaction : null; + + if (transaction is not null) + { + connection = GetFromTransactedPool(transaction); + } + // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations // (channel reads, semaphore waits) are cancelled when the overall budget expires. using CancellationTokenSource cancellationTokenSource = timeout.CreateCancellationTokenSource(); CancellationToken cancellationToken = cancellationTokenSource.Token; - // Continue looping until we create or retrieve a connection - do + // Continue looping until we create or retrieve a connection. A connection vended from + // the transacted pool skips this loop entirely: it has already been liveness-checked and + // is exempt from the idle/generation gates, because closing it would abort its + // (possibly distributed) transaction. + while (connection is null) { try { @@ -950,6 +1405,18 @@ private async Task GetInternalConnection( cancellationToken, timeout); + // Before parking on the idle channel (potentially for the full timeout), sweep + // for connections whose owning SqlConnection was garbage collected without ever + // being closed or disposed. Those "emancipated" connections still occupy pool + // slots, so at MaxPoolSize every subsequent request would otherwise time out + // forever. WaitHandleDbConnectionPool performs the same sweep before waiting. + // This is deliberately confined to the slow path: it is O(MaxPoolSize) and + // allocates a snapshot, so it must not run on the hot acquire path. + if (connection is null && ReclaimEmancipatedConnections()) + { + connection = GetIdleConnection(); + } + // If we're at max capacity and couldn't open a connection. Block on the idle channel with a // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync // (first-come, first-served), which is crucial to us. @@ -964,10 +1431,18 @@ private async Task GetInternalConnection( } catch (OperationCanceledException) { - throw ADP.PooledOpenTimeout(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Wait timed out.", Id); + + // Attach the most recent physical connection failure, if any, so a timeout + // caused by repeatedly failing opens reports that failure instead of only + // reporting pool exhaustion. See GH#3545. + throw ADP.PooledOpenTimeout(_lastConnectionCreateException); } catch (ChannelClosedException) { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Pool is shutting down; abandoning wait.", Id); throw new InvalidOperationException(StringsHelper.GetString(Strings.SQL_ConnectionPoolShutDown)); } @@ -978,12 +1453,75 @@ private async Task GetInternalConnection( connection = null; } } - while (connection is null); - PrepareConnection(owningConnection, connection); + PrepareConnection(owningConnection, connection, transaction); + SqlClientDiagnostics.Metrics.SoftConnectRequest(); return connection; } + /// + /// Reclaims connections whose owning has been garbage collected + /// without being closed or disposed. Such connections are still tracked by the pool but can + /// never be returned by their owner, so without this sweep they would leak pool slots. + /// + /// True if at least one connection was reclaimed; otherwise, false. + private bool ReclaimEmancipatedConnections() + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}", Id); + + List? reclaimed = null; + + foreach (DbConnectionInternal connection in _connectionSlots.Snapshot()) + { + // TryEnter rather than Enter: IsEmancipated must be read under the connection lock to + // avoid racing PrePush/PostPop, but a connection that is currently locked is being + // actively handed out or returned and therefore is not emancipated anyway. Skipping + // it keeps this sweep from blocking the caller. + bool locked = false; + try + { + Monitor.TryEnter(connection, ref locked); + + if (locked && connection.IsEmancipated) + { + // Do as little as possible under the lock: just claim the connection for the + // pool and defer deactivation (which can make server round trips) until the + // lock is released. + connection.PrePush(null); + (reclaimed ??= new List()).Add(connection); + } + } + finally + { + if (locked) + { + Monitor.Exit(connection); + } + } + } + + if (reclaimed is null) + { + return false; + } + + foreach (DbConnectionInternal connection in reclaimed) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Reclaiming.", + Id, + connection.ObjectID); + + SqlClientDiagnostics.Metrics.ReclaimedConnectionRequest(); + + connection.DetachCurrentTransactionIfEnded(); + DeactivateAndRouteConnection(connection); + } + + return true; + } + /// /// Performs a blocking synchronous read from the idle connection channel. /// @@ -1023,10 +1561,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 +1575,7 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c try { - //TODO: pass through transaction - connection.ActivateConnection(null); + connection.ActivateConnection(transaction); } catch { @@ -1049,6 +1587,60 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } } + /// + /// Attempts to retrieve a connection that is already enlisted in the given transaction. + /// + /// The transaction the connection must already be enlisted in. + /// A live connection already enlisted in the transaction, or null. + private DbConnectionInternal? GetFromTransactedPool(Transaction transaction) + { + DbConnectionInternal? connection = TransactedConnectionPool.GetTransactedObject(transaction); + if (connection is null) + { + return null; + } + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, Popped from transacted pool.", + Id, + connection.ObjectID); + + SqlClientDiagnostics.Metrics.ExitFreeConnection(); + + // Transacting connections are exempt from idle-timeout and clear-generation eviction + // (closing them would abort the transaction, which may be distributed), so only + // liveness is checked here rather than the full IsLiveConnection gate. + if (connection.IsTransactionRoot) + { + try + { + // A dead transaction root must surface the underlying failure to the caller: + // there is no way to recover the delegated transaction on another connection. + connection.IsConnectionAlive(throwOnException: true); + } + catch + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, found dead and removed.", + Id, + connection.ObjectID); + RemoveConnection(connection); + throw; + } + } + else if (!connection.IsConnectionAlive()) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Connection {1}, found dead and removed.", + Id, + connection.ObjectID); + RemoveConnection(connection); + connection = null; + } + + return connection; + } + /// /// Validates that the connection is owned by the provided DbConnection and that it is in a valid state to be returned to the pool. /// @@ -1275,6 +1867,15 @@ private async Task RunWarmupLoopAsync() /// internal void PruneConnections(int count) { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Pruning up to {1} idle connections. IdleCount={2}, Count={3}", + Id, + count, + IdleCount, + Count); + + int pruned = 0; + while (count > 0 && IsRunning && _connectionSlots.ReservationCount > MinPoolSize @@ -1287,7 +1888,13 @@ internal void PruneConnections(int count) RemoveConnection(connection); count--; + pruned++; } + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Pruned {1} idle connections.", + Id, + pruned); } #endregion } 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..9977f2dddd 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 @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Diagnostics; using System.Threading; using Microsoft.Data.ProviderBase; @@ -60,6 +61,7 @@ internal void Keep() private readonly DbConnectionInternal?[] _connections; private readonly uint _capacity; private volatile int _reservations; + private volatile int _connectionCount; /// /// Constructs a ConnectionPoolSlots instance with the given fixed capacity. @@ -82,14 +84,23 @@ internal ConnectionPoolSlots(uint fixedCapacity) _capacity = fixedCapacity; _reservations = 0; + _connectionCount = 0; _connections = new DbConnectionInternal?[fixedCapacity]; } /// - /// Gets the total number of reservations currently held. + /// Gets the total number of reservations currently held. This includes reservations held on + /// behalf of connections that are still being opened and are therefore not yet tracked. /// internal int ReservationCount => _reservations; + /// + /// Gets the number of connections currently tracked by this collection. Unlike + /// , this excludes reservations held for connections that are + /// still being opened, so it reports connections that actually belong to the pool. + /// + internal int ConnectionCount => _connectionCount; + /// /// Adds a connection to the collection. /// @@ -127,6 +138,7 @@ internal ConnectionPoolSlots(uint fixedCapacity) { if (Interlocked.CompareExchange(ref _connections[i], connection, null) == null) { + Interlocked.Increment(ref _connectionCount); reservation.Keep(); return connection; } @@ -162,6 +174,7 @@ internal bool TryRemove(DbConnectionInternal connection) { if (Interlocked.CompareExchange(ref _connections[i], null, connection) == connection) { + Interlocked.Decrement(ref _connectionCount); ReleaseReservation(); return true; } @@ -170,6 +183,48 @@ 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; + } + + /// + /// Returns a point-in-time snapshot of the connections currently tracked by this collection. + /// The snapshot is best-effort: connections may be added or removed while it is being taken, + /// so callers must tolerate entries that have since left the pool. Intended for infrequent + /// bookkeeping passes (e.g. reclaiming emancipated connections), not for hot paths. + /// + internal List Snapshot() + { + List snapshot = new(_connections.Length); + + for (int i = 0; i < _connections.Length; i++) + { + DbConnectionInternal? connection = Volatile.Read(ref _connections[i]); + if (connection is not null) + { + snapshot.Add(connection); + } + } + + return snapshot; + } + /// /// 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 3799cf54ea..92cc3cbcfe 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 @@ -49,6 +49,19 @@ internal interface IDbConnectionPool /// TODO: rename to indicate that this relates to the blocking period bool ErrorOccurred { get; } + /// + /// The exception thrown by the most recent failed attempt to open a physical connection, + /// or null if no attempt has failed since the last successful open. + /// + /// A caller that waits for a pooled connection and ultimately times out cannot otherwise + /// tell whether the pool was merely saturated or whether every creation attempt behind the + /// scenes was failing (e.g. the server refused the TCP connection). This property lets the + /// timeout be reported with the underlying failure attached as an inner exception. It is + /// diagnostic only and is not used to make control-flow decisions. + /// + /// + Exception? LastConnectionCreateException { get; } + /// /// An id that uniqely identifies this connection pool. /// diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs index 348ad33d9e..e263ae77b7 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs @@ -56,6 +56,7 @@ internal bool TryWrite(DbConnectionInternal? connection) if (connection is not null) { Interlocked.Increment(ref _count); + SqlClientDiagnostics.Metrics.EnterFreeConnection(); } return true; } @@ -74,6 +75,7 @@ internal bool TryRead(out DbConnectionInternal? connection) if (connection is not null) { Interlocked.Decrement(ref _count); + SqlClientDiagnostics.Metrics.ExitFreeConnection(); } return true; @@ -93,6 +95,7 @@ internal bool TryRead(out DbConnectionInternal? connection) if (connection is not null) { Interlocked.Decrement(ref _count); + SqlClientDiagnostics.Metrics.ExitFreeConnection(); } return connection; 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 00ddb01c26..43aa53dc2c 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 @@ -195,6 +195,15 @@ public void Dispose() private readonly TimeProvider _timeProvider; private readonly BlockingPeriodErrorState _errorState; + /// + /// The exception from the most recent failed physical connection open, retained purely so + /// that a subsequent pooled-open timeout can report it as an inner exception. Cleared on the + /// next successful open. Volatile rather than lock-protected: this is a best-effort + /// diagnostic snapshot, and a torn read across concurrent failures would at worst attach a + /// slightly older failure. See GH#3545. + /// + private volatile Exception _lastConnectionCreateException; + internal Timer _cleanupTimer; private readonly TransactedConnectionPool _transactedConnectionPool; @@ -288,6 +297,9 @@ private int CreationTimeout public bool ErrorOccurred => _errorState.HasError; + /// + public Exception LastConnectionCreateException => _lastConnectionCreateException; + private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; public TimeSpan LoadBalanceTimeout => PoolGroupOptions.LoadBalanceTimeout; @@ -551,6 +563,10 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Added to pool.", Id, newObj?.ObjectID); + // A successful open proves the server is reachable, so a previously recorded + // failure is no longer a useful explanation for a later timeout. See GH#3545. + _lastConnectionCreateException = null; + // A successful creation clears any prior error state and resets backoff. _errorState.Clear(); } @@ -558,6 +574,12 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio { ADP.TraceExceptionWithoutRethrow(e); + // Retain the failure so a caller that ultimately times out waiting for a pooled + // connection can report why creation kept failing. Recorded before the + // blocking-period check below so it is captured even when blocking is disabled + // and this method rethrows immediately. See GH#3545. + _lastConnectionCreateException = e; + if (!_connectionPoolGroup.IsBlockingPeriodEnabled()) { throw; @@ -809,7 +831,8 @@ private void WaitForPendingOpen() } else if (timeout) { - next.Completion.TrySetException(ADP.ExceptionWithStackTrace(ADP.PooledOpenTimeout())); + next.Completion.TrySetException( + ADP.ExceptionWithStackTrace(ADP.PooledOpenTimeout(_lastConnectionCreateException))); } else { 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/Microsoft/Data/SqlClient/SqlConnectionFactory.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs index 8840864958..b041453269 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs @@ -337,6 +337,12 @@ internal bool TryGetConnection( int retriesLeft = 10; int timeBetweenRetriesMilliseconds = 1; + // Tracks the most recent physical connection failure observed by the pool we last + // consulted, so a pooled-open timeout can report it as an inner exception rather than + // only reporting pool exhaustion. Hoisted out of the loop because the final give-up + // throw below is outside the pool variable's scope. See GH#3545. + Exception lastConnectionCreateException = null; + do { DbConnectionPoolGroup poolGroup = GetConnectionPoolGroup(owningConnection); @@ -436,12 +442,14 @@ internal bool TryGetConnection( if (connection is null) { + lastConnectionCreateException = connectionPool.LastConnectionCreateException; + // connection creation failed on semaphore waiting or if max pool reached if (connectionPool.IsRunning) { SqlClientEventSource.Log.TryTraceEvent(" {0}, GetConnection failed because a pool timeout occurred.", ObjectId); // If GetConnection failed while the pool is running, the pool timeout occurred. - throw ADP.PooledOpenTimeout(); + throw ADP.PooledOpenTimeout(lastConnectionCreateException); } // We've hit the race condition, where the pool was shut down after we @@ -458,7 +466,7 @@ internal bool TryGetConnection( { SqlClientEventSource.Log.TryTraceEvent(" {0}, GetConnection failed because a pool timeout occurred and all retries were exhausted.", ObjectId); // exhausted all retries or timed out - give up - throw ADP.PooledOpenTimeout(); + throw ADP.PooledOpenTimeout(lastConnectionCreateException); } return true; 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/Common/ConnectionPoolVersionScope.cs b/src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs new file mode 100644 index 0000000000..73cf652a71 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs @@ -0,0 +1,62 @@ +// 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. + +namespace Microsoft.Data.SqlClient.Tests.Common; + +/// +/// Selects the connection pool implementation (WaitHandleDbConnectionPool or +/// ChannelDbConnectionPool) for the duration of a test. +/// +/// A pool is bound to an implementation when it is created, so simply flipping the +/// UseConnectionPoolV2 switch is not enough: pools created before the switch was flipped +/// keep their original implementation, and pools created inside the scope would otherwise outlive +/// it and leak the chosen implementation into unrelated tests. This scope therefore clears all +/// pools both on entry and on exit. +/// +/// This follows the RAII pattern; construct it at the start of a test and dispose it at the end. +/// Like , it manipulates global state and enforces a +/// single-instance policy, so it must not be held for longer than necessary. +/// +public sealed class ConnectionPoolVersionScope : IDisposable +{ + private readonly LocalAppContextSwitchesHelper _switches; + + /// + /// Clears all existing pools and selects the requested pool implementation. + /// + /// + /// True to use ChannelDbConnectionPool; false to use WaitHandleDbConnectionPool. + /// + public ConnectionPoolVersionScope(bool usePoolV2) + { + _switches = new LocalAppContextSwitchesHelper(); + + try + { + SqlConnection.ClearAllPools(); + _switches.UseConnectionPoolV2 = usePoolV2; + } + catch + { + _switches.Dispose(); + throw; + } + } + + /// + /// Clears all pools created under the selected implementation and restores the original + /// switch values. + /// + public void Dispose() + { + try + { + SqlConnection.ClearAllPools(); + } + finally + { + _switches.Dispose(); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs index 4b63ff655f..a3ae028a5d 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs @@ -150,8 +150,7 @@ public static void AccessTokenConnectionPoolingTest() [ClassData(typeof(ConnectionPoolConnectionStringAndPoolVersionProvider))] public static void ClearAllPoolsTest(string connectionString, bool usePoolV2) { - using LocalAppContextSwitchesHelper switchesHelper = new(); - switchesHelper.UseConnectionPoolV2 = usePoolV2; + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); SqlConnection.ClearAllPools(); Assert.True(0 == ConnectionPoolWrapper.AllConnectionPools().Length, "Pools exist after clearing all pools"); @@ -178,9 +177,11 @@ public static void ClearAllPoolsTest(string connectionString, bool usePoolV2) /// NOTE: 'emancipated' means that the internal connection's SqlConnection has fallen out of scope and has no references, but was not explicitly disposed\closed /// [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - [ClassData(typeof(ConnectionPoolConnectionStringProvider))] - public static void ReclaimEmancipatedOnOpenTest(string connectionString) + [ClassData(typeof(ConnectionPoolConnectionStringAndPoolVersionProvider))] + public static void ReclaimEmancipatedOnOpenTest(string connectionString, bool usePoolV2) { + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); + string newConnectionString = (new SqlConnectionStringBuilder(connectionString) { MaxPoolSize = 1 }).ConnectionString; SqlConnection.ClearAllPools(); @@ -205,9 +206,11 @@ public static void ReclaimEmancipatedOnOpenTest(string connectionString) /// Tests if, when max pool size is reached, Open() will block until a connection becomes available /// [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - [ClassData(typeof(ConnectionPoolConnectionStringProvider))] - public static void MaxPoolWaitForConnectionTest(string connectionString) + [ClassData(typeof(ConnectionPoolConnectionStringAndPoolVersionProvider))] + public static void MaxPoolWaitForConnectionTest(string connectionString, bool usePoolV2) { + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); + string newConnectionString = (new SqlConnectionStringBuilder(connectionString) { MaxPoolSize = 1 }).ConnectionString; SqlConnection.ClearAllPools(); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs index fbbee8db26..52a34f4b52 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs @@ -8,6 +8,7 @@ using System.Diagnostics; using System.Threading; using System.Threading.Tasks; +using Microsoft.Data.SqlClient.Tests.Common; using Xunit; namespace Microsoft.Data.SqlClient.ManualTesting.Tests @@ -270,9 +271,13 @@ public static void ConnectionKilledTest() } // Synapse: KILL not supported on Azure Synapse - Parse error at line: 1, column: 6: Incorrect syntax near '105'. - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse), nameof(DataTestUtility.IsNotManagedInstance))] - public static void ConnectionResiliencySPIDTest() + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse), nameof(DataTestUtility.IsNotManagedInstance))] + [InlineData(false)] + [InlineData(true)] + public static void ConnectionResiliencySPIDTest(bool usePoolV2) { + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); + SqlConnectionStringBuilder builder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) { ConnectRetryCount = 0, diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs index a8baee786c..c3891f59e2 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs @@ -66,8 +66,9 @@ public async Task TestPacketNumberWraparound() stopwatch.Start(); Task actionTask = Task.Factory.StartNew( - async () => await RunPacketNumberWraparound(enumerator, cancellationTokenSource.Token), - TaskCreationOptions.DenyChildAttach | TaskCreationOptions.LongRunning); + () => RunPacketNumberWraparound(enumerator, cancellationTokenSource.Token), + TaskCreationOptions.DenyChildAttach | TaskCreationOptions.LongRunning) + .Unwrap(); Task timeoutTask = Task.Delay(TimeSpan.FromSeconds(60), cancellationTokenSource.Token); await Task.WhenAny(actionTask, timeoutTask); diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs b/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs index 6d37051821..ecb6b4b651 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Reflection; using System.Transactions; +using Microsoft.Data.SqlClient.Tests.Common; using Xunit; namespace Microsoft.Data.SqlClient.ManualTesting.Tests @@ -63,9 +64,13 @@ public void NonPooledConnectionsCounters_Functional() } } - [ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] - public void PooledConnectionsCounters_Functional() + [ConditionalTheory(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))] + [InlineData(false)] + [InlineData(true)] + public void PooledConnectionsCounters_Functional(bool usePoolV2) { + using ConnectionPoolVersionScope poolVersion = new(usePoolV2); + //create a pooled connection var stringBuilder = new SqlConnectionStringBuilder(DataTestUtility.TCPConnectionString) { Pooling = true }; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolInstrumentationTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolInstrumentationTest.cs new file mode 100644 index 0000000000..a187316925 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolInstrumentationTest.cs @@ -0,0 +1,679 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Data.Common; +using System.Diagnostics.Tracing; +using System.Globalization; +using System.Linq; +using System.Reflection; +using System.Text.RegularExpressions; +using System.Threading; +using System.Threading.RateLimiting; +using Microsoft.Data.Common; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Data.SqlClient.Diagnostics; +using Microsoft.Data.SqlClient.Tests.Common; +using Xunit; + +using static Microsoft.Data.SqlClient.UnitTests.ConnectionPool.ChannelDbConnectionPoolTest; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Verifies the diagnostic instrumentation of : the pooler + /// trace events emitted across the connection lifecycle, and the last-connection-create + /// exception that is surfaced as the inner exception of a pooled-open timeout (GH#3545). + /// + public class ChannelDbConnectionPoolInstrumentationTest + { + /// + /// Builds a pool for instrumentation tests. Defaults mirror + /// so behavior is comparable across suites. + /// + /// The factory used to create physical connections. + /// Connection string backing the pool group. Tests override + /// it to control the Pool Blocking Period. + /// Maximum pool size. + /// Minimum pool size. + /// Connection Idle Timeout, in seconds. + /// Optional limiter throttling physical creates. + private static ChannelDbConnectionPool ConstructPool( + SqlConnectionFactory connectionFactory, + string connectionString = "Data Source=localhost;", + int maxPoolSize = 50, + int minPoolSize = 0, + int idleTimeout = 0, + ConcurrencyLimiter? connectionCreationRateLimiter = null) + { + DbConnectionPoolGroupOptions poolGroupOptions = new( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: idleTimeout); + + DbConnectionPoolGroup poolGroup = new( + new SqlConnectionOptions(connectionString), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + + return new ChannelDbConnectionPool( + connectionFactory, + poolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo(), + connectionCreationRateLimiter); + } + + #region Trace parity + + /// + /// Verifies that the pool traces its own construction, so a trace capture can attribute + /// every later pool-scoped event to a pool whose creation it observed. + /// + [Fact] + public void Construction_EmitsConstructedTrace() + { + // Arrange + using PoolerTraceListener listener = new(); + + // Act + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + + // Assert + TraceAssert.Contains("Constructed.", listener.MessagesForPool(pool.Id)); + } + + /// + /// Verifies that creating a new physical connection is traced, covering Story 1 scenario 2. + /// + [Fact] + public void NewConnection_EmitsCreationTraces() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + using PoolerTraceListener listener = new(); + + // Act + Assert.True(pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection)); + + // Assert + IReadOnlyList messages = listener.MessagesForPool(pool.Id); + Assert.NotNull(connection); + TraceAssert.Contains("Getting connection.", messages); + TraceAssert.Contains("Creating new connection.", messages); + TraceAssert.Contains("Added to pool.", messages); + } + + /// + /// Verifies that retrieving a connection from the idle pool is traced, covering Story 1 + /// scenario 1. + /// + [Fact] + public void IdleConnectionReuse_EmitsPoppedFromGeneralPoolTrace() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + pool.ReturnInternalConnection(connection!, owner); + + using PoolerTraceListener listener = new(); + + // Act + Assert.True(pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? reused)); + + // Assert + Assert.Same(connection, reused); + TraceAssert.Contains("Popped from general pool.", listener.MessagesForPool(pool.Id)); + } + + /// + /// Verifies that returning a connection traces both the deactivation and the routing + /// decision that put it back into the idle pool, covering Story 1 scenario 3. + /// + [Fact] + public void Return_EmitsDeactivateAndPushTraces() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + + using PoolerTraceListener listener = new(); + + // Act + pool.ReturnInternalConnection(connection!, owner); + + // Assert + IReadOnlyList messages = listener.MessagesForPool(pool.Id); + TraceAssert.Contains("Deactivating.", messages); + TraceAssert.Contains("Pushing to general pool.", messages); + } + + /// + /// Verifies that destroying a connection traces the removal and the disposal, covering + /// Story 1 scenario 4. Clear is used as the destruction trigger because it drains the idle + /// channel through the same removal path as every other destroy. + /// + [Fact] + public void Destroy_EmitsRemoveAndDisposeTraces() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + pool.ReturnInternalConnection(connection!, owner); + + using PoolerTraceListener listener = new(); + + // Act + pool.Clear(); + + // Assert + IReadOnlyList messages = listener.MessagesForPool(pool.Id); + TraceAssert.Contains("Clearing.", messages); + TraceAssert.Contains("Removing from pool.", messages); + TraceAssert.Contains("Removed from pool.", messages); + TraceAssert.Contains("Disposed.", messages); + TraceAssert.Contains("Cleared.", messages); + } + + /// + /// Verifies that startup and shutdown are traced with the pool identifier, covering Story 1 + /// scenario 5. + /// + [Fact] + public void StartupAndShutdown_EmitTraces() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + using PoolerTraceListener listener = new(); + + // Act + pool.Startup(); + pool.Shutdown(); + + // Assert + IReadOnlyList messages = listener.MessagesForPool(pool.Id); + Assert.Contains(messages, m => m.IndexOf("Startup", StringComparison.Ordinal) >= 0); + Assert.Contains(messages, m => m.IndexOf("Shutdown", StringComparison.Ordinal) >= 0); + } + + /// + /// Verifies that a failed physical open is traced on the pool's create path, so an operator + /// can see why the pool stopped growing. + /// + [Fact] + public void CreateFailure_EmitsCreateThrewTrace() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new FailingSqlConnectionFactory()); + using PoolerTraceListener listener = new(); + + // Act + Assert.ThrowsAny(() => + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + + // Assert + TraceAssert.Contains("which threw an exception", listener.MessagesForPool(pool.Id)); + } + + /// + /// Verifies that a connection discarded for exceeding the Connection Idle Timeout is traced + /// with that specific reason, rather than silently disappearing from the pool. + /// + [Fact] + public void IdleTimeoutEviction_EmitsReasonTrace() + { + // Arrange - idle-timeout eviction is opt-in; the switch defaults to legacy behavior. + using LocalAppContextSwitchesHelper switchesHelper = new(); + switchesHelper.UseLegacyIdleTimeoutBehavior = false; + + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory(), idleTimeout: 1); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + pool.ReturnInternalConnection(connection!, owner); + + // Back-date the return stamp only after the connection is parked in the idle channel: + // the return path re-stamps it so that time spent checked out is not counted as idle. + connection!.SetReturnedTime(DateTime.UtcNow - TimeSpan.FromMinutes(5)); + + using PoolerTraceListener listener = new(); + + // Act - retrieval trips the idle-expiry gate and discards the connection. + Assert.True(pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? replacement)); + + // Assert + Assert.NotNull(replacement); + Assert.NotSame(connection, replacement); + TraceAssert.Contains("exceeded the connection idle timeout and removed.", listener.MessagesForPool(pool.Id)); + } + + /// + /// Verifies that pruning traces each invocation, so idle reclamation is attributable in a + /// trace capture even when it removes nothing (Story 3). + /// + [Fact] + public void Prune_EmitsTracePerInvocation() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory(), maxPoolSize: 4, idleTimeout: 300); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + pool.ReturnInternalConnection(connection!, owner); + + using PoolerTraceListener listener = new(); + + // Act + pool.PruneConnections(1); + + // Assert + IReadOnlyList messages = listener.MessagesForPool(pool.Id); + TraceAssert.Contains("Pruning up to 1 idle connections.", messages); + TraceAssert.Contains("Pruned 1 idle connections.", messages); + } + + #endregion + +#if NET + #region Metric parity + + // The metric counters are process-wide and other suites run in parallel, so these tests + // assert that a counter advanced by at least the expected amount rather than by exactly it. + // The rate counters only ever increase, which makes that assertion stable under concurrency. + + /// + /// Verifies that retrieving an idle connection counts a soft connect, covering Story 2 + /// scenario 3. + /// + [Fact] + public void IdleConnectionReuse_CountsSoftConnect() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + pool.ReturnInternalConnection(connection!, owner); + + long before = MetricReader.Read("_softConnectsRate"); + + // Act + Assert.True(pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? reused)); + + // Assert + Assert.Same(connection, reused); + Assert.True(MetricReader.Read("_softConnectsRate") >= before + 1); + } + + /// + /// Verifies that returning a connection to the idle pool counts a soft disconnect, covering + /// Story 2 scenario 4. + /// + [Fact] + public void Return_CountsSoftDisconnect() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + + long before = MetricReader.Read("_softDisconnectsRate"); + + // Act + pool.ReturnInternalConnection(connection!, owner); + + // Assert + Assert.True(MetricReader.Read("_softDisconnectsRate") >= before + 1); + } + + /// + /// Verifies that destroying a physical connection counts a hard disconnect, covering Story 2 + /// scenario 2. + /// + [Fact] + public void Destroy_CountsHardDisconnect() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + Assert.NotNull(connection); + pool.ReturnInternalConnection(connection!, owner); + + long before = MetricReader.Read("_hardDisconnectsRate"); + + // Act + pool.Clear(); + + // Assert + Assert.True(MetricReader.Read("_hardDisconnectsRate") >= before + 1); + } + + /// + /// Verifies that replacing a connection counts a hard disconnect for the connection it + /// discards. The channel pool swaps the new connection into the old connection's slot, so + /// the pooled-connection gauge is deliberately left untouched. + /// + [Fact] + public void ReplaceConnection_CountsHardDisconnectForDiscardedConnection() + { + // Arrange + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + SqlConnection owner = new(); + Assert.True(pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? oldConnection)); + Assert.NotNull(oldConnection); + + long beforeDisconnects = MetricReader.Read("_hardDisconnectsRate"); + long beforePooled = MetricReader.Read("_pooledConnections"); + + // Act + DbConnectionInternal newConnection = pool.ReplaceConnection(owner, oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert + Assert.NotSame(oldConnection, newConnection); + Assert.True(MetricReader.Read("_hardDisconnectsRate") >= beforeDisconnects + 1); + Assert.Equal(beforePooled, MetricReader.Read("_pooledConnections")); + } + + #endregion +#endif + + #region Last connection create exception (GH#3545) + + /// + /// Verifies that a pool that has never attempted a physical open reports no create failure, + /// so a timeout from a genuinely saturated pool is not annotated with a stale cause. + /// + [Fact] + public void LastConnectionCreateException_NoAttempts_IsNull() + { + // Arrange / Act + ChannelDbConnectionPool pool = ConstructPool(new SuccessfulSqlConnectionFactory()); + + // Assert + Assert.Null(pool.LastConnectionCreateException); + } + + /// + /// Verifies that a failed physical open is retained on the pool and then discarded once a + /// later open succeeds, so the recorded cause never outlives its relevance. + /// + [Fact] + public void LastConnectionCreateException_RecordedOnFailure_ClearedOnSuccess() + { + // Arrange - NeverBlock keeps the pool out of the blocking period so the second request + // actually attempts another physical open instead of fast-failing on cached state. + ToggleableConnectionFactory factory = new() { ShouldFail = true }; + ChannelDbConnectionPool pool = ConstructPool( + factory, + connectionString: "Data Source=localhost;Pool Blocking Period=NeverBlock;"); + + // Act - a failed open records the cause. + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + + // Assert + Assert.IsType(pool.LastConnectionCreateException); + + // Act - a successful open proves the server is reachable and clears the cause. + factory.ShouldFail = false; + Assert.True(pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? connection)); + + // Assert + Assert.NotNull(connection); + Assert.Null(pool.LastConnectionCreateException); + } + + /// + /// Verifies GH#3545 end to end for this pool: when a request waits for a pooled connection + /// and times out, the most recent physical connection failure is attached as the inner + /// exception instead of being lost behind the generic pool-exhaustion message. + /// + [Fact] + public void PooledOpenTimeout_CarriesLastCreateExceptionAsInner() + { + // Arrange - a single-permit limiter lets the test hold the only creation permit, so the + // second request cannot attempt an open and must wait on the idle channel until its + // budget expires. NeverBlock keeps the pool out of the blocking period, which would + // otherwise fast-fail the second request with the cached exception directly. + ToggleableConnectionFactory factory = new() { ShouldFail = true }; + using ConcurrencyLimiter rateLimiter = new( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); + ChannelDbConnectionPool pool = ConstructPool( + factory, + connectionString: "Data Source=localhost;Pool Blocking Period=NeverBlock;", + maxPoolSize: 4, + connectionCreationRateLimiter: rateLimiter); + + // The first request fails its physical open, which records the cause on the pool. + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + Assert.IsType(pool.LastConnectionCreateException); + + // Hold the only permit so no further creation can be attempted. + using RateLimitLease lease = rateLimiter.AttemptAcquire(1); + Assert.True(lease.IsAcquired); + + // Act + InvalidOperationException timeout = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromMilliseconds(100)), out _)); + + // Assert + Assert.IsType(timeout.InnerException); + } + + /// + /// Verifies that a timeout with no preceding create failure still reports the plain + /// pool-exhaustion message, so the change does not fabricate a cause. + /// + [Fact] + public void PooledOpenTimeout_NoCreateFailure_HasNoInnerException() + { + // Arrange - hold the only creation permit up front so no open is ever attempted. + using ConcurrencyLimiter rateLimiter = new( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); + ChannelDbConnectionPool pool = ConstructPool( + new SuccessfulSqlConnectionFactory(), + maxPoolSize: 4, + connectionCreationRateLimiter: rateLimiter); + + using RateLimitLease lease = rateLimiter.AttemptAcquire(1); + Assert.True(lease.IsAcquired); + + // Act + InvalidOperationException timeout = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromMilliseconds(100)), out _)); + + // Assert + Assert.Null(timeout.InnerException); + } + + #endregion + + #region Test classes + + /// + /// Distinctive exception type used to prove that the exact failure recorded by the pool is + /// the one attached to the pooled-open timeout. + /// + internal sealed class TestConnectionCreateException : Exception + { + internal TestConnectionCreateException() + : base("Simulated physical connection failure.") + { + } + } + + /// + /// Connection factory whose success or failure can be flipped between requests, so a single + /// pool can be driven through a failure and a subsequent recovery. + /// + internal sealed class ToggleableConnectionFactory : SqlConnectionFactory + { + /// + /// When true, the next creation attempt throws . + /// + internal volatile bool ShouldFail; + + /// + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (ShouldFail) + { + throw new TestConnectionCreateException(); + } + + return new StubDbConnectionInternal(); + } + } + + /// + /// Connection factory that always fails with . + /// + internal sealed class FailingSqlConnectionFactory : SqlConnectionFactory + { + /// + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + => throw new TestConnectionCreateException(); + } + + /// + /// Captures PoolerTrace events from the SqlClient event source. + /// + /// Tests filter captured messages by pool id (see ) because the + /// event source is process-wide: xUnit runs test classes in parallel, so traces from other + /// pools are expected to appear in the same capture. + /// + /// + internal sealed class PoolerTraceListener : EventListener + { + private const string SqlClientEventSourceName = "Microsoft.Data.SqlClient.EventSource"; + + // Mirrors SqlClientEventSource.Keywords.PoolerTrace. Duplicated as a literal because + // that type is not visible to this assembly. + private const EventKeywords PoolerTraceKeyword = (EventKeywords)32; + + // Lazily initialized: EventListener's base constructor invokes OnEventSourceCreated, + // which enables events, before this class's field initializers have run. Traces can + // therefore arrive on another thread before a plain field initializer would have + // assigned the queue. + private ConcurrentQueue? _messages; + + private ConcurrentQueue Messages => + LazyInitializer.EnsureInitialized(ref _messages)!; + + /// + protected override void OnEventSourceCreated(EventSource eventSource) + { + if (eventSource.Name == SqlClientEventSourceName) + { + EnableEvents(eventSource, EventLevel.Informational, PoolerTraceKeyword); + } + } + + /// + protected override void OnEventWritten(EventWrittenEventArgs eventData) + { + if (eventData.Payload is null) + { + return; + } + + foreach (object? payload in eventData.Payload) + { + if (payload is string message) + { + Messages.Enqueue(message); + } + } + } + + /// + /// Returns the captured messages emitted for the given pool. + /// + /// The to filter on. + internal IReadOnlyList MessagesForPool(int poolId) + { + // Pool-scoped traces render the id as the first substituted argument, immediately + // after the "|CPOOL> " marker, e.g. + // " 7, Clearing." + // " 7" + // The trailing boundary keeps pool 7 from matching pool 70. + Regex pattern = new( + @"CPOOL> " + Regex.Escape(poolId.ToString(CultureInfo.InvariantCulture)) + @"(\D|$)", + RegexOptions.CultureInvariant); + + return Messages.Where(m => pattern.IsMatch(m)).ToList(); + } + } + + #endregion + } + +#if NET + /// + /// Reads the private counter fields of the process-wide instance. + /// The counters are not otherwise observable without an EventCounter listener and its polling + /// interval, which would make these tests slow and timing dependent. + /// + internal static class MetricReader + { + /// + /// Reads the current value of the named counter field. + /// + /// Private field name declared on . + internal static long Read(string fieldName) + { + FieldInfo? field = typeof(SqlClientMetrics).GetField(fieldName, BindingFlags.Instance | BindingFlags.NonPublic); + Assert.NotNull(field); + return (long)field!.GetValue(SqlClientDiagnostics.Metrics)!; + } + } +#endif + + /// + /// xUnit assertion helper for substring matching over a captured trace stream. + /// + internal static class TraceAssert + { + /// + /// Asserts that at least one captured message contains . + /// + internal static void Contains(string fragment, IReadOnlyList messages) => + Assert.True( + messages.Any(m => m.IndexOf(fragment, StringComparison.Ordinal) >= 0), + $"Expected a trace containing \"{fragment}\". Captured:{Environment.NewLine}{string.Join(Environment.NewLine, messages)}"); + } +} 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..1b22372de2 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -0,0 +1,728 @@ +// 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 + { + + /// + /// 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. + /// + 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;"), + 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(new TunableSqlConnectionFactory()); + 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(new TunableSqlConnectionFactory()); + 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(new TunableSqlConnectionFactory()); + 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(new TunableSqlConnectionFactory(), 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 factory = new TunableSqlConnectionFactory(); + 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 factory = new TunableSqlConnectionFactory(); + 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 factory = new TunableSqlConnectionFactory(); + 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 factory = new TunableSqlConnectionFactory(); + 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(new TunableSqlConnectionFactory()); + 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(new TunableSqlConnectionFactory(), 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 factory = new TunableSqlConnectionFactory(); + 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(new TunableSqlConnectionFactory()); + 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 — localhost is non-Azure, so the pool's blocking period is enabled by default. + var factory = new TunableSqlConnectionFactory(); + 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 — localhost is non-Azure, so the pool's blocking period is enabled by default. + var factory = new TunableSqlConnectionFactory(); + 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 factory = new TunableSqlConnectionFactory(); + 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 9d24200b17..f38e3c2baf 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Data.Common; using System.Threading; using System.Threading.RateLimiting; @@ -250,10 +251,16 @@ public async Task GetConnectionMaxPoolSize_ShouldReuseAfterConnectionReleased() out DbConnectionInternal? firstConnection ); + // The owning connections must stay reachable for the duration of the test. If they were + // collected, their internal connections would become emancipated and the pool would be + // entitled to reclaim them, which would defeat the pool-exhaustion this test relies on. + List owningConnections = new(); for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize; i++) { + SqlConnection owningConnection = new(); + owningConnections.Add(owningConnection); var completed = pool.TryGetConnection( - new SqlConnection(), + owningConnection, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection @@ -280,6 +287,8 @@ out DbConnectionInternal? extraConnection // Assert Assert.Equal(firstConnection, extraConnection); + + GC.KeepAlive(owningConnections); } /// @@ -349,10 +358,16 @@ public async Task GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest() out DbConnectionInternal? firstConnection ); + // The owning connections must stay reachable for the duration of the test. If they were + // collected, their internal connections would become emancipated and the pool would be + // entitled to reclaim them, which would defeat the pool exhaustion this test relies on. + List owningConnections = new(); for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize; i++) { + SqlConnection owningConnection = new(); + owningConnections.Add(owningConnection); var completed = pool.TryGetConnection( - new SqlConnection(), + owningConnection, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection @@ -402,6 +417,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => await failedTask); + + GC.KeepAlive(owningConnections); } /// @@ -423,10 +440,16 @@ public async Task GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest() out DbConnectionInternal? firstConnection ); + // The owning connections must stay reachable for the duration of the test. If they were + // collected, their internal connections would become emancipated and the pool would be + // entitled to reclaim them, which would defeat the pool exhaustion this test relies on. + List owningConnections = new(); for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize; i++) { + SqlConnection owningConnection = new(); + owningConnections.Add(owningConnection); var completed = pool.TryGetConnection( - new SqlConnection(), + owningConnection, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection @@ -464,6 +487,8 @@ out DbConnectionInternal? failedConnection // Assert Assert.Equal(firstConnection, recycledConnection); await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); + + GC.KeepAlive(owningConnections); } /// @@ -625,8 +650,13 @@ public void StressTestAsync() TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? internalConnection ); - internalConnection = await taskCompletionSource.Task; - pool.ReturnInternalConnection(internalConnection, owningObject); + // The pool may satisfy the request synchronously from the idle channel, in + // which case the task completion source is never signalled. + if (!completed) + { + internalConnection = await taskCompletionSource.Task; + } + pool.ReturnInternalConnection(internalConnection!, owningObject); Assert.NotNull(internalConnection); }); @@ -858,49 +888,33 @@ public void TestUseLoadBalancing() #endregion - #region Not Implemented Method Tests - - /// - /// Verifies that remains - /// unimplemented and throws . - /// - [Fact] - public void TestPutObjectFromTransactedPool() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - - // Act & Assert - Assert.Throws(() => pool.PutObjectFromTransactedPool(null!)); - } + #region Replace Connection Tests /// /// Verifies that - /// remains unimplemented and throws . + /// replaces a checked-out connection with a new, distinct connection instance. /// [Fact] 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.ReplaceConnection(null!, null!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); - } + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); - /// - /// Verifies that - /// remains unimplemented and throws . - /// - [Fact] - public void TestTransactionEnded() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); + Assert.NotNull(oldConnection); - // Act & Assert - Assert.Throws(() => pool.TransactionEnded(null!, null!)); + var newConnection = pool.ReplaceConnection(owner, oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); } + #endregion #region Pool Clear Tests @@ -2353,3 +2367,4 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } } + diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs new file mode 100644 index 0000000000..53667f418d --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -0,0 +1,1169 @@ +// 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; +using System.Data.Common; +using System.Threading; +using System.Threading.Tasks; +using System.Transactions; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool; + +/// +/// Deterministic tests for ChannelDbConnectionPool transaction functionality. +/// These tests exercise transacted connection pathways with controlled synchronization +/// to verify correct behavior without relying on probabilistic concurrency. +/// +public class ChannelDbConnectionPoolTransactionTest : IDisposable +{ + private const int DefaultMaxPoolSize = 50; + private const int DefaultMinPoolSize = 0; + private const int DefaultCreationTimeoutInMilliseconds = 15000; + + private IDbConnectionPool _pool = null!; + + public ChannelDbConnectionPoolTransactionTest() + { + _pool = CreatePool(); + } + + public void Dispose() + { + // Verify no leaked transactions before cleanup + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + + _pool?.Shutdown(); + _pool?.Clear(); + } + + #region Helper Methods + + private ChannelDbConnectionPool CreatePool( + int maxPoolSize = DefaultMaxPoolSize, + int minPoolSize = DefaultMinPoolSize, + bool hasTransactionAffinity = true) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: minPoolSize, + maxPoolSize: maxPoolSize, + creationTimeout: DefaultCreationTimeoutInMilliseconds, + loadBalanceTimeout: 0, + hasTransactionAffinity: hasTransactionAffinity, + idleTimeout: 0 + ); + + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + + var connectionFactory = new MockSqlConnectionFactory(); + + var pool = new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo() + ); + + pool.Startup(); + return pool; + } + + private DbConnectionInternal GetConnection(SqlConnection owner) + { + _pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + return connection!; + } + + private async Task GetConnectionAsync( + SqlConnection owner, + Transaction? transaction = null) + { + var tcs = new TaskCompletionSource(transaction); + _pool.TryGetConnection( + owner, + taskCompletionSource: tcs, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + return connection ?? await tcs.Task; + } + + private void ReturnConnection(DbConnectionInternal connection, SqlConnection owner) + { + _pool.ReturnInternalConnection(connection, owner); + } + + private void AssertPoolMetrics() + { + Assert.True(_pool.Count <= _pool.PoolGroupOptions.MaxPoolSize, + $"Pool count ({_pool.Count}) exceeded max pool size ({_pool.PoolGroupOptions.MaxPoolSize})"); + Assert.True(_pool.Count >= 0, + $"Pool count ({_pool.Count}) is negative"); + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + } + + #endregion + + #region Transaction Routing Tests + + [Fact] + public void GetConnection_UnderTransaction_RoutesToTransactedPool() + { + // Arrange & Act + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + ReturnConnection(conn, owner); + + // Assert - connection should be in the transacted pool + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public void GetConnection_WithoutTransaction_RoutesToGeneralPool() + { + // Arrange & Act (no TransactionScope) + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + ReturnConnection(conn, owner); + + // Assert - transacted pool should be empty + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + } + + [Fact] + public void GetConnection_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + { + // Arrange + using var scope = new TransactionScope(); + + // Act - first call creates a new connection + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Second call should retrieve the SAME connection from the transacted pool (LIFO) + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); + + ReturnConnection(conn2, owner2); + scope.Complete(); + } + + [Fact] + public async Task GetConnectionAsync_UnderTransaction_ReturnsSameConnectionFromTransactedPool() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + + // Act - first call creates a new connection + var owner1 = new SqlConnection(); + var conn1 = await GetConnectionAsync(owner1, transaction: transaction); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Second call should retrieve the SAME connection from the transacted pool + var owner2 = new SqlConnection(); + var conn2 = await GetConnectionAsync(owner2, transaction: transaction); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); + + ReturnConnection(conn2, owner2); + scope.Complete(); + } + + [Fact] + public void GetConnection_WithTransactionAffinityDisabled_SkipsTransactedPool() + { + // Arrange + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(hasTransactionAffinity: false); + + using var scope = new TransactionScope(); + + // Act + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Assert - even though a transaction is active, transacted pool is not used + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + + scope.Complete(); + } + + #endregion + + #region Transaction Lifecycle Tests + + [Fact] + public void TransactionCommit_ClearsTransactedPool() + { + // Arrange & Act + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // While transaction is active, connection should be in transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + scope.Complete(); + } + + // Assert - after transaction completes, transacted pool should be empty + AssertPoolMetrics(); + } + + [Fact] + public void TransactionRollback_ClearsTransactedPool() + { + // Arrange & Act + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + // Don't call scope.Complete() — triggers rollback + } + + // Assert - transacted pool should be empty after rollback too + AssertPoolMetrics(); + } + + [Fact] + public void MultipleGetReturn_SameTransaction_ReusesConnection() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - get and return multiple times within same transaction + for (int i = 0; i < 10; i++) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + + // Assert - only one connection should be in the transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public async Task MultipleGetReturn_SameTransaction_Async_ReusesConnection() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - get and return multiple times within same transaction + for (int i = 0; i < 10; i++) + { + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + + // Assert - only one connection should be in the transacted pool + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + [Fact] + public void AlternatingCommitAndRollback_MaintainsConsistentState() + { + // Act - alternate between commit and rollback + for (int i = 0; i < 20; i++) + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + if (i % 2 == 0) + { + scope.Complete(); + } + // else: rollback (no Complete) + } + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Nested Transaction Tests + + [Fact] + public void NestedTransaction_Required_SharesSameTransactedEntry() + { + // Arrange + using var outerScope = new TransactionScope(); + var outerTxn = Transaction.Current; + Assert.NotNull(outerTxn); + + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Act - nested scope with Required shares the same transaction + using (var innerScope = new TransactionScope(TransactionScopeOption.Required)) + { + Assert.Same(outerTxn, Transaction.Current); + + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); // Same transaction -> same connection from transacted pool + ReturnConnection(conn2, owner2); + + // Only one transaction tracked + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + innerScope.Complete(); + } + + outerScope.Complete(); + } + + [Fact] + public void NestedTransaction_RequiresNew_CreatesSeparateTransactedEntry() + { + // Arrange + using var outerScope = new TransactionScope(); + var outerTxn = Transaction.Current; + Assert.NotNull(outerTxn); + + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Act - nested scope with RequiresNew creates a new transaction + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) + { + var innerTxn = Transaction.Current; + Assert.NotNull(innerTxn); + Assert.NotEqual(outerTxn, innerTxn); + + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.NotSame(conn1, conn2); // Different transaction -> different connection + ReturnConnection(conn2, owner2); + + // Two separate transactions tracked + Assert.Equal(2, _pool.TransactedConnectionPool.TransactedConnections.Count); + + innerScope.Complete(); + } + + outerScope.Complete(); + } + + [Fact] + public void NestedTransaction_RequiresNew_CompletesIndependently() + { + // Arrange & Act + using (var outerScope = new TransactionScope()) + { + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + using (var innerScope = new TransactionScope(TransactionScopeOption.RequiresNew)) + { + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + ReturnConnection(conn2, owner2); + innerScope.Complete(); + } + + // Inner transaction completed - its entry should be cleared + // Outer transaction entry should still exist + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + outerScope.Complete(); + } + + // Both completed + AssertPoolMetrics(); + } + + [Fact] + public void DeeplyNestedTransactions_RequiresNew_AllTrackedSeparately() + { + // Arrange & Act + using var scope1 = new TransactionScope(); + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + ReturnConnection(conn1, owner1); + + using var scope2 = new TransactionScope(TransactionScopeOption.RequiresNew); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + ReturnConnection(conn2, owner2); + + using var scope3 = new TransactionScope(TransactionScopeOption.RequiresNew); + var owner3 = new SqlConnection(); + var conn3 = GetConnection(owner3); + ReturnConnection(conn3, owner3); + + // Assert - three separate transactions tracked + Assert.Equal(3, _pool.TransactedConnectionPool.TransactedConnections.Count); + + scope3.Complete(); + scope2.Complete(); + scope1.Complete(); + } + + [Fact] + public void DeeplyNestedTransactions_Required_AllShareOneEntry() + { + // Arrange & Act + using var scope1 = new TransactionScope(); + var txn = Transaction.Current; + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + ReturnConnection(conn1, owner1); + + using var scope2 = new TransactionScope(TransactionScopeOption.Required); + Assert.Same(txn, Transaction.Current); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn1, conn2); + ReturnConnection(conn2, owner2); + + using var scope3 = new TransactionScope(TransactionScopeOption.Required); + Assert.Same(txn, Transaction.Current); + var owner3 = new SqlConnection(); + var conn3 = GetConnection(owner3); + Assert.Same(conn1, conn3); + ReturnConnection(conn3, owner3); + + // Assert - single transaction entry + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + + scope3.Complete(); + scope2.Complete(); + scope1.Complete(); + } + + #endregion + + #region Mixed Transacted and Non-Transacted Tests + + [Fact] + public void MixedWorkload_AlternatingTransactedAndNonTransacted() + { + // Act - alternate between transacted and non-transacted + for (int i = 0; i < 10; i++) + { + if (i % 2 == 0) + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + scope.Complete(); + } + else + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + } + } + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Shared Transaction Tests + + [Fact] + public void SharedTransaction_DependentScopes_UseTransactedPool() + { + // Arrange + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - first connection + var owner1 = new SqlConnection(); + var conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + + // Use dependent scope on same transaction + using (var innerScope = new TransactionScope(transaction)) + { + Assert.Same(transaction, Transaction.Current); + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + Assert.Same(conn1, conn2); // Same transaction -> same connection + ReturnConnection(conn2, owner2); + innerScope.Complete(); + } + + // Assert - still one transaction entry + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + + scope.Complete(); + } + + #endregion + + #region Pool Saturation with Transactions Tests + + [Fact] + public void PoolSaturation_BlocksUntilConnectionAvailable() + { + // Arrange - small pool + _pool.Shutdown(); + _pool.Clear(); + _pool = CreatePool(maxPoolSize: 1); + + using var allAcquired = new ManualResetEventSlim(false); + using var releaseFirst = new ManualResetEventSlim(false); + + var saturatingTask = Task.Run(() => + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + allAcquired.Set(); // Signal that this connection is held + + Assert.True(releaseFirst.Wait(TimeSpan.FromSeconds(15)), + "Timed out waiting for releaseFirst signal."); + + ReturnConnection(conn, owner); + scope.Complete(); + }); + + Assert.True(allAcquired.Wait(TimeSpan.FromSeconds(10)), + "Timed out waiting for connection to be acquired."); + Assert.Equal(1, _pool.Count); + + using var acquired = new ManualResetEventSlim(false); + var waitingTask = Task.Run(() => + { + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + acquired.Set(); + ReturnConnection(conn, owner); + scope.Complete(); + }); + + // Give the waiting task time to block — it should NOT complete yet + Assert.False(acquired.Wait(TimeSpan.FromMilliseconds(500)), + "Waiting task should not have acquired a connection while pool is saturated"); + + // Release one connection to unblock the waiting task + releaseFirst.Set(); + + // Now the waiting task should complete + Assert.True(waitingTask.Wait(TimeSpan.FromSeconds(15)), + "Waiting task should have completed after a connection was released"); + Assert.True(acquired.IsSet); + + // Cleanup remaining held connections + Task.WaitAll(saturatingTask); + } + + #endregion + + #region Controlled Concurrency Tests + + // Flaky under CI load only (never reproduces locally): the two worker tasks are + // scheduled via Task.Run on the thread pool. On a loaded agent the pool can be slow to + // spin up a worker, so task1 starts late and fails to signal task1Returned within + // task2's 10s wait, producing a WaitAll timeout. That is thread-pool starvation, not a + // pool/transaction defect. + [Trait("Category", "flaky")] + [Fact] + public void TwoThreads_SharedTransaction_AccessSameTransactedEntry() + { + // Arrange + // Use 3-phase synchronization so task1 gets AND returns before task2 requests. + // This ensures the connection is back in the transacted pool for task2 to reuse. + using var task1Returned = new ManualResetEventSlim(false); + using var task2Done = new ManualResetEventSlim(false); + DbConnectionInternal? connFromTask1 = null; + DbConnectionInternal? connFromTask2 = null; + + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - two threads sharing the same transaction, sequenced so the + // transacted pool can vend the same connection to both. + var task1 = Task.Run(() => + { + using var innerScope = new TransactionScope(transaction); + var owner = new SqlConnection(); + connFromTask1 = GetConnection(owner); + Assert.NotNull(connFromTask1); + + // Return the connection so it's available in the transacted pool + ReturnConnection(connFromTask1, owner); + innerScope.Complete(); + + task1Returned.Set(); // Signal: connection is back in the transacted pool + }); + + var task2 = Task.Run(() => + { + // Wait until task1 has returned the connection to the transacted pool + Assert.True(task1Returned.Wait(TimeSpan.FromSeconds(10)), + "Timed out waiting for task1 to return its connection."); + + using var innerScope = new TransactionScope(transaction); + var owner = new SqlConnection(); + connFromTask2 = GetConnection(owner); + Assert.NotNull(connFromTask2); + ReturnConnection(connFromTask2, owner); + innerScope.Complete(); + }); + + Task.WaitAll(task1, task2); + + // Both tasks should have received the same connection via the transacted pool + Assert.Same(connFromTask1, connFromTask2); + scope.Complete(); + } + + [Fact] + public async Task TwoThreads_SeparateTransactions_Async_IsolatedTransactedEntries() + { + // Arrange + using var barrier = new SemaphoreSlim(0, 2); + + // Act + var task1 = Task.Run(async () => + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + barrier.Release(); // Signal ready + await barrier.WaitAsync(); // Wait for other task + + scope.Complete(); + }); + + var task2 = Task.Run(async () => + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + barrier.Release(); // Signal ready + await barrier.WaitAsync(); // Wait for other task + + scope.Complete(); + }); + + await Task.WhenAll(task1, task2); + + // Assert + AssertPoolMetrics(); + } + + #endregion + + #region Pool Shutdown with Transactions Tests + + [Fact] + public void PoolShutdown_AfterTransactionComplete_NoLeaks() + { + // Arrange + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + scope.Complete(); + } + + // Act + _pool.Shutdown(); + + // Assert + AssertPoolMetrics(); + } + + [Fact] + public void PoolShutdown_WhileConnectionHeld_NoException() + { + // Arrange + using var scope = new TransactionScope(); + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + // Act - shutdown while connection is held (not yet returned) + _pool.Shutdown(); + + // Return after shutdown — the pool deactivates and disposes the connection + // rather than returning it to the pool. Verify this doesn't throw. + ReturnConnection(conn, owner); + + // Assert + // The connection should have been deactivated and disposed (not returned to the pool). + // After Dispose(), IsConnectionDoomed is set to true and Pool is set to null. + Assert.True(conn.IsConnectionDoomed, + "Connection should be doomed after returning to a shut-down pool."); + Assert.Null(conn.Pool); + } + + #endregion + + #region Transaction Complete Before Return Tests + + [Fact] + public void TransactionComplete_ThenReturn_ConnectionStillReturned() + { + // Arrange + var owner = new SqlConnection(); + DbConnectionInternal conn; + + using (var scope = new TransactionScope()) + { + conn = GetConnection(owner); + Assert.NotNull(conn); + scope.Complete(); + } + // Transaction is fully disposed here + + // Act - return connection after transaction ended + ReturnConnection(conn, owner); + + // Assert - no leak, pool metrics consistent + AssertPoolMetrics(); + Assert.True(_pool.Count > 0, "Pool should still have the connection"); + } + + #endregion + + #region Sequential Transaction Isolation Tests + + [Fact] + public void SequentialTransactions_EachGetsOwnTransactedEntry() + { + // Act - create multiple sequential transactions + for (int i = 0; i < 5; i++) + { + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Only the current transaction should be tracked + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + + scope.Complete(); + } + + // Assert - after all are done, pool should be clean + AssertPoolMetrics(); + } + + [Fact] + public async Task SequentialTransactions_Async_EachGetsOwnTransactedEntry() + { + // Act - create multiple sequential transactions + for (int i = 0; i < 5; i++) + { + using var scope = new TransactionScope(TransactionScopeAsyncFlowOption.Enabled); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var conn = await GetConnectionAsync(owner, transaction: transaction); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.True(_pool.TransactedConnectionPool.TransactedConnections.ContainsKey(transaction)); + + scope.Complete(); + } + + // Assert + AssertPoolMetrics(); + } + + [Fact] + public void SequentialTransactions_CanReuseConnections() + { + // Act + DbConnectionInternal conn1; + DbConnectionInternal conn2; + Transaction? txn1; + Transaction? txn2; + + using (var scope1 = new TransactionScope()) + { + txn1 = Transaction.Current; + var owner1 = new SqlConnection(); + conn1 = GetConnection(owner1); + Assert.NotNull(conn1); + ReturnConnection(conn1, owner1); + scope1.Complete(); + } + + using (var scope2 = new TransactionScope()) + { + txn2 = Transaction.Current; + var owner2 = new SqlConnection(); + conn2 = GetConnection(owner2); + Assert.NotNull(conn2); + ReturnConnection(conn2, owner2); + scope2.Complete(); + } + + // Assert + // The connection was returned to the general pool and picked up by the second transaction + Assert.NotSame(txn1, txn2); + Assert.Same(conn1, conn2); + AssertPoolMetrics(); + } + + #endregion + + #region Transacted Pool Plumbing Tests + + [Fact] + public void TransactionCompletion_ReturnsConnectionToIdleChannel() + { + // Arrange - park a connection in the transacted pool. + DbConnectionInternal conn; + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // While the transaction is live the connection is held by the transacted pool and is + // deliberately absent from the idle channel. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(0, _pool.IdleCount); + + scope.Complete(); + } + + // Assert - completion drives TransactionEnded -> PutObjectFromTransactedPool, which puts + // the connection back into general circulation. + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(1, _pool.IdleCount); + Assert.Equal(1, _pool.Count); + + // The connection is now reusable by a caller with no ambient transaction. + var owner2 = new SqlConnection(); + var conn2 = GetConnection(owner2); + Assert.Same(conn, conn2); + ReturnConnection(conn2, owner2); + } + + [Fact] + public void TransactionCompletion_AfterShutdown_DestroysConnection() + { + // Arrange - park a connection in the transacted pool, then shut the pool down. The + // transacted connection survives the shutdown drain because closing it would abort the + // (possibly distributed) transaction. + DbConnectionInternal conn; + using (var scope = new TransactionScope()) + { + var owner = new SqlConnection(); + conn = GetConnection(owner); + Assert.NotNull(conn); + ReturnConnection(conn, owner); + + // Act + _pool.Shutdown(); + scope.Complete(); + } + + // Assert - a shut-down pool must not re-pool the connection when the transaction ends. + Assert.Empty(_pool.TransactedConnectionPool.TransactedConnections); + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(0, _pool.Count); + Assert.True(conn.IsConnectionDoomed); + } + + [Fact] + public void TransactionEnded_UnknownConnection_DoesNotPoolConnection() + { + // Arrange - a connection that was never parked in the transacted pool. + var owner = new SqlConnection(); + var conn = GetConnection(owner); + Assert.NotNull(conn); + + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act + _pool.TransactionEnded(transaction!, conn); + + // Assert - nothing to remove, so the connection stays checked out. + Assert.Equal(0, _pool.IdleCount); + Assert.Equal(1, _pool.Count); + + ReturnConnection(conn, owner); + scope.Complete(); + } + + [Fact] + public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + var owner = new SqlConnection(); + var oldConnection = GetConnection(owner); + Assert.NotNull(oldConnection); + Assert.Equal(1, _pool.Count); + + // Act + var newConnection = _pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert - a distinct connection took over the old connection's slot and enlistment. + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); + Assert.Equal(1, _pool.Count); + Assert.True(oldConnection.IsConnectionDoomed); + + ReturnConnection(newConnection, owner); + + // The replacement inherited the transaction, so it parks in the transacted pool. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + + scope.Complete(); + } + + #endregion + + #region Async Ambient Transaction Flow Tests + + /// + /// A created without + /// keeps the ambient transaction in thread-static storage, so it is not observable from the thread + /// pool thread the pool opens on. The pool must therefore take the transaction from the + /// 's AsyncState, which is where SqlConnection.OpenAsync + /// captures it. + /// + [Fact] + public async Task GetConnectionAsync_AmbientTransactionNotFlowed_StillEnlistsFromAsyncState() + { + // Arrange - a transaction that is never ambient on any thread, so AsyncState is the only + // way the pool can learn about it. + using var transaction = new CommittableTransaction(); + Assert.Null(Transaction.Current); + Assert.Null(await Task.Run(() => Transaction.Current)); + + // Act + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + + // Being enlisted, the connection parks in the transacted pool rather than the idle channel. + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction]); + Assert.Equal(0, _pool.IdleCount); + + transaction.Rollback(); + } + + /// + /// Assigning writes to thread-static storage that the + /// ExecutionContext does not unwind, so doing it on a thread pool thread would leave a stale + /// transaction behind for unrelated work later scheduled onto that same thread -- including the + /// login-time auto-enlistment that non-pooled connections perform against the ambient + /// transaction. The pool must pass the transaction explicitly instead of assigning it. + /// + [Fact] + public async Task GetConnectionAsync_DoesNotLeakAmbientTransactionOntoThreadPool() + { + // Arrange & Act - several async opens under a transaction, each on a thread pool thread. + for (int i = 0; i < 8; i++) + { + using var transaction = new CommittableTransaction(); + var owner = new SqlConnection(); + var connection = await GetConnectionAsync(owner, transaction); + ReturnConnection(connection, owner); + transaction.Rollback(); + } + + // Assert - no thread pool thread was left with an ambient transaction. + for (int i = 0; i < 16; i++) + { + Assert.Null(await Task.Run(() => Transaction.Current)); + } + } + + /// + /// The synchronous path runs on the caller's thread, where the ambient transaction set by a + /// TransactionScope is directly observable and must still be honored. + /// + [Fact] + public void GetConnection_Sync_UsesAmbientTransactionFromCallersThread() + { + // Arrange + using var scope = new TransactionScope(); + var transaction = Transaction.Current; + Assert.NotNull(transaction); + + // Act - no transaction is handed to the pool explicitly; it must read Transaction.Current. + var owner = new SqlConnection(); + var connection = GetConnection(owner); + + // Assert + Assert.NotNull(connection); + Assert.Equal(transaction, connection.EnlistedTransaction); + + ReturnConnection(connection, owner); + Assert.Single(_pool.TransactedConnectionPool.TransactedConnections[transaction!]); + + scope.Complete(); + } + + #endregion + + #region Mock Classes + + internal class MockSqlConnectionFactory : SqlConnectionFactory + { + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new MockDbConnectionInternal(); + } + } + + internal class MockDbConnectionInternal : DbConnectionInternal + { + private static int s_nextId = 1; + public int MockId { get; } = Interlocked.Increment(ref s_nextId); + + public override string ServerVersion => "Mock"; + + public override ConnectionCapabilities Capabilities => new(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction? transaction) + { + if (transaction != null) + { + EnlistedTransaction = transaction; + } + } + + protected override void Activate(Transaction? transaction) + { + EnlistedTransaction = transaction; + } + + protected override void Deactivate() + { + } + + public override string ToString() => $"MockConnection_{MockId}"; + + internal override void ResetConnection() + { + } + } + + #endregion +} 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/TransactedConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs index 588418ac03..99645c6979 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/TransactedConnectionPoolTest.cs @@ -660,6 +660,7 @@ internal class MockDbConnectionPool : IDbConnectionPool public SqlConnectionFactory ConnectionFactory => throw new NotImplementedException(); public int Count => throw new NotImplementedException(); public bool ErrorOccurred => throw new NotImplementedException(); + public Exception? LastConnectionCreateException => null; public int Id { get; } = 1; public int IdleCount => throw new NotImplementedException(); public DbConnectionPoolIdentity Identity => throw new NotImplementedException(); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs index e84f040caa..ca7bdf1bde 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/WaitHandleDbConnectionPoolBlockingPeriodTest.cs @@ -200,6 +200,34 @@ public void TryGetConnection_WhenFactorySucceeds_DoesNotEnterBlockingPeriod() Assert.Equal(1, factory.CreateConnectionCallCount); } + /// + /// Verifies that the pool records the exception from a failed physical create and clears it + /// once a create succeeds, so a later pooled-open timeout reports the most recent cause rather + /// than a stale one (GH#3545). + /// + [Fact] + public void LastConnectionCreateException_RecordedOnFailure_ClearedOnSuccess() + { + // Arrange - NeverBlock keeps the error state from fast-failing the second request. + bool shouldFail = true; + var factory = new ConfigurableSqlConnectionFactory(_ => + shouldFail ? throw SqlExceptionHelper.CreateSqlException("server unreachable") : new MockDbConnectionInternal()); + var pool = CreatePool(factory, "Data Source=localhost;Pool Blocking Period=NeverBlock;"); + using var owner = new SqlConnection(); + + Assert.Null(pool.LastConnectionCreateException); + + // Act & Assert - the failure is recorded. + Assert.Throws(() => TryGetConnectionSync(pool, owner, out _)); + Assert.IsType(pool.LastConnectionCreateException); + + // Act & Assert - a subsequent success clears it. + shouldFail = false; + Assert.True(TryGetConnectionSync(pool, owner, out DbConnectionInternal? connection)); + Assert.NotNull(connection); + Assert.Null(pool.LastConnectionCreateException); + } + /// /// Verifies that once the blocking period's exit timer fires, the next request retries the /// factory and a successful create recovers the pool: