From d3e2fe79c7d5b29a67559ead383c568d5df47a62 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 23 Jun 2026 10:32:04 -0700 Subject: [PATCH 01/39] Add connection-creation rate limiting to ChannelDbConnectionPool Introduce an optional System.Threading.RateLimiting policy that throttles new physical connection opens in the channel pool: when a permit is denied the caller waits for a returned connection instead of forcing a create, and leases are always released (including on failure) to avoid starvation. Adds NoOpAcquiredLease, wires the RateLimiting package into the product and test projects, and includes the 006-pool-rate-limiting spec. Also repairs two pre-existing build breaks in ChannelDbConnectionPoolTest (a dropped CountingSuccessfulConnectionFactory declaration and DbConnectionPoolGroupOptions calls missing the new idleTimeout argument). --- Directory.Packages.props | 2 + specs/006-pool-rate-limiting/diagrams.md | 41 + specs/006-pool-rate-limiting/spec.md | 146 +++ .../src/Microsoft.Data.SqlClient.csproj | 2 + .../ConnectionPool/ChannelDbConnectionPool.cs | 199 +++- .../ConnectionPool/NoOpAcquiredLease.cs | 45 + ...icrosoft.Data.SqlClient.ManualTests.csproj | 2 + .../ChannelDbConnectionPoolTest.cs | 864 +++++++++++++++++- .../Microsoft.Data.SqlClient.UnitTests.csproj | 2 + 9 files changed, 1238 insertions(+), 65 deletions(-) create mode 100644 specs/006-pool-rate-limiting/diagrams.md create mode 100644 specs/006-pool-rate-limiting/spec.md create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 59839f2351..76142810ca 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -150,6 +150,7 @@ + @@ -157,6 +158,7 @@ + diff --git a/specs/006-pool-rate-limiting/diagrams.md b/specs/006-pool-rate-limiting/diagrams.md new file mode 100644 index 0000000000..4b3f3130d7 --- /dev/null +++ b/specs/006-pool-rate-limiting/diagrams.md @@ -0,0 +1,41 @@ +# Rate limiting comparison + +## Existing rate limiting + +```mermaid +flowchart TD + Start([Open request]) --> WaitAny["WaitHandle.WaitAny
(blocking, no queue)"] + + WaitAny -->|idle available| S0["PoolSemaphore
Semaphore 0..MAX"] + WaitAny -->|error state| S1["ErrorEvent
ManualResetEvent"] + WaitAny -->|permit to open one conn| S2["CreationSemaphore
Semaphore 1,1"] + + S0 -->|got connection| Done([Return connection]) + S2 --> Open["Open physical connection"] + Open --> Release["Semaphore.Release 1"] + Release -->|got connection| Done + + classDef prim fill:#bfdbfe,stroke:#1e3a8a,color:#111 + class WaitAny,S0,S1,S2,Open,Release prim +``` + +## New rate limiting + +```mermaid +flowchart TD + Start([Open request]) --> Idle["Idle channel
TryRead
(non-blocking)"] + + Idle -->|got connection| Done([Return connection]) + Idle -->|empty| Limiter["RateLimiter
AttemptAcquire 1
(non-blocking)"] + + Limiter -->|acquired lease| Open["Open physical connection"] + Limiter -->|not acquired| Channel["Idle channel
await ReadAsync
(FIFO queued)"] + + Open --> Lease["RateLimitLease.Dispose"] + Lease --> |got connection| Done + Channel -->|loop on wake signal| Idle + Channel --> |got connection| Done + + classDef prim fill:#bfdbfe,stroke:#1e3a8a,color:#111 + class Idle,Limiter,Open,Lease,Channel prim +``` \ No newline at end of file diff --git a/specs/006-pool-rate-limiting/spec.md b/specs/006-pool-rate-limiting/spec.md new file mode 100644 index 0000000000..2c3c164309 --- /dev/null +++ b/specs/006-pool-rate-limiting/spec.md @@ -0,0 +1,146 @@ +# Feature Specification: Pool Rate Limiting and Blocking Period + +**Feature Branch**: `dev/mdaigle/pool-rate-limit` +**Created**: 2026-05-19 +**Status**: Draft +**Input**: ADO Work Item 37824 — "Implement connection open rate limiting" + +## Description + +Add rate limiting to `ChannelDbConnectionPool` to control how many physical connections can be +created concurrently. Without throttling, a burst of concurrent requests can trigger a login +storm against SQL Server. The implementation uses +`System.Threading.RateLimiting.ConcurrencyLimiter` from the BCL — no custom rate limiting +primitives are defined. + +This feature also adds the `PoolBlockingPeriod` error state (fast-fail after a connection +creation failure) with exponential backoff recovery, matching the existing +`WaitHandleDbConnectionPool` behavior. + +> Time spent waiting for the rate limiter counts against the caller's overall `ConnectTimeout` +> budget. `ReplaceConnection` (when implemented) MUST bypass the rate limiter: it already holds +> a pool slot and must not deadlock. + +## User Scenarios & Testing + +### User Story 1 — Throttled Connection Creation Under Burst Demand (P1) + +The pool limits the number of simultaneous physical connection creation attempts. Callers that +cannot immediately create a connection wait in FIFO order until the limiter allows them to +proceed, subject to their `ConnectTimeout`. + +**Acceptance Scenarios**: + +1. **Given** the pool has no idle connections and many callers request connections simultaneously, + **When** the concurrency limit is reached, **Then** additional callers wait until an in-flight + creation completes before starting their own. +2. **Given** a caller is waiting for the rate limiter, **When** its `ConnectTimeout` elapses, + **Then** the caller receives a timeout error without ever attempting to create a connection. +3. **Given** the rate limiter has available capacity, **When** a caller requests a new connection, + **Then** the create proceeds immediately with no added latency. +4. **Given** a connection creation completes (success or failure), **When** the `RateLimitLease` + is disposed, **Then** the next waiting caller is allowed to proceed. + +--- + +### User Story 2 — Blocking Period Fast-Fail on Connection Failure (P1) + +When a connection creation attempt fails because the server is unreachable, the pool enters an +error state and immediately fails subsequent requests for a limited period, returning the cached +error. This prevents cascading timeouts when the server is down. + +**Acceptance Scenarios**: + +1. **Given** a creation failure has occurred and blocking period is enabled, **When** a new + connection is requested within the blocking window, **Then** the request fails immediately + with the cached error. +2. **Given** a creation failure has occurred and blocking period is enabled, **When** the + blocking window expires, **Then** the next request attempts fresh connection creation. +3. **Given** `PoolBlockingPeriod=NeverBlock`, **When** a creation failure occurs, **Then** each + subsequent request independently attempts creation (no fast-fail). +4. **Given** `PoolBlockingPeriod=Auto` connecting to an Azure SQL endpoint and a failure occurs, + **Then** no blocking period is applied (same as `NeverBlock`). +5. **Given** `PoolBlockingPeriod=Auto` connecting to an on-premises SQL Server and a failure + occurs, **Then** the blocking period is applied (same as `AlwaysBlock`). + +--- + +### User Story 3 — Error State Recovery with Exponential Backoff (P2) + +While in the error state the pool waits using exponential backoff (5s → 10s → 20s → 30s → 60s +cap) before allowing the next attempt. Once an attempt after the backoff succeeds, the error +state clears and backoff resets. + +**Acceptance Scenarios**: + +1. **Given** the pool is in error state, **When** the backoff timer fires and the next caller's + attempt succeeds, **Then** the error state is cleared and subsequent requests attempt normal + creation. +2. **Given** the pool is in error state, **When** the backoff timer fires and the next caller's + attempt fails, **Then** the backoff interval increases (up to the 60s cap) and the pool + re-enters the error state. +3. **Given** the pool is in error state, **When** the error is cleared, **Then** the cached + exception, the error flag, and the backoff interval are all reset. + +--- + +### User Story 4 — Rate Limiting Counts Against Connection Timeout (P2) + +Time spent waiting for rate limiter capacity counts against the caller's overall +`ConnectTimeout` budget. + +**Acceptance Scenarios**: + +1. **Given** a caller's timeout is 15s and the caller waits 10s for rate limiting, **When** the + rate limiter releases, **Then** the remaining budget for connection creation is 5s. +2. **Given** a caller's timeout expires while waiting for the rate limiter, **When** the timeout + fires, **Then** the caller receives a timeout error and is removed from the limiter queue. + +--- + +### User Story 5 — Rate Limiter Built on System.Threading.RateLimiting (P3) + +The pool uses `System.Threading.RateLimiting.RateLimiter` as the base abstraction and +`ConcurrencyLimiter` as the initial implementation. No custom rate limiting primitives are +defined. + +**Acceptance Scenarios**: + +1. **Given** the pool is configured with the default `ConcurrencyLimiter`, **When** connections + are created, **Then** the limiter throttles concurrent creation to the configured maximum. +2. **Given** a different `RateLimiter` implementation is substituted, **When** connections are + created, **Then** the pool delegates throttling to the substituted implementation without + code changes to pool logic. + +--- + +## Functional Requirements + +- **FR-001**: The pool MUST limit the number of concurrent physical connection creation attempts + to a configurable maximum. +- **FR-002**: Callers that cannot immediately create a connection due to rate limiting MUST wait + in FIFO order until capacity is available or their timeout expires. +- **FR-003**: Time spent waiting for rate limiter capacity MUST count against the caller's + overall connection timeout budget. +- **FR-004**: When a connection creation attempt completes (success or failure), the + `RateLimitLease` MUST be disposed so the next waiting caller can proceed. +- **FR-005**: The pool MUST support three `PoolBlockingPeriod` modes: `Auto`, `AlwaysBlock`, and + `NeverBlock`. +- **FR-006**: When the blocking period is enabled, the pool MUST enter an error state after a + creation failure and immediately fail subsequent requests with the cached error. +- **FR-007**: When the blocking period is disabled, the pool MUST NOT enter an error state; + each request MUST independently attempt creation. +- **FR-008**: While in error state, the backoff MUST use exponential growth starting at 5s, + doubling each attempt, capped at 60s. +- **FR-009**: When an attempt succeeds, the pool MUST clear the error state and reset the + backoff to its initial value. +- **FR-010**: The `ErrorOccurred` property MUST return `true` when in the error state and + `false` otherwise. +- **FR-011**: `ClearPool` MUST clear the error state in addition to invalidating pooled + connections. +- **FR-012**: The rate limiter MUST use `System.Threading.RateLimiting.RateLimiter` as the base + abstraction so that any `RateLimiter` implementation can be substituted without modifying + pool acquisition logic. +- **FR-013**: The initial implementation MUST use + `System.Threading.RateLimiting.ConcurrencyLimiter` configured with the desired maximum number + of concurrent connection creation attempts. diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj index 06eaf9e914..92c648c283 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj @@ -309,6 +309,7 @@ + @@ -321,6 +322,7 @@ + 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 f5d758ebb7..ca4b177064 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 @@ -8,6 +8,7 @@ using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; +using System.Threading.RateLimiting; using System.Threading.Tasks; using System.Transactions; using Microsoft.Data.Common; @@ -98,6 +99,21 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// . /// private int _shutdownInitiated; + + /// + /// Optional rate limiter that throttles the number of concurrent physical connection + /// creation attempts. When null, no rate limiting is applied. A non-null limiter is + /// supplied at pool construction time; there is no default. Callers fast-fail against + /// the limiter and fall back to the idle-channel wait when no permit is available. + /// + private readonly RateLimiter? _connectionCreationRateLimiter; + + /// + /// Encapsulates the blocking-period error state for this pool: cached exception, exponential + /// backoff timer, and synchronization. Created only when blocking period is enabled for + /// this pool group. See . + /// + private readonly BlockingPeriodErrorState? _errorState; #endregion /// @@ -107,7 +123,8 @@ internal ChannelDbConnectionPool( SqlConnectionFactory connectionFactory, DbConnectionPoolGroup connectionPoolGroup, DbConnectionPoolIdentity identity, - DbConnectionPoolProviderInfo connectionPoolProviderInfo) + DbConnectionPoolProviderInfo connectionPoolProviderInfo, + RateLimiter? connectionCreationRateLimiter = null) { ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; @@ -117,9 +134,14 @@ internal ChannelDbConnectionPool( AuthenticationContexts = new(); MaxPoolSize = Convert.ToUInt32(PoolGroupOptions.MaxPoolSize); TransactedConnectionPool = new(this); + _connectionCreationRateLimiter = connectionCreationRateLimiter; _connectionSlots = new(MaxPoolSize); _idleChannel = new(); + if (PoolGroup.IsBlockingPeriodEnabled()) + { + _errorState = new BlockingPeriodErrorState(_instanceId); + } // Pruning is only useful when the pool can grow beyond MinPoolSize. // If min >= max, the pool is fixed-size and pruning would never activate. @@ -147,8 +169,7 @@ public ConcurrentDictionary< public int IdleCount => _idleChannel.Count; /// - /// This will be implemented later when we add support for the pool blocking period after errors. For now, it always returns false. - public bool ErrorOccurred => false; + public bool ErrorOccurred => _errorState?.HasError ?? false; /// public int Id => _instanceId; @@ -192,6 +213,10 @@ public void Clear() SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Clearing.", Id); + // Clearing the pool implies the caller wants a clean slate, so abandon any cached + // error state. FR-011. + _errorState?.Clear(); + Interlocked.Increment(ref _clearGeneration); // If another thread is already draining, skip the drain. The generation counter has @@ -324,6 +349,19 @@ public void Shutdown() " {0}, Pruner.Dispose threw, continuing shutdown: {1}", Id, ex); } + // Dispose the error state so its exit timer is released. Otherwise a timer scheduled + // during the blocking period would keep this pool reachable and continue firing + // callbacks/logging after shutdown. + try + { + _errorState?.Dispose(); + } + catch (Exception ex) + { + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, _errorState.Dispose threw, continuing shutdown: {1}", Id, ex); + } + // Complete the channel writer so: // - no further idle connections can be enqueued (TryWrite returns false), and // - in-flight / future async waiters on ReadAsync fault with ChannelClosedException. @@ -496,13 +534,16 @@ public bool TryGetConnection( } /// - /// Opens a new internal connection to the database. + /// Opens a new internal connection to the database, throttled by the pool's rate limiter. /// /// The owning connection. /// The cancellation token to cancel the operation. /// The overall timeout budget. Passed through to the physical connection /// so it uses the remaining budget rather than starting a fresh timeout. - /// A task representing the asynchronous operation, with a result of the new internal connection. + /// The new internal connection, or null if the pool has no available slot or the + /// rate limiter is currently saturated. In the latter case the caller should fall back to + /// the idle-channel wait; the rate limiter will write a null to the idle channel when a + /// permit is released so the waiter can retry. /// /// Thrown when the cancellation token is cancelled before the connection operation completes. /// @@ -513,50 +554,120 @@ public bool TryGetConnection( { cancellationToken.ThrowIfCancellationRequested(); - // Opening a connection can be a slow operation and we don't want to hold a lock for the duration. - // Instead, we reserve a connection slot prior to attempting to open a new connection and release the slot - // in case of an exception. + // Fast-fail in the error state. FR-006. + _errorState?.ThrowIfActive(); - var result = _connectionSlots.Add( - createCallback: () => - { - // https://github.com/dotnet/SqlClient/issues/3459 - // TODO: This blocks the thread for several network calls! - // When running async, the blocked thread is one allocated from the managed thread pool (due to - // use of Task.Run in TryGetConnection). This is why it's critical for async callers to - // pre-provision threads in the managed thread pool. Our options are limited because - // DbConnectionInternal doesn't support an async open. It's better to block this thread and keep - // throughput high than to queue all of our opens onto a single worker thread. Add an async path - // when this support is added to DbConnectionInternal. - // TODO: ultimately, the connection factory should also accept our cancellation token. - var connection = ConnectionFactory.CreatePooledConnection( - owningConnection, - this, - timeout); - - if (connection is not null) + try + { + // Reserve a pool slot up front so we don't pay the rate-limit cost only to + // discover the pool is full. Add() reserves synchronously and returns null + // immediately if no slot is available; the rate-limit check only happens inside + // the createCallback, which runs after the reservation succeeds. + DbConnectionInternal? connection = _connectionSlots.Add( + createCallback: () => { - connection.ClearGeneration = _clearGeneration; - } + // Fast-fail rate-limit attempt when a limiter is configured. + // AttemptAcquire returns synchronously and does not queue: if no permit + // is available right now, the lease comes back with IsAcquired == false. + // We deliberately do not block here so the caller can fall back to + // waiting on the idle channel, where it can be satisfied either by a + // returning connection or by a null poke from another caller releasing + // its rate-limit lease (see finally below). We prefer to recycle existing + // connections rather then queue on the rate limit. When no limiter is + // configured we substitute a no-op acquired lease. + // FR-001, FR-002, FR-003. + + //TODO: some other options to consider: + // 1. fail immediately and surface an error to the caller + // 2. block on acquire (subject to overall timeout) - this would be the simplest + RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; + bool leaseAcquired = lease.IsAcquired; + try + { + if (!leaseAcquired) + { + // 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. + return null; + } + + cancellationToken.ThrowIfCancellationRequested(); + + // https://github.com/dotnet/SqlClient/issues/3459 + // TODO: This blocks the thread for several network calls! + // When running async, the blocked thread is one allocated from the managed thread pool (due to + // use of Task.Run in TryGetConnection). This is why it's critical for async callers to + // pre-provision threads in the managed thread pool. Our options are limited because + // DbConnectionInternal doesn't support an async open. It's better to block this thread and keep + // throughput high than to queue all of our opens onto a single worker thread. Add an async path + // when this support is added to DbConnectionInternal. + // TODO: ultimately, the connection factory should also accept our cancellation token. + var newConnection = ConnectionFactory.CreatePooledConnection( + owningConnection, + this, + timeout); + + if (newConnection is not null) + { + newConnection.ClearGeneration = _clearGeneration; + } + + return newConnection; + } + finally + { + // Release the permit back to the limiter (no-op for the default lease) + // BEFORE signaling a waiter. Otherwise a woken waiter could consume the + // null poke and retry its acquire before the permit is actually returned, + // fail to acquire, and fall back to waiting with no subsequent signal - + // stalling connection creation even though the limiter has capacity. + lease.Dispose(); + + // After releasing, signal a waiter on the idle channel that they may now + // retry an open. We only poke when a limiter is configured (a waiter only + // falls back to the idle channel due to rate limiting in that case) and + // the pool can still grow; if we're at MaxPoolSize, only a connection + // return can satisfy a waiter. FR-004. This is best-effort; releasing a + // lease doesn't guarantee the rate limiter immediately has an available + // permit, but the waiter we wake will fall back to waiting again if not. + if (leaseAcquired && + _connectionCreationRateLimiter is not null && + _connectionSlots.ReservationCount < MaxPoolSize) + { + _idleChannel.TryWrite(null); + } + } + }, + cleanupCallback: (newConnection) => + { + // If we fail to open a connection, we need to write a null to the idle channel to + // wake up any waiters + _idleChannel.TryWrite(null); + newConnection?.Dispose(); + }); - return connection; - }, - cleanupCallback: (newConnection) => + if (connection is not null) { - // If we fail to open a connection, we need to write a null to the idle channel to - // wake up any waiters - _idleChannel?.TryWrite(null); - newConnection?.Dispose(); - }); + // 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(); - if (result is not null) - { - // 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 creation clears error/backoff state + // FR-009. + _errorState?.Clear(); + } + + return connection; } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) + { + // Enter the blocking period error state on creation failure if configured. + // FR-006, FR-007. + _errorState?.Enter(ex); - return result; + throw; + } } /// @@ -686,7 +797,9 @@ private async Task GetInternalConnection( connection ??= GetIdleConnection(); - // If we didn't find an idle connection, try to open a new one. + // If we didn't find an idle connection, try to open a new one. This may + // return null if the pool is full or the rate limiter is currently saturated; + // in either case the caller falls through to the idle-channel wait below. connection ??= OpenNewInternalConnection( owningConnection, cancellationToken, diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs new file mode 100644 index 0000000000..96785fe0e8 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs @@ -0,0 +1,45 @@ +// 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.Generic; +using System.Threading.RateLimiting; + +#nullable enable + +namespace Microsoft.Data.SqlClient.ConnectionPool +{ + /// + /// A no-op that is always acquired and performs no work on + /// dispose. Used as a stand-in when no rate limiter is configured so the open path can + /// treat the lease as unconditional. Stateless and safe to share across all callers; access + /// the singleton via . + /// + internal sealed class NoOpAcquiredLease : RateLimitLease + { + /// + /// The shared singleton instance. + /// + public static readonly NoOpAcquiredLease Instance = new(); + + private NoOpAcquiredLease() + { + } + + public override bool IsAcquired => true; + + public override IEnumerable MetadataNames => Array.Empty(); + + public override bool TryGetMetadata(string metadataName, out object? metadata) + { + metadata = null; + return false; + } + + protected override void Dispose(bool disposing) + { + // No resources to release. + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj index f3e4b61b45..be691606ce 100644 --- a/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj +++ b/src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj @@ -98,6 +98,7 @@ + @@ -124,6 +125,7 @@ + diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 914e63019a..ae76445897 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -3,9 +3,11 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; using System.Collections.Concurrent; using System.Data.Common; using System.Threading; +using System.Threading.RateLimiting; using System.Threading.Tasks; using System.Transactions; using Microsoft.Data.Common; @@ -18,16 +20,32 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool { + /// + /// Unit tests for covering connection acquisition, + /// timeouts, reuse, pool clearing, blocking-period behavior, and timeout-budget propagation. + /// public class ChannelDbConnectionPoolTest { private static readonly SqlConnectionFactory SuccessfulConnectionFactory = new SuccessfulSqlConnectionFactory(); private static readonly SqlConnectionFactory TimeoutConnectionFactory = new TimeoutSqlConnectionFactory(); + /// + /// Creates a with configurable test dependencies so + /// individual tests can focus on the behavior under test without repeating setup logic. + /// + /// The factory used to create physical connections. + /// Optional pool identity override. + /// Optional pool group override. + /// Optional pool options override. + /// Optional provider info override. + /// Optional rate limiter controlling physical connection creation. + /// A configured instance for testing. private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFactory, DbConnectionPoolIdentity? identity = null, DbConnectionPoolGroup? dbConnectionPoolGroup = null, DbConnectionPoolGroupOptions? poolGroupOptions = null, - DbConnectionPoolProviderInfo? connectionPoolProviderInfo = null) + DbConnectionPoolProviderInfo? connectionPoolProviderInfo = null, + RateLimiter? connectionCreationRateLimiter = null) { poolGroupOptions ??= new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -47,10 +65,15 @@ private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFac connectionFactory, dbConnectionPoolGroup, identity ?? DbConnectionPoolIdentity.NoIdentity, - connectionPoolProviderInfo ?? new DbConnectionPoolProviderInfo() + connectionPoolProviderInfo ?? new DbConnectionPoolProviderInfo(), + connectionCreationRateLimiter ); } + /// + /// Verifies that requesting connections from an empty pool causes the pool to create new + /// physical connections until the requested count is reached. + /// [Theory] [InlineData(1)] [InlineData(5)] @@ -75,11 +98,14 @@ out DbConnectionInternal? internalConnection Assert.NotNull(internalConnection); } - // Assert Assert.Equal(numConnections, pool.Count); } + /// + /// Verifies that asynchronous requests against an empty pool create new physical + /// connections and complete through the provided task completion source. + /// [Theory] [InlineData(1)] [InlineData(5)] @@ -106,11 +132,14 @@ out DbConnectionInternal? internalConnection Assert.NotNull(await tcs.Task); } - // Assert Assert.Equal(numConnections, pool.Count); } + /// + /// Verifies that a synchronous request against an exhausted pool fails with the pooled-open + /// timeout once the caller's timeout budget has already expired. + /// [Fact] public void GetConnectionMaxPoolSize_ShouldTimeoutAfterPeriod() { @@ -126,6 +155,7 @@ public void GetConnectionMaxPoolSize_ShouldTimeoutAfterPeriod() out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -154,6 +184,10 @@ out DbConnectionInternal? internalConnection Assert.Equal(pool.PoolGroupOptions.MaxPoolSize, pool.Count); } + /// + /// Verifies that an asynchronous request against an exhausted pool completes with the + /// pooled-open timeout once the caller's timeout budget has already expired. + /// [Fact] public async Task GetConnectionAsyncMaxPoolSize_ShouldTimeoutAfterPeriod() { @@ -169,6 +203,7 @@ public async Task GetConnectionAsyncMaxPoolSize_ShouldTimeoutAfterPeriod() out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -195,6 +230,10 @@ out DbConnectionInternal? internalConnection Assert.Equal(pool.PoolGroupOptions.MaxPoolSize, pool.Count); } + /// + /// Verifies that a waiting synchronous caller reuses a connection that is returned to an + /// exhausted pool instead of creating a new physical connection. + /// [Fact] public async Task GetConnectionMaxPoolSize_ShouldReuseAfterConnectionReleased() { @@ -218,16 +257,15 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } - TaskCompletionSource tcs = new(); - // Act var task = Task.Run(() => { - var exceeded = pool.TryGetConnection( + pool.TryGetConnection( new SqlConnection(""), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), @@ -242,6 +280,10 @@ out DbConnectionInternal? extraConnection Assert.Equal(firstConnection, extraConnection); } + /// + /// Verifies that a waiting asynchronous caller reuses a connection that is returned to an + /// exhausted pool instead of creating a new physical connection. + /// [Fact] public async Task GetConnectionAsyncMaxPoolSize_ShouldReuseAfterConnectionReleased() { @@ -265,6 +307,7 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -272,7 +315,7 @@ out DbConnectionInternal? internalConnection TaskCompletionSource taskCompletionSource = new(); // Act - var exceeded = pool.TryGetConnection( + pool.TryGetConnection( new SqlConnection(""), taskCompletionSource, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), @@ -285,6 +328,10 @@ out DbConnectionInternal? recycledConnection Assert.Equal(firstConnection, recycledConnection); } + /// + /// Verifies that synchronous waiters are served in request order when the pool is full, + /// ensuring the first queued request receives the next returned connection. + /// [Fact] [ActiveIssue("https://github.com/dotnet/SqlClient/issues/3730")] public async Task GetConnectionMaxPoolSize_ShouldRespectOrderOfRequest() @@ -309,6 +356,7 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -354,6 +402,10 @@ out DbConnectionInternal? failedConnection await Assert.ThrowsAsync(async () => await failedTask); } + /// + /// Verifies that asynchronous waiters are served in request order when the pool is full, + /// ensuring the first queued request receives the next returned connection. + /// [Fact] [ActiveIssue("https://github.com/dotnet/SqlClient/issues/3730")] public async Task GetConnectionAsyncMaxPoolSize_ShouldRespectOrderOfRequest() @@ -378,6 +430,7 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -386,7 +439,7 @@ out DbConnectionInternal? internalConnection TaskCompletionSource failedCompletionSource = new(); // Act - var exceeded = pool.TryGetConnection( + pool.TryGetConnection( new SqlConnection(""), recycledTaskCompletionSource, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), @@ -396,7 +449,7 @@ out DbConnectionInternal? recycledConnection // Gives time for the recycled connection to be queued before the failed request is initiated. await Task.Delay(1000); - var exceeded2 = pool.TryGetConnection( + pool.TryGetConnection( new SqlConnection("Timeout=1"), failedCompletionSource, TimeoutTimer.StartNew(TimeSpan.FromSeconds(1)), @@ -411,6 +464,10 @@ out DbConnectionInternal? failedConnection await Assert.ThrowsAsync(async () => failedConnection = await failedCompletionSource.Task); } + /// + /// Verifies that a connection returned to the idle channel is reused by a subsequent + /// request instead of allocating a new internal connection. + /// [Fact] public void ConnectionsAreReused() { @@ -447,6 +504,10 @@ out DbConnectionInternal? internalConnection2 Assert.Same(internalConnection1, internalConnection2); } + /// + /// Verifies that synchronous connection creation failures propagate the pooled-open timeout + /// exception from the connection factory. + /// [Fact] public void GetConnectionTimeout_ShouldThrowTimeoutException() { @@ -469,6 +530,10 @@ out DbConnectionInternal? internalConnection Assert.Equal(ADP.PooledOpenTimeout().Message, ex.Message); } + /// + /// Verifies that asynchronous connection creation failures propagate the pooled-open timeout + /// exception through the caller's task completion source. + /// [Fact] public async Task GetConnectionAsyncTimeout_ShouldThrowTimeoutException() { @@ -494,13 +559,18 @@ out DbConnectionInternal? internalConnection Assert.Equal(ADP.PooledOpenTimeout().Message, ex.Message); } + /// + /// Verifies under concurrent synchronous load that the pool never grows beyond its + /// configured maximum size and continues to serve requests safely. + /// [Fact] public void StressTest() { - //Arrange + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); ConcurrentBag tasks = new(); + // Act for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize * 3; i++) { var t = Task.Run(() => @@ -524,16 +594,23 @@ out DbConnectionInternal? internalConnection } Task.WaitAll(tasks.ToArray()); + + // Assert Assert.True(pool.Count <= pool.PoolGroupOptions.MaxPoolSize, "Pool size exceeded max pool size after stress test."); } + /// + /// Verifies under concurrent asynchronous load that the pool never grows beyond its + /// configured maximum size and continues to serve requests safely. + /// [Fact] public void StressTestAsync() { - //Arrange + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); ConcurrentBag tasks = new(); + // Act for (int i = 1; i < pool.PoolGroupOptions.MaxPoolSize * 3; i++) { var t = Task.Run(async () => @@ -555,58 +632,102 @@ out DbConnectionInternal? internalConnection } Task.WaitAll(tasks.ToArray()); + + // Assert Assert.True(pool.Count <= pool.PoolGroupOptions.MaxPoolSize, "Pool size exceeded max pool size after stress test."); } #region Property Tests + /// + /// Verifies that the pool exposes the instance it was + /// constructed with. + /// [Fact] public void TestConnectionFactory() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.Equal(SuccessfulConnectionFactory, pool.ConnectionFactory); } + /// + /// Verifies that a newly constructed pool starts with zero tracked connections. + /// [Fact] public void TestCount() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.Equal(0, pool.Count); } + /// + /// Verifies that a newly constructed pool reports no blocking-period error by default. + /// [Fact] public void TestErrorOccurred() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.False(pool.ErrorOccurred); } + /// + /// Verifies that the pool assigns a positive instance identifier at construction time. + /// [Fact] public void TestId() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.True(pool.Id >= 1); } + /// + /// Verifies that the pool exposes the identity object it was constructed with. + /// [Fact] public void TestIdentity() { + // Arrange var identity = DbConnectionPoolIdentity.GetCurrent(); var pool = ConstructPool(SuccessfulConnectionFactory, identity); + + // Act & Assert Assert.Equal(identity, pool.Identity); } + /// + /// Verifies that a newly constructed pool begins in the running state. + /// [Fact] public void TestIsRunning() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.True(pool.IsRunning); } + /// + /// Verifies that the pool exposes the configured load-balance timeout from its pool group + /// options. + /// [Fact] public void TestLoadBalanceTimeout() { + // Arrange var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -617,12 +738,19 @@ public void TestLoadBalanceTimeout() idleTimeout: 0 ); var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions: poolGroupOptions); + + // Act & Assert Assert.Equal(poolGroupOptions.LoadBalanceTimeout, pool.LoadBalanceTimeout); } + /// + /// Verifies that the pool exposes the exact instance it + /// was constructed with. + /// [Fact] public void TestPoolGroup() { + // Arrange var dbConnectionPoolGroup = new DbConnectionPoolGroup( new SqlConnectionOptions("Data Source=localhost;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), @@ -635,12 +763,19 @@ public void TestPoolGroup() hasTransactionAffinity: true, idleTimeout: 0)); var pool = ConstructPool(SuccessfulConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + // Act & Assert Assert.Equal(dbConnectionPoolGroup, pool.PoolGroup); } + /// + /// Verifies that the pool exposes the exact + /// instance it was constructed with. + /// [Fact] public void TestPoolGroupOptions() { + // Arrange var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -650,34 +785,61 @@ public void TestPoolGroupOptions() hasTransactionAffinity: true, idleTimeout: 0); var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions: poolGroupOptions); + + // Act & Assert Assert.Equal(poolGroupOptions, pool.PoolGroupOptions); } + /// + /// Verifies that the pool exposes the provider info object it was constructed with. + /// [Fact] public void TestProviderInfo() { + // Arrange var connectionPoolProviderInfo = new DbConnectionPoolProviderInfo(); var pool = ConstructPool(SuccessfulConnectionFactory, connectionPoolProviderInfo: connectionPoolProviderInfo); + + // Act & Assert Assert.Equal(connectionPoolProviderInfo, pool.ProviderInfo); } + /// + /// Verifies that the pool state getter reports + /// immediately after construction. + /// [Fact] public void TestStateGetter() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.Equal(DbConnectionPoolState.Running, pool.State); } + /// + /// Verifies that the pool state remains after + /// construction when no shutdown has been requested. + /// [Fact] public void TestStateSetter() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.Equal(DbConnectionPoolState.Running, pool.State); } + /// + /// Verifies that the pool exposes whether load balancing is enabled based on its configured + /// pool group options. + /// [Fact] public void TestUseLoadBalancing() { + // Arrange var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -687,6 +849,8 @@ public void TestUseLoadBalancing() hasTransactionAffinity: true, idleTimeout: 0); var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions: poolGroupOptions); + + // Act & Assert Assert.Equal(poolGroupOptions.UseLoadBalancing, pool.UseLoadBalancing); } @@ -694,41 +858,71 @@ public void TestUseLoadBalancing() #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!)); } + /// + /// Verifies that + /// remains unimplemented and throws . + /// [Fact] public void TestReplaceConnection() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.Throws(() => pool.ReplaceConnection(null!, null!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); } + /// + /// Verifies that + /// remains unimplemented and throws . + /// [Fact] public void TestTransactionEnded() { + // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + + // Act & Assert Assert.Throws(() => pool.TransactionEnded(null!, null!)); } #endregion #region Pool Clear Tests + /// + /// Verifies that clearing an empty pool is a no-op and leaves the pool in a valid state. + /// [Fact] public void Clear_EmptyPool_DoesNotThrow() { // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); - // Act & Assert - Should complete without error + // Act pool.Clear(); + + // Assert Assert.Equal(0, pool.Count); } + /// + /// Verifies that clearing a pool with only idle connections destroys them immediately and + /// leaves the pool empty. + /// [Fact] public void Clear_MultipleIdleConnections_AllAreDestroyed() { @@ -763,6 +957,10 @@ out internalConnections[i] Assert.Equal(0, pool.Count); } + /// + /// Verifies that clearing the pool does not immediately destroy a connection that is still + /// checked out by a caller. + /// [Fact] public void Clear_BusyConnection_NotDestroyedImmediately() { @@ -787,6 +985,10 @@ out DbConnectionInternal? busyConnection Assert.Equal(0, busyConnection.ClearGeneration); } + /// + /// Verifies that a busy connection checked out during + /// is destroyed when it is later returned because its generation is stale. + /// [Fact] public void Clear_BusyConnectionReturned_IsDestroyed() { @@ -816,6 +1018,10 @@ out DbConnectionInternal? busyConnection Assert.Equal(0, pool.Count); } + /// + /// Verifies that clearing a pool with both busy and idle connections destroys only the idle + /// connections immediately and defers busy-connection cleanup until return. + /// [Fact] public void Clear_MixedBusyAndIdle_OnlyIdleDestroyedImmediately() { @@ -856,6 +1062,10 @@ out DbConnectionInternal? idleConnection Assert.Equal(0, pool.Count); } + /// + /// Verifies that connections created after a clear are stamped with the new generation and + /// are pooled and reused normally. + /// [Fact] public void Clear_NewConnectionsAfterClear_ArePooledNormally() { @@ -905,6 +1115,10 @@ out DbConnectionInternal? reusedConnection Assert.Equal(1, reusedConnection!.ClearGeneration); } + /// + /// Verifies that repeated clear operations do not corrupt pool state and that each clear + /// increments the pool generation as expected. + /// [Fact] public void Clear_MultipleClearCalls_DoNotCorruptState() { @@ -1157,10 +1371,22 @@ private static void BackdateReturnedTime(DbConnectionInternal connection, TimeSp #endregion #region Test classes + + /// + /// Test connection factory that always succeeds and captures the timeout budget passed in by + /// the pool so timeout propagation can be asserted. + /// internal class SuccessfulSqlConnectionFactory : SqlConnectionFactory { + /// + /// Gets the last timeout budget passed through by the pool to the factory. + /// internal TimeoutTimer? CapturedTimeout { get; private set; } + /// + /// Creates a successful stub internal connection and records the timeout budget used for + /// the creation attempt. + /// protected override DbConnectionInternal CreateConnection( SqlConnectionOptions options, ConnectionPoolKey poolKey, @@ -1174,8 +1400,16 @@ protected override DbConnectionInternal CreateConnection( } } + /// + /// Test connection factory that always throws the pooled-open timeout to exercise failure + /// paths in the pool. + /// internal class TimeoutSqlConnectionFactory : SqlConnectionFactory { + /// + /// Throws the pooled-open timeout exception to simulate a failed physical connection + /// creation. + /// protected override DbConnectionInternal CreateConnection( SqlConnectionOptions options, ConnectionPoolKey poolKey, @@ -1188,6 +1422,10 @@ protected override DbConnectionInternal CreateConnection( } } + /// + /// Minimal test double used by the pool tests to avoid + /// involving a real provider-specific connection implementation. + /// internal class StubDbConnectionInternal : DbConnectionInternal { #region Not Implemented Members @@ -1223,6 +1461,10 @@ internal override void ResetConnection() } #endregion + /// + /// Verifies that constructing the pool with a zero max pool size fails with the expected + /// capacity validation error. + /// [Fact] public void Constructor_WithZeroMaxPoolSize_ThrowsArgumentOutOfRangeException() { @@ -1242,7 +1484,7 @@ public void Constructor_WithZeroMaxPoolSize_ThrowsArgumentOutOfRangeException() poolGroupOptions ); - // Act & Assert + // Act var exception = Assert.Throws(() => new ChannelDbConnectionPool( SuccessfulConnectionFactory, @@ -1250,15 +1492,20 @@ public void Constructor_WithZeroMaxPoolSize_ThrowsArgumentOutOfRangeException() DbConnectionPoolIdentity.NoIdentity, new DbConnectionPoolProviderInfo() )); - + + // Assert Assert.Equal("fixedCapacity", exception.ParamName); Assert.Contains("Capacity must be greater than zero", exception.Message); } + /// + /// Verifies that large but valid max pool sizes pass capacity validation and either succeed + /// or fail only due to memory pressure rather than argument validation. + /// [Fact] public void Constructor_WithLargeMaxPoolSize() { - // Arrange - Test that Int32.MaxValue is accepted as a valid pool size + // Arrange var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -1276,7 +1523,7 @@ public void Constructor_WithLargeMaxPoolSize() try { - // Act & Assert - This should not throw ArgumentOutOfRangeException, but may throw OutOfMemoryException + // Act var pool = new ChannelDbConnectionPool( SuccessfulConnectionFactory, dbConnectionPoolGroup, @@ -1284,6 +1531,7 @@ public void Constructor_WithLargeMaxPoolSize() new DbConnectionPoolProviderInfo() ); + // Assert Assert.NotNull(pool); Assert.Equal(0, pool.Count); } @@ -1295,12 +1543,14 @@ public void Constructor_WithLargeMaxPoolSize() } } + /// + /// Verifies that small valid max pool sizes construct successfully and produce usable pool + /// instances. + /// [Fact] public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() { - // Arrange - Test various small pool sizes that should work correctly - - // Test with pool size of 1 + // Arrange var poolGroupOptions1 = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -1316,7 +1566,7 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() poolGroupOptions1 ); - // Act & Assert - Pool size of 1 should work + // Act var pool1 = new ChannelDbConnectionPool( SuccessfulConnectionFactory, dbConnectionPoolGroup1, @@ -1324,10 +1574,11 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() new DbConnectionPoolProviderInfo() ); + // Assert Assert.NotNull(pool1); Assert.Equal(0, pool1.Count); - // Test with pool size of 2 + // Arrange var poolGroupOptions2 = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -1343,6 +1594,7 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() poolGroupOptions2 ); + // Act var pool2 = new ChannelDbConnectionPool( SuccessfulConnectionFactory, dbConnectionPoolGroup2, @@ -1350,10 +1602,577 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() new DbConnectionPoolProviderInfo() ); + // Assert Assert.NotNull(pool2); Assert.Equal(0, pool2.Count); } + #region Rate Limiting And Blocking Period Tests + + /// + /// Verifies that a connection creation failure enters the blocking-period error state when + /// blocking is enabled for the pool. + /// + [Fact] + public void ErrorOccurred_FailureWithBlockingEnabled_BecomesTrue() + { + // Arrange + // Default PoolBlockingPeriod is Auto; localhost is non-Azure so blocking is enabled. + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + // Act + Assert.False(pool.ErrorOccurred); + + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + + // Assert + Assert.True(pool.ErrorOccurred); + } + + /// + /// Verifies that a connection creation failure does not enter the blocking-period error state + /// when the connection string disables blocking with NeverBlock. + /// + [Fact] + public void ErrorOccurred_FailureWithNeverBlock_StaysFalse() + { + // Arrange + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=NeverBlock;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + // Act + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + + // Assert - FR-007: NeverBlock must not enter the error state. + Assert.False(pool.ErrorOccurred); + } + + /// + /// Verifies that a connection creation failure enters the blocking-period error state when + /// the connection string explicitly enables AlwaysBlock. + /// + [Fact] + public void ErrorOccurred_FailureWithAlwaysBlock_BecomesTrue() + { + // Arrange + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=AlwaysBlock;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + // Act + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + + // Assert + Assert.True(pool.ErrorOccurred); + } + + /// + /// Verifies that once the pool enters the blocking period, subsequent synchronous requests + /// fail fast with the cached exception without attempting another physical open. + /// + [Fact] + public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() + { + // Arrange + var factory = new CountingTimeoutConnectionFactory(); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool(factory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + // Act + var first = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + Assert.True(pool.ErrorOccurred); + Assert.Equal(1, factory.CreateCount); + + // FR-006: subsequent requests inside the blocking window must fail fast with the + // cached exception without attempting another physical open. + var second = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + + // Assert - the second request reused the cached exception and did not invoke + // CreateConnection again while the pool remained in the error state. + Assert.Equal(first.Message, second.Message); + Assert.True(pool.ErrorOccurred); + Assert.Equal(1, factory.CreateCount); + } + + /// + /// Verifies that clearing the pool while in the blocking-period error state resets the + /// externally visible error indicator. + /// + [Fact] + public void Clear_InErrorState_ResetsErrorOccurred() + { + // Arrange + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + Assert.True(pool.ErrorOccurred); + + // Act - FR-011: Clear must reset the error state. + pool.Clear(); + + // Assert + Assert.False(pool.ErrorOccurred); + } + + /// + /// Verifies that a successful connection creation after a prior failure leaves the pool out + /// of the blocking-period error state. + /// + [Fact] + public void SuccessfulCreate_AfterFailure_ClearsErrorState() + { + // Arrange + var factory = new ToggleFailureConnectionFactory(); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool(factory, dbConnectionPoolGroup: dbConnectionPoolGroup); + + // First call fails and enters the error state. + factory.FailNextCreate = true; + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + Assert.True(pool.ErrorOccurred); + + // Manually clear the error flag (simulating the backoff timer firing) and then + // verify that a subsequent successful create clears the cached error state. FR-009. + pool.Clear(); + Assert.False(pool.ErrorOccurred); + + factory.FailNextCreate = false; + + // Act + var completed = pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out var conn); + + // Assert + Assert.True(completed); + Assert.NotNull(conn); + Assert.False(pool.ErrorOccurred); + } + + /// + /// Verifies that an available rate-limiter permit allows the pool to create a physical + /// connection immediately and that the permit is released after the open completes. + /// + [Fact] + public void RateLimiter_PermitAvailable_CreatesPhysicalConnection() + { + // Arrange + var factory = new CountingSuccessfulConnectionFactory(); + var rateLimiter = new TestRateLimiter(); + var pool = ConstructPool( + factory, + connectionCreationRateLimiter: rateLimiter); + + // Act + bool completed = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? connection); + + // Assert + Assert.True(completed); + Assert.NotNull(connection); + Assert.Equal(1, factory.CreateCount); + Assert.Equal(1, rateLimiter.AttemptAcquireCount); + Assert.Equal(0, rateLimiter.OutstandingPermitCount); + } + + /// + /// Verifies that when the rate limiter denies a new physical open, the caller falls back + /// to waiting for an existing connection to be returned instead of forcing a second create. + /// + [Fact] + public async Task RateLimiter_PermitDenied_ReusesReturnedConnection() + { + // Arrange + var factory = new CountingSuccessfulConnectionFactory(); + var rateLimiter = new TestRateLimiter(true, false); + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 2, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0); + var pool = ConstructPool( + factory, + poolGroupOptions: poolGroupOptions, + connectionCreationRateLimiter: rateLimiter); + SqlConnection firstOwner = new(); + + pool.TryGetConnection( + firstOwner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? firstConnection); + + // Act + Task waitingRequest = Task.Run(() => + { + pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? queuedConnection); + return queuedConnection; + }); + + Assert.True( + SpinWait.SpinUntil(() => rateLimiter.AttemptAcquireCount == 2, TimeSpan.FromSeconds(5)), + "Timed out waiting for the second request to reach the rate limiter."); + + pool.ReturnInternalConnection(firstConnection!, firstOwner); + DbConnectionInternal? reusedConnection = await waitingRequest; + + // Assert + Assert.Same(firstConnection, reusedConnection); + Assert.Equal(1, factory.CreateCount); + Assert.Equal(2, rateLimiter.AttemptAcquireCount); + Assert.Equal(0, rateLimiter.OutstandingPermitCount); + } + + /// + /// Verifies that failed connection attempts release any acquired rate-limiter lease so the + /// pool does not starve future callers after repeated failures. + /// + [Fact] + public async Task RateLimiter_LeaseDisposedOnFailure_DoesNotStarvePool() + { + // Arrange + // If the rate limiter lease were not disposed on failure, after N failures (where N is + // the limiter's permit count) every subsequent request would deadlock. Verify that we + // can keep getting failures back without ever blocking the thread pool. + var rateLimiter = new TestRateLimiter(); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=NeverBlock;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 4, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0)); + var pool = ConstructPool( + TimeoutConnectionFactory, + dbConnectionPoolGroup: dbConnectionPoolGroup, + connectionCreationRateLimiter: rateLimiter); + + // Act & Assert + for (int i = 0; i < 8; i++) + { + await Assert.ThrowsAsync(async () => + { + var tcs = new TaskCompletionSource(); + pool.TryGetConnection(new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _); + await tcs.Task; + }); + } + + Assert.Equal(8, rateLimiter.AttemptAcquireCount); + Assert.Equal(0, rateLimiter.OutstandingPermitCount); + } + + /// + /// Test connection factory that can be toggled between failure and success to exercise pool + /// recovery behavior after blocking-period entry. + /// + internal class ToggleFailureConnectionFactory : SqlConnectionFactory + { + /// + /// Gets or sets whether the next connection creation attempt should fail. + /// + public bool FailNextCreate { get; set; } + + /// + /// Creates a stub connection or throws the pooled-open timeout based on + /// . + /// + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (FailNextCreate) + { + throw ADP.PooledOpenTimeout(); + } + + return new StubDbConnectionInternal(); + } + } + + /// + /// Test connection factory that always throws the pooled-open timeout and records how many + /// physical connection creations the pool attempted, so blocking-period tests can assert + /// that subsequent requests fail fast without invoking another open. + /// + internal sealed class CountingTimeoutConnectionFactory : SqlConnectionFactory + { + /// + /// Gets the number of times the pool asked the factory to create a physical connection. + /// + internal int CreateCount { get; private set; } + + /// + /// Increments the creation counter and throws the pooled-open timeout exception to + /// simulate a failed physical connection creation. + /// + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + CreateCount++; + throw ADP.PooledOpenTimeout(); + } + } + + /// + /// Test connection factory that returns a successful stub connection each time and records + /// how many physical connection creations the pool attempted, so rate-limiting tests can + /// assert how often the pool actually opened a connection. + /// + internal sealed class CountingSuccessfulConnectionFactory : SqlConnectionFactory + { + /// + /// Gets the number of times the pool asked the factory to create a physical connection. + /// + internal int CreateCount { get; private set; } + + /// + /// Creates a successful stub internal connection and increments the creation counter. + /// + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + CreateCount++; + return new StubDbConnectionInternal(); + } + } + + /// + /// Minimal rate limiter test double that supports scripted allow/deny results while also + /// enforcing a single outstanding permit so lease disposal behavior can be verified. + /// + internal sealed class TestRateLimiter : RateLimiter + { + private readonly Queue _scriptedResults; + private int _outstandingPermitCount; + + /// + /// Initializes the limiter with an optional script of acquisition outcomes. + /// + /// Ordered allow/deny results for successive acquire attempts. + internal TestRateLimiter(params bool[] scriptedResults) + { + _scriptedResults = new Queue(scriptedResults); + } + + /// + /// Gets the number of synchronous acquire attempts made by the pool. + /// + internal int AttemptAcquireCount { get; private set; } + + /// + /// Gets the number of permits currently held by undisposed leases. + /// + internal int OutstandingPermitCount => Volatile.Read(ref _outstandingPermitCount); + + /// + /// Gets the idle duration for the test limiter. This limiter never tracks idle time. + /// + public override TimeSpan? IdleDuration => null; + + /// + /// Attempts to acquire a permit synchronously according to the scripted outcome and + /// current outstanding lease count. + /// + /// The requested permit count. + /// An acquired or denied lease for the requested permit. + protected override RateLimitLease AttemptAcquireCore(int permitCount) + { + AttemptAcquireCount++; + + if (permitCount != 1) + { + throw new NotSupportedException("Tests only support single-permit acquisition."); + } + + if (OutstandingPermitCount > 0) + { + return DeniedTestLease.Instance; + } + + if (_scriptedResults.Count > 0 && !_scriptedResults.Dequeue()) + { + return DeniedTestLease.Instance; + } + + Interlocked.Increment(ref _outstandingPermitCount); + return new AcquiredTestLease(this); + } + + /// + /// Attempts to acquire a permit asynchronously by delegating to the synchronous test + /// behavior. + /// + /// The requested permit count. + /// Ignored because this test limiter never queues. + /// A completed task containing the acquired or denied lease. + protected override ValueTask AcquireAsyncCore(int permitCount, CancellationToken cancellationToken) + => new(AttemptAcquireCore(permitCount)); + + /// + /// Gets statistics for the test limiter. Tests assert behavior through explicit counters + /// instead of the framework statistics object. + /// + /// because this test limiter does not expose framework statistics. + public override RateLimiterStatistics? GetStatistics() => null; + + private void ReleasePermit() + { + Interlocked.Decrement(ref _outstandingPermitCount); + } + + /// + /// Lease representing a successful single-permit acquisition. + /// + private sealed class AcquiredTestLease : RateLimitLease + { + private readonly TestRateLimiter _owner; + private int _disposed; + + internal AcquiredTestLease(TestRateLimiter owner) + { + _owner = owner; + } + + public override bool IsAcquired => true; + + public override IEnumerable MetadataNames => Array.Empty(); + + public override bool TryGetMetadata(string metadataName, out object? metadata) + { + metadata = null; + return false; + } + + protected override void Dispose(bool disposing) + { + if (disposing && Interlocked.Exchange(ref _disposed, 1) == 0) + { + _owner.ReleasePermit(); + } + } + } + + /// + /// Shared denied lease used when the test limiter refuses an acquisition. + /// + private sealed class DeniedTestLease : RateLimitLease + { + internal static readonly DeniedTestLease Instance = new(); + + public override bool IsAcquired => false; + + public override IEnumerable MetadataNames => Array.Empty(); + + public override bool TryGetMetadata(string metadataName, out object? metadata) + { + metadata = null; + return false; + } + + protected override void Dispose(bool disposing) + { + // No resources to release. + } + } + } + + #endregion + #region Connection Timeout Awareness Tests /// @@ -1482,3 +2301,4 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } } + diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj index d3405ed113..04afcf3910 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj @@ -101,6 +101,7 @@ + @@ -118,6 +119,7 @@ + From 4396432afe7827ea0173300ea79122df3d7d6527 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 11:00:30 -0700 Subject: [PATCH 02/39] Narrow pool rate limiter to ConcurrencyLimiter The connection pool only needs a concurrency limiter (pooling against on-prem SQL Server), so change ChannelDbConnectionPool to take a concrete System.Threading.RateLimiting.ConcurrencyLimiter? instead of the abstract RateLimiter base. The limiter remains optional (null = no limiting), and AttemptAcquire(1)/RateLimitLease usage is unchanged (both inherited). Rework the three rate-limiter unit tests to use real ConcurrencyLimiter instances and assert via GetStatistics() (CurrentAvailablePermits, TotalFailedLeases) instead of the now-removed TestRateLimiter double. Update the spec and diagram to describe a concurrency limiter specifically, noting other limiter types can be added later if needed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/006-pool-rate-limiting/diagrams.md | 2 +- specs/006-pool-rate-limiting/spec.md | 25 +-- .../ConnectionPool/ChannelDbConnectionPool.cs | 6 +- .../ChannelDbConnectionPoolTest.cs | 184 +++--------------- 4 files changed, 48 insertions(+), 169 deletions(-) diff --git a/specs/006-pool-rate-limiting/diagrams.md b/specs/006-pool-rate-limiting/diagrams.md index 4b3f3130d7..f05951d61d 100644 --- a/specs/006-pool-rate-limiting/diagrams.md +++ b/specs/006-pool-rate-limiting/diagrams.md @@ -26,7 +26,7 @@ flowchart TD Start([Open request]) --> Idle["Idle channel
TryRead
(non-blocking)"] Idle -->|got connection| Done([Return connection]) - Idle -->|empty| Limiter["RateLimiter
AttemptAcquire 1
(non-blocking)"] + Idle -->|empty| Limiter["ConcurrencyLimiter
AttemptAcquire 1
(non-blocking)"] Limiter -->|acquired lease| Open["Open physical connection"] Limiter -->|not acquired| Channel["Idle channel
await ReadAsync
(FIFO queued)"] diff --git a/specs/006-pool-rate-limiting/spec.md b/specs/006-pool-rate-limiting/spec.md index 2c3c164309..d0afa5ac88 100644 --- a/specs/006-pool-rate-limiting/spec.md +++ b/specs/006-pool-rate-limiting/spec.md @@ -98,19 +98,20 @@ Time spent waiting for rate limiter capacity counts against the caller's overall --- -### User Story 5 — Rate Limiter Built on System.Threading.RateLimiting (P3) +### User Story 5 — Rate Limiting Built on a Concurrency Limiter (P3) -The pool uses `System.Threading.RateLimiting.RateLimiter` as the base abstraction and -`ConcurrencyLimiter` as the initial implementation. No custom rate limiting primitives are -defined. +The pool supports an optional `System.Threading.RateLimiting.ConcurrencyLimiter` to throttle +concurrent physical connection creation. This is the only limiter type the pool currently needs +(pooling against on-prem SQL Server), so the pool takes a concrete `ConcurrencyLimiter?` rather +than the abstract `RateLimiter` base. Support for other limiter types can be added later if a +concrete need arises. When no limiter is supplied (`null`), no rate limiting is applied. **Acceptance Scenarios**: -1. **Given** the pool is configured with the default `ConcurrencyLimiter`, **When** connections +1. **Given** the pool is configured with a `ConcurrencyLimiter`, **When** connections are created, **Then** the limiter throttles concurrent creation to the configured maximum. -2. **Given** a different `RateLimiter` implementation is substituted, **When** connections are - created, **Then** the pool delegates throttling to the substituted implementation without - code changes to pool logic. +2. **Given** no limiter is supplied (`null`), **When** connections are created, **Then** the + pool applies no rate limiting. --- @@ -138,9 +139,9 @@ defined. `false` otherwise. - **FR-011**: `ClearPool` MUST clear the error state in addition to invalidating pooled connections. -- **FR-012**: The rate limiter MUST use `System.Threading.RateLimiting.RateLimiter` as the base - abstraction so that any `RateLimiter` implementation can be substituted without modifying - pool acquisition logic. -- **FR-013**: The initial implementation MUST use +- **FR-012**: The rate limiter MUST be an optional `System.Threading.RateLimiting.ConcurrencyLimiter`. + When no limiter is supplied (`null`), the pool MUST apply no rate limiting. Support for other + `RateLimiter` types is intentionally out of scope for now and may be added later if needed. +- **FR-013**: When a limiter is supplied, it MUST be a `System.Threading.RateLimiting.ConcurrencyLimiter` configured with the desired maximum number of concurrent connection creation attempts. 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 ca4b177064..7e461eddb6 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 @@ -101,12 +101,12 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable private int _shutdownInitiated; /// - /// Optional rate limiter that throttles the number of concurrent physical connection + /// Optional concurrency limiter that throttles the number of concurrent physical connection /// creation attempts. When null, no rate limiting is applied. A non-null limiter is /// supplied at pool construction time; there is no default. Callers fast-fail against /// the limiter and fall back to the idle-channel wait when no permit is available. /// - private readonly RateLimiter? _connectionCreationRateLimiter; + private readonly ConcurrencyLimiter? _connectionCreationRateLimiter; /// /// Encapsulates the blocking-period error state for this pool: cached exception, exponential @@ -124,7 +124,7 @@ internal ChannelDbConnectionPool( DbConnectionPoolGroup connectionPoolGroup, DbConnectionPoolIdentity identity, DbConnectionPoolProviderInfo connectionPoolProviderInfo, - RateLimiter? connectionCreationRateLimiter = null) + ConcurrencyLimiter? connectionCreationRateLimiter = null) { ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index ae76445897..a797af191a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -38,14 +38,14 @@ public class ChannelDbConnectionPoolTest /// Optional pool group override. /// Optional pool options override. /// Optional provider info override. - /// Optional rate limiter controlling physical connection creation. + /// Optional concurrency limiter controlling physical connection creation. /// A configured instance for testing. private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFactory, DbConnectionPoolIdentity? identity = null, DbConnectionPoolGroup? dbConnectionPoolGroup = null, DbConnectionPoolGroupOptions? poolGroupOptions = null, DbConnectionPoolProviderInfo? connectionPoolProviderInfo = null, - RateLimiter? connectionCreationRateLimiter = null) + ConcurrencyLimiter? connectionCreationRateLimiter = null) { poolGroupOptions ??= new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1824,7 +1824,8 @@ public void RateLimiter_PermitAvailable_CreatesPhysicalConnection() { // Arrange var factory = new CountingSuccessfulConnectionFactory(); - var rateLimiter = new TestRateLimiter(); + var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); var pool = ConstructPool( factory, connectionCreationRateLimiter: rateLimiter); @@ -1840,8 +1841,9 @@ public void RateLimiter_PermitAvailable_CreatesPhysicalConnection() Assert.True(completed); Assert.NotNull(connection); Assert.Equal(1, factory.CreateCount); - Assert.Equal(1, rateLimiter.AttemptAcquireCount); - Assert.Equal(0, rateLimiter.OutstandingPermitCount); + // The single permit was acquired for the open and released afterwards, so it is + // available again once the connection has been handed back to the caller. + Assert.Equal(1, rateLimiter.GetStatistics()!.CurrentAvailablePermits); } /// @@ -1853,7 +1855,8 @@ public async Task RateLimiter_PermitDenied_ReusesReturnedConnection() { // Arrange var factory = new CountingSuccessfulConnectionFactory(); - var rateLimiter = new TestRateLimiter(true, false); + var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -1868,11 +1871,21 @@ public async Task RateLimiter_PermitDenied_ReusesReturnedConnection() connectionCreationRateLimiter: rateLimiter); SqlConnection firstOwner = new(); + // The first open acquires and releases the single permit, creating a physical + // connection that the second request can later reuse. pool.TryGetConnection( firstOwner, taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? firstConnection); + Assert.NotNull(firstConnection); + Assert.Equal(1, factory.CreateCount); + + // Externally hold the only permit so the pool's next AttemptAcquire is denied and the + // waiting request must fall back to waiting for a returned connection. + using RateLimitLease heldLease = rateLimiter.AttemptAcquire(1); + Assert.True(heldLease.IsAcquired); + long failedLeasesBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; // Act Task waitingRequest = Task.Run(() => @@ -1886,8 +1899,10 @@ public async Task RateLimiter_PermitDenied_ReusesReturnedConnection() }); Assert.True( - SpinWait.SpinUntil(() => rateLimiter.AttemptAcquireCount == 2, TimeSpan.FromSeconds(5)), - "Timed out waiting for the second request to reach the rate limiter."); + SpinWait.SpinUntil( + () => rateLimiter.GetStatistics()!.TotalFailedLeases > failedLeasesBefore, + TimeSpan.FromSeconds(5)), + "Timed out waiting for the second request to be denied by the rate limiter."); pool.ReturnInternalConnection(firstConnection!, firstOwner); DbConnectionInternal? reusedConnection = await waitingRequest; @@ -1895,8 +1910,6 @@ public async Task RateLimiter_PermitDenied_ReusesReturnedConnection() // Assert Assert.Same(firstConnection, reusedConnection); Assert.Equal(1, factory.CreateCount); - Assert.Equal(2, rateLimiter.AttemptAcquireCount); - Assert.Equal(0, rateLimiter.OutstandingPermitCount); } /// @@ -1910,7 +1923,8 @@ public async Task RateLimiter_LeaseDisposedOnFailure_DoesNotStarvePool() // If the rate limiter lease were not disposed on failure, after N failures (where N is // the limiter's permit count) every subsequent request would deadlock. Verify that we // can keep getting failures back without ever blocking the thread pool. - var rateLimiter = new TestRateLimiter(); + var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 4, QueueLimit = 0 }); var dbConnectionPoolGroup = new DbConnectionPoolGroup( new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=NeverBlock;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), @@ -1938,8 +1952,12 @@ await Assert.ThrowsAsync(async () => }); } - Assert.Equal(8, rateLimiter.AttemptAcquireCount); - Assert.Equal(0, rateLimiter.OutstandingPermitCount); + // Every failed open must have released its permit; otherwise the pool would starve. + Assert.True( + SpinWait.SpinUntil( + () => rateLimiter.GetStatistics()!.CurrentAvailablePermits == 4, + TimeSpan.FromSeconds(5)), + "Rate limiter did not release all permits after failed opens."); } /// @@ -2031,146 +2049,6 @@ protected override DbConnectionInternal CreateConnection( } } - /// - /// Minimal rate limiter test double that supports scripted allow/deny results while also - /// enforcing a single outstanding permit so lease disposal behavior can be verified. - /// - internal sealed class TestRateLimiter : RateLimiter - { - private readonly Queue _scriptedResults; - private int _outstandingPermitCount; - - /// - /// Initializes the limiter with an optional script of acquisition outcomes. - /// - /// Ordered allow/deny results for successive acquire attempts. - internal TestRateLimiter(params bool[] scriptedResults) - { - _scriptedResults = new Queue(scriptedResults); - } - - /// - /// Gets the number of synchronous acquire attempts made by the pool. - /// - internal int AttemptAcquireCount { get; private set; } - - /// - /// Gets the number of permits currently held by undisposed leases. - /// - internal int OutstandingPermitCount => Volatile.Read(ref _outstandingPermitCount); - - /// - /// Gets the idle duration for the test limiter. This limiter never tracks idle time. - /// - public override TimeSpan? IdleDuration => null; - - /// - /// Attempts to acquire a permit synchronously according to the scripted outcome and - /// current outstanding lease count. - /// - /// The requested permit count. - /// An acquired or denied lease for the requested permit. - protected override RateLimitLease AttemptAcquireCore(int permitCount) - { - AttemptAcquireCount++; - - if (permitCount != 1) - { - throw new NotSupportedException("Tests only support single-permit acquisition."); - } - - if (OutstandingPermitCount > 0) - { - return DeniedTestLease.Instance; - } - - if (_scriptedResults.Count > 0 && !_scriptedResults.Dequeue()) - { - return DeniedTestLease.Instance; - } - - Interlocked.Increment(ref _outstandingPermitCount); - return new AcquiredTestLease(this); - } - - /// - /// Attempts to acquire a permit asynchronously by delegating to the synchronous test - /// behavior. - /// - /// The requested permit count. - /// Ignored because this test limiter never queues. - /// A completed task containing the acquired or denied lease. - protected override ValueTask AcquireAsyncCore(int permitCount, CancellationToken cancellationToken) - => new(AttemptAcquireCore(permitCount)); - - /// - /// Gets statistics for the test limiter. Tests assert behavior through explicit counters - /// instead of the framework statistics object. - /// - /// because this test limiter does not expose framework statistics. - public override RateLimiterStatistics? GetStatistics() => null; - - private void ReleasePermit() - { - Interlocked.Decrement(ref _outstandingPermitCount); - } - - /// - /// Lease representing a successful single-permit acquisition. - /// - private sealed class AcquiredTestLease : RateLimitLease - { - private readonly TestRateLimiter _owner; - private int _disposed; - - internal AcquiredTestLease(TestRateLimiter owner) - { - _owner = owner; - } - - public override bool IsAcquired => true; - - public override IEnumerable MetadataNames => Array.Empty(); - - public override bool TryGetMetadata(string metadataName, out object? metadata) - { - metadata = null; - return false; - } - - protected override void Dispose(bool disposing) - { - if (disposing && Interlocked.Exchange(ref _disposed, 1) == 0) - { - _owner.ReleasePermit(); - } - } - } - - /// - /// Shared denied lease used when the test limiter refuses an acquisition. - /// - private sealed class DeniedTestLease : RateLimitLease - { - internal static readonly DeniedTestLease Instance = new(); - - public override bool IsAcquired => false; - - public override IEnumerable MetadataNames => Array.Empty(); - - public override bool TryGetMetadata(string metadataName, out object? metadata) - { - metadata = null; - return false; - } - - protected override void Dispose(bool disposing) - { - // No resources to release. - } - } - } - #endregion #region Connection Timeout Awareness Tests From 86b4b2a895f7d1d3ca24697b8abba32c8d867a0f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 11:36:37 -0700 Subject: [PATCH 03/39] Replace rate-limit TODOs with rationale comment Remove the two "options to consider" TODOs above the AttemptAcquire call and replace them with a comment explaining why non-blocking fast-fail was chosen over failing immediately or blocking on the limiter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 7e461eddb6..cf65962792 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 @@ -577,9 +577,18 @@ public bool TryGetConnection( // configured we substitute a no-op acquired lease. // FR-001, FR-002, FR-003. - //TODO: some other options to consider: - // 1. fail immediately and surface an error to the caller - // 2. block on acquire (subject to overall timeout) - this would be the simplest + // We chose non-blocking fast-fail over the two alternatives: + // 1. Fail immediately and surface an error to the caller. Rejected because + // a denied permit doesn't mean the pool can't serve the request - an + // in-use connection may be returned momentarily, so falling back to the + // idle-channel wait recycles that connection instead of erroring out. + // 2. Block on AttemptAcquireAsync (subject to the overall timeout). Simpler, + // but it queues the caller on the limiter and forces a brand-new physical + // open even when a returning connection would satisfy it sooner; it also + // couples the caller's wait to limiter capacity rather than to the pool's + // existing "wake on returned/created connection" signaling. Fast-fail plus + // the idle-channel fallback prefers connection reuse and reuses one wait + // path for both sources of capacity. RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; bool leaseAcquired = lease.IsAcquired; try From 144a324c23b7cecbb3086dfeddc8504f3894e3d3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 11:37:03 -0700 Subject: [PATCH 04/39] Remove rate-limit TODOs from AttemptAcquire call Drop the two "options to consider" TODOs above the AttemptAcquire call. The rationale for choosing non-blocking fast-fail lives in the PR discussion rather than in code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index cf65962792..54eae73fff 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 @@ -577,18 +577,6 @@ public bool TryGetConnection( // configured we substitute a no-op acquired lease. // FR-001, FR-002, FR-003. - // We chose non-blocking fast-fail over the two alternatives: - // 1. Fail immediately and surface an error to the caller. Rejected because - // a denied permit doesn't mean the pool can't serve the request - an - // in-use connection may be returned momentarily, so falling back to the - // idle-channel wait recycles that connection instead of erroring out. - // 2. Block on AttemptAcquireAsync (subject to the overall timeout). Simpler, - // but it queues the caller on the limiter and forces a brand-new physical - // open even when a returning connection would satisfy it sooner; it also - // couples the caller's wait to limiter capacity rather than to the pool's - // existing "wake on returned/created connection" signaling. Fast-fail plus - // the idle-channel fallback prefers connection reuse and reuses one wait - // path for both sources of capacity. RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; bool leaseAcquired = lease.IsAcquired; try From 662b36d7b14b347571cfe20b51547cebd3cc6383 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 11:44:22 -0700 Subject: [PATCH 05/39] Inline leaseAcquired into lease.IsAcquired checks Remove the redundant leaseAcquired local; read lease.IsAcquired directly in the early-return guard and the finally-block poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 54eae73fff..f80af9a9f3 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 @@ -578,10 +578,9 @@ public bool TryGetConnection( // FR-001, FR-002, FR-003. RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; - bool leaseAcquired = lease.IsAcquired; try { - if (!leaseAcquired) + if (!lease.IsAcquired) { // TODO: When we fail to acquire a lease, surface the lease metadata // (e.g. RateLimitMetadataName.RetryAfter, ReasonPhrase) in the error @@ -628,7 +627,7 @@ public bool TryGetConnection( // return can satisfy a waiter. FR-004. This is best-effort; releasing a // lease doesn't guarantee the rate limiter immediately has an available // permit, but the waiter we wake will fall back to waiting again if not. - if (leaseAcquired && + if (lease.IsAcquired && _connectionCreationRateLimiter is not null && _connectionSlots.ReservationCount < MaxPoolSize) { From 87c86313e62e55e3b9df395034f0b465b5a6ff5f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 11:54:47 -0700 Subject: [PATCH 06/39] Add test that successful create releases its rate-limiter lease RateLimiter_SuccessfulCreate_ReleasesLeaseForNextCreate exercises a single-permit ConcurrencyLimiter with two sequential opens against distinct owners. A leaked lease on the success path would deny the second open, so asserting both create physical connections (CreateCount == 2) guards the release-on-success behavior at the behavioral level rather than only via the permit counter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTest.cs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index a797af191a..f5eeb1ef3c 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -1846,6 +1846,56 @@ public void RateLimiter_PermitAvailable_CreatesPhysicalConnection() Assert.Equal(1, rateLimiter.GetStatistics()!.CurrentAvailablePermits); } + /// + /// Verifies that a successful physical open releases its rate-limiter lease so that a + /// subsequent open can acquire the same permit. With a single-permit limiter, a leaked + /// lease would deny the second open and force connection reuse, leaving CreateCount at 1. + /// + [Fact] + public void RateLimiter_SuccessfulCreate_ReleasesLeaseForNextCreate() + { + // Arrange + var factory = new CountingSuccessfulConnectionFactory(); + var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 2, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0); + var pool = ConstructPool( + factory, + poolGroupOptions: poolGroupOptions, + connectionCreationRateLimiter: rateLimiter); + + // Act + // Two distinct owners so neither open can be satisfied by reusing the other's + // connection - each must acquire a fresh permit and create a physical connection. + bool firstCompleted = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? firstConnection); + bool secondCompleted = pool.TryGetConnection( + new SqlConnection(), + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? secondConnection); + + // Assert + Assert.True(firstCompleted); + Assert.True(secondCompleted); + Assert.NotNull(firstConnection); + Assert.NotNull(secondConnection); + Assert.NotSame(firstConnection, secondConnection); + // The second create only succeeds if the first release returned the single permit. + Assert.Equal(2, factory.CreateCount); + Assert.Equal(1, rateLimiter.GetStatistics()!.CurrentAvailablePermits); + } + /// /// Verifies that when the rate limiter denies a new physical open, the caller falls back /// to waiting for an existing connection to be returned instead of forcing a second create. From 19948c237ca86d6da1b7d37d783501bbd630651d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 12:01:38 -0700 Subject: [PATCH 07/39] Add test for lease-release wake path (FR-004) Cover the previously untested concurrency behavior where a caller blocked purely by rate limiting is woken by another caller's lease release (the finally-block null poke) and then creates its own physical connection. RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysicalConnection is a [Theory] over the sync and async idle-channel wait mechanisms. It uses a new GatedSuccessfulConnectionFactory that blocks the first physical create so the permit is held in-flight while a second caller is denied and parks on the idle channel; releasing the gate triggers the release poke that must wake and satisfy the waiter. Verified the test fails (waiter times out) when the poke is disabled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ChannelDbConnectionPoolTest.cs | 134 ++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index f5eeb1ef3c..9b04c2c201 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -1896,6 +1896,92 @@ public void RateLimiter_SuccessfulCreate_ReleasesLeaseForNextCreate() Assert.Equal(1, rateLimiter.GetStatistics()!.CurrentAvailablePermits); } + /// + /// Verifies the FR-004 wake path: a caller blocked purely because the rate limiter denied + /// its permit is woken when a different caller releases its lease, and then creates its own + /// physical connection (rather than reusing one, since the permit holder never returns its + /// connection). Exercises both the sync and async idle-channel wait mechanisms. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysicalConnection(bool async) + { + // Arrange + using var createGate = new ManualResetEventSlim(initialState: false); + var factory = new GatedSuccessfulConnectionFactory(createGate); + var rateLimiter = new ConcurrencyLimiter( + new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + // Room to grow so the release actually pokes a waiter (ReservationCount < MaxPoolSize). + maxPoolSize: 2, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0); + var pool = ConstructPool( + factory, + poolGroupOptions: poolGroupOptions, + connectionCreationRateLimiter: rateLimiter); + + Task Open(SqlConnection owner) + { + if (async) + { + // The async path dispatches the open onto the thread pool and completes the TCS, + // so this returns immediately while creation proceeds on another thread. + var tcs = new TaskCompletionSource(); + pool.TryGetConnection(owner, tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _); + return tcs.Task!; + } + + return Task.Run(() => + { + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? c); + return c; + }); + } + + // Act + // Caller A acquires the only permit and blocks inside creation, holding the permit. + SqlConnection ownerA = new(); + Task requestA = Open(ownerA); + Assert.True( + factory.FirstCreateStarted.Wait(TimeSpan.FromSeconds(5)), + "Timed out waiting for the first open to begin physical creation."); + + // Caller B is denied a permit (A holds it) and must fall back to the idle-channel wait. + long failedLeasesBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; + Task requestB = Open(new SqlConnection()); + Assert.True( + SpinWait.SpinUntil( + () => rateLimiter.GetStatistics()!.TotalFailedLeases > failedLeasesBefore, + TimeSpan.FromSeconds(5)), + "Timed out waiting for the second request to be denied by the rate limiter."); + + // Releasing A's create lets it finish and dispose its lease, which pokes the idle + // channel to wake B. B then finds the permit available and creates its own connection. + createGate.Set(); + + DbConnectionInternal? connectionA = await requestA; + DbConnectionInternal? connectionB = await requestB; + + // Assert + Assert.NotNull(connectionA); + Assert.NotNull(connectionB); + // B was woken by the lease-release poke and created a fresh connection; A never returned + // its connection, so this cannot be reuse. + Assert.NotSame(connectionA, connectionB); + Assert.Equal(2, factory.CreateCount); + Assert.Equal(1, rateLimiter.GetStatistics()!.CurrentAvailablePermits); + } + /// /// Verifies that when the rate limiter denies a new physical open, the caller falls back /// to waiting for an existing connection to be returned instead of forcing a second create. @@ -2099,6 +2185,54 @@ protected override DbConnectionInternal CreateConnection( } } + /// + /// Test connection factory that blocks inside its first physical creation until an external + /// gate is released, so a test can hold a rate-limiter permit in-flight while orchestrating + /// a second caller. Counts creations and signals when the first creation begins. + /// + internal sealed class GatedSuccessfulConnectionFactory : SqlConnectionFactory + { + private readonly ManualResetEventSlim _createGate; + private int _createCount; + + internal GatedSuccessfulConnectionFactory(ManualResetEventSlim createGate) + { + _createGate = createGate; + } + + /// + /// Gets the number of times the pool asked the factory to create a physical connection. + /// + internal int CreateCount => Volatile.Read(ref _createCount); + + /// + /// Signaled when the first physical creation begins, before it blocks on the gate. + /// + internal ManualResetEventSlim FirstCreateStarted { get; } = new(initialState: false); + + /// + /// Creates a successful stub connection. The first creation signals that it has started + /// and then blocks on the gate, holding whatever rate-limiter permit it acquired until + /// the test releases it; subsequent creations complete immediately. + /// + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (Interlocked.Increment(ref _createCount) == 1) + { + FirstCreateStarted.Set(); + _createGate.Wait(); + } + + return new StubDbConnectionInternal(); + } + } + #endregion #region Connection Timeout Awareness Tests From 0d60cd4fa916523b7e119a6a0e0aa47d0116b0ed Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 14 Jul 2026 12:10:35 -0700 Subject: [PATCH 08/39] Address Copilot review: OCE handling, redundant wake, docs, test disposal - Exclude OperationCanceledException from the creation-failure catch so a caller's own timeout/cancellation no longer poisons the pool blocking period. - Gate the finally idle-channel poke to non-faulted completion via a faulted flag, avoiding a redundant double wake on exception paths (cleanupCallback already writes a wake). - Document that the pool does not own the injected ConcurrencyLimiter and never disposes it (caller owns its lifetime). - Fix comment typo (rather then -> rather than) and trailing whitespace. - Reword spec User Story 1 / FR-002 from strict FIFO to best-effort idle-channel wait, matching the non-blocking AttemptAcquire implementation. - Dispose ConcurrencyLimiter instances in tests (using var) and drop the unused System.Collections.Generic using. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- specs/006-pool-rate-limiting/spec.md | 13 ++++--- .../ConnectionPool/ChannelDbConnectionPool.cs | 34 +++++++++++++------ .../ChannelDbConnectionPoolTest.cs | 11 +++--- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/specs/006-pool-rate-limiting/spec.md b/specs/006-pool-rate-limiting/spec.md index d0afa5ac88..d373c7d681 100644 --- a/specs/006-pool-rate-limiting/spec.md +++ b/specs/006-pool-rate-limiting/spec.md @@ -26,8 +26,11 @@ creation failure) with exponential backoff recovery, matching the existing ### User Story 1 — Throttled Connection Creation Under Burst Demand (P1) The pool limits the number of simultaneous physical connection creation attempts. Callers that -cannot immediately create a connection wait in FIFO order until the limiter allows them to -proceed, subject to their `ConnectTimeout`. +cannot immediately create a connection do not queue on the limiter; they fall back to waiting on +the idle channel, where they are satisfied either by a returned connection or by a best-effort +wake when another caller releases its permit. The idle channel preserves FIFO order for returned +connections, but rate-limit retries are best-effort rather than strictly ordered. All waiting is +subject to the caller's `ConnectTimeout`. **Acceptance Scenarios**: @@ -119,8 +122,10 @@ concrete need arises. When no limiter is supplied (`null`), no rate limiting is - **FR-001**: The pool MUST limit the number of concurrent physical connection creation attempts to a configurable maximum. -- **FR-002**: Callers that cannot immediately create a connection due to rate limiting MUST wait - in FIFO order until capacity is available or their timeout expires. +- **FR-002**: Callers that cannot immediately create a connection due to rate limiting MUST fall + back to waiting on the idle channel until capacity becomes available (via a returned connection + or a best-effort wake when a permit is released) or their timeout expires. Rate-limit retries are + best-effort and not strictly FIFO-ordered. - **FR-003**: Time spent waiting for rate limiter capacity MUST count against the caller's overall connection timeout budget. - **FR-004**: When a connection creation attempt completes (success or failure), the 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 f80af9a9f3..8a71cf3d00 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 @@ -105,6 +105,9 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// creation attempts. When null, no rate limiting is applied. A non-null limiter is /// supplied at pool construction time; there is no default. Callers fast-fail against /// the limiter and fall back to the idle-channel wait when no permit is available. + /// Lifetime note: the pool does not own this limiter and never disposes it. The caller that + /// constructs the limiter owns its lifetime, since a single limiter may be shared across + /// pools or outlive any one pool. /// private readonly ConcurrencyLimiter? _connectionCreationRateLimiter; @@ -572,12 +575,13 @@ public bool TryGetConnection( // We deliberately do not block here so the caller can fall back to // waiting on the idle channel, where it can be satisfied either by a // returning connection or by a null poke from another caller releasing - // its rate-limit lease (see finally below). We prefer to recycle existing - // connections rather then queue on the rate limit. When no limiter is + // its rate-limit lease (see finally below). We prefer to recycle existing + // connections rather than queue on the rate limit. When no limiter is // configured we substitute a no-op acquired lease. // FR-001, FR-002, FR-003. RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; + bool faulted = true; try { if (!lease.IsAcquired) @@ -585,6 +589,7 @@ 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. + faulted = false; return null; } @@ -609,6 +614,7 @@ public bool TryGetConnection( newConnection.ClearGeneration = _clearGeneration; } + faulted = false; return newConnection; } finally @@ -621,13 +627,17 @@ public bool TryGetConnection( lease.Dispose(); // After releasing, signal a waiter on the idle channel that they may now - // retry an open. We only poke when a limiter is configured (a waiter only - // falls back to the idle channel due to rate limiting in that case) and - // the pool can still grow; if we're at MaxPoolSize, only a connection - // return can satisfy a waiter. FR-004. This is best-effort; releasing a - // lease doesn't guarantee the rate limiter immediately has an available - // permit, but the waiter we wake will fall back to waiting again if not. - if (lease.IsAcquired && + // retry an open. We only poke on non-faulted completion: on exception paths + // the cleanupCallback below already writes a wake, so poking here too would + // produce a redundant double wake. We also only poke when a limiter is + // configured (a waiter only falls back to the idle channel due to rate + // limiting in that case) and the pool can still grow; if we're at + // MaxPoolSize, only a connection return can satisfy a waiter. FR-004. This + // is best-effort; releasing a lease doesn't guarantee the rate limiter + // immediately has an available permit, but the waiter we wake will fall + // back to waiting again if not. + if (!faulted && + lease.IsAcquired && _connectionCreationRateLimiter is not null && _connectionSlots.ReservationCount < MaxPoolSize) { @@ -656,9 +666,13 @@ _connectionCreationRateLimiter is not null && return connection; } - catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) { // Enter the blocking period error state on creation failure if configured. + // We deliberately exclude OperationCanceledException: that is thrown when the + // caller's own timeout/cancellation budget expires while waiting, which is + // client-side contention rather than a physical connection creation failure and + // must not poison the pool into fast-fail/backoff for other callers. // FR-006, FR-007. _errorState?.Enter(ex); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 9b04c2c201..b200e6ffc5 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System; -using System.Collections.Generic; using System.Collections.Concurrent; using System.Data.Common; using System.Threading; @@ -1824,7 +1823,7 @@ public void RateLimiter_PermitAvailable_CreatesPhysicalConnection() { // Arrange var factory = new CountingSuccessfulConnectionFactory(); - var rateLimiter = new ConcurrencyLimiter( + using var rateLimiter = new ConcurrencyLimiter( new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); var pool = ConstructPool( factory, @@ -1856,7 +1855,7 @@ public void RateLimiter_SuccessfulCreate_ReleasesLeaseForNextCreate() { // Arrange var factory = new CountingSuccessfulConnectionFactory(); - var rateLimiter = new ConcurrencyLimiter( + using var rateLimiter = new ConcurrencyLimiter( new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1910,7 +1909,7 @@ public async Task RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysical // Arrange using var createGate = new ManualResetEventSlim(initialState: false); var factory = new GatedSuccessfulConnectionFactory(createGate); - var rateLimiter = new ConcurrencyLimiter( + using var rateLimiter = new ConcurrencyLimiter( new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1991,7 +1990,7 @@ public async Task RateLimiter_PermitDenied_ReusesReturnedConnection() { // Arrange var factory = new CountingSuccessfulConnectionFactory(); - var rateLimiter = new ConcurrencyLimiter( + using var rateLimiter = new ConcurrencyLimiter( new ConcurrencyLimiterOptions { PermitLimit = 1, QueueLimit = 0 }); var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -2059,7 +2058,7 @@ public async Task RateLimiter_LeaseDisposedOnFailure_DoesNotStarvePool() // If the rate limiter lease were not disposed on failure, after N failures (where N is // the limiter's permit count) every subsequent request would deadlock. Verify that we // can keep getting failures back without ever blocking the thread pool. - var rateLimiter = new ConcurrencyLimiter( + using var rateLimiter = new ConcurrencyLimiter( new ConcurrencyLimiterOptions { PermitLimit = 4, QueueLimit = 0 }); var dbConnectionPoolGroup = new DbConnectionPoolGroup( new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=NeverBlock;"), From 9b91a459eaad2af7070bd00166a77132658e4719 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 15:13:03 -0700 Subject: [PATCH 09/39] Remove instance-level ForceNewConnection property. Replace with explicit method parameters. --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 36 ++++++++++--------- .../SqlConnectionStateTransitionTests.cs | 4 +-- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index d63571bd55..594f2eaf1d 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1595,8 +1595,8 @@ public override void EnlistTransaction(System.Transactions.Transaction transacti public override void Open() => Open(SqlConnectionOverrides.None); - private bool TryOpenWithRetry(TaskCompletionSource retry, bool forceNewConnection, SqlConnectionOverrides overrides) - => RetryLogicProvider.Execute(this, () => TryOpen(retry, forceNewConnection, overrides)); + private bool TryOpenWithRetry(TaskCompletionSource retry, SqlConnectionOverrides overrides) + => RetryLogicProvider.Execute(this, () => TryOpen(retry, false, overrides)); /// public void Open(SqlConnectionOverrides overrides) @@ -1616,7 +1616,7 @@ public void Open(SqlConnectionOverrides overrides) { statistics = SqlStatistics.StartTimer(Statistics); - if (!(IsProviderRetriable ? TryOpenWithRetry(null, false, overrides) : TryOpen(null, false, overrides))) + if (!(IsProviderRetriable ? TryOpenWithRetry(null, overrides) : TryOpen(null, false, overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -1688,15 +1688,7 @@ private async Task ReconnectAsync(int timeout) #endif try { - if (IsProviderRetriable) - { - await InternalOpenWithRetryAsync(SqlConnectionOverrides.None, forceNewConnection: true, ctoken).ConfigureAwait(false); - } - else - { - await InternalOpenAsync(SqlConnectionOverrides.None, forceNewConnection: true, ctoken).ConfigureAwait(false); - } - + await ReconnectAsync(ctoken).ConfigureAwait(false); // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. _reconnectCount = unchecked(_reconnectCount + 1); #if DEBUG @@ -1917,12 +1909,21 @@ public override Task OpenAsync(CancellationToken cancellationToken) /// public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) => IsProviderRetriable ? - InternalOpenWithRetryAsync(overrides, forceNewConnection: false, cancellationToken) : - InternalOpenAsync(overrides, forceNewConnection: false, cancellationToken); + InternalOpenWithRetryAsync(overrides, false, cancellationToken) : + InternalOpenAsync(overrides, false, cancellationToken); + + internal Task ReconnectAsync(CancellationToken cancellationToken) + => ReconnectAsync(SqlConnectionOverrides.None, cancellationToken); + + internal Task ReconnectAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) + => IsProviderRetriable ? + InternalOpenWithRetryAsync(overrides, true, cancellationToken) : + InternalOpenAsync(overrides, true, cancellationToken); private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, forceNewConnection, cancellationToken), cancellationToken); + private Task InternalOpenAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) { long scopeID = SqlClientEventSource.Log.TryPoolerScopeEnterEvent("SqlConnection.InternalOpenAsync | API | Object Id {0}", ObjectID); @@ -2107,7 +2108,7 @@ public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource retry, bool forc /// The inner connection is snapshotted after the open call so downstream parser access uses a single observed /// instance and does not rely on a second racy read of . /// - /// forceNewConnection may only be true when the connection is already open (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 + /// 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. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs index 2955a6825e..bd47826de5 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs @@ -27,7 +27,7 @@ public void TryOpenInner_WhenInnerConnectionChangesAfterTryOpen_ThrowsInvalidOpe bool initialized = connection.SetInnerConnectionFrom(new TestRaceDbConnectionInternal(), DbConnectionClosedNeverOpened.SingletonInstance); Assert.True(initialized); - InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, forceNewConnection: false)); + InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, false)); Assert.NotNull(exception); Assert.Same(DbConnectionClosedPreviouslyOpened.SingletonInstance, connection.InnerConnection); } @@ -42,7 +42,7 @@ public void TryOpenInner_WhenInnerConnectionChangesAfterTryReplace_ThrowsInvalid bool initialized = connection.SetInnerConnectionFrom(new TestRaceDbConnectionInternal(), DbConnectionClosedNeverOpened.SingletonInstance); Assert.True(initialized); - InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, forceNewConnection: true)); + InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, true)); Assert.NotNull(exception); Assert.Same(DbConnectionClosedPreviouslyOpened.SingletonInstance, connection.InnerConnection); } From 08d24b36ece7b5fbde1d13c491a9d0991577c825 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 15:30:59 -0700 Subject: [PATCH 10/39] Fix initialization. Add doc comments. --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 594f2eaf1d..ed52883697 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1912,9 +1912,35 @@ public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancel InternalOpenWithRetryAsync(overrides, false, cancellationToken) : InternalOpenAsync(overrides, false, cancellationToken); + /// + /// Re-establishes the underlying physical connection for an already-open , + /// replacing its current (broken) inner connection with a fresh one while preserving the outer + /// object. Used by the connection-resiliency reconnect path. + /// + /// A token that cancels the reconnect attempt. + /// A task that completes when the connection has been re-established. + /// + /// Unlike , this forces a new underlying connection (it must + /// only be called when the connection is already open), so the replacement flows through + /// with forceNewConnection set to . + /// internal Task ReconnectAsync(CancellationToken cancellationToken) => ReconnectAsync(SqlConnectionOverrides.None, cancellationToken); + /// + /// Re-establishes the underlying physical connection for an already-open , + /// replacing its current (broken) inner connection with a fresh one while preserving the outer + /// object. Used by the connection-resiliency reconnect path. + /// + /// Options that relax some of the checks performed when opening the connection. + /// A token that cancels the reconnect attempt. + /// A task that completes when the connection has been re-established. + /// + /// Unlike , this forces a new + /// underlying connection (it must only be called when the connection is already open), so the + /// replacement flows through with forceNewConnection set to + /// . The provider retry logic is applied when . + /// internal Task ReconnectAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) => IsProviderRetriable ? InternalOpenWithRetryAsync(overrides, true, cancellationToken) : @@ -2108,7 +2134,7 @@ public OpenAsyncRetry(SqlConnection parent, TaskCompletionSource Date: Mon, 29 Jun 2026 15:37:17 -0700 Subject: [PATCH 11/39] Remove unnecessary internal API surface. --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 44 ++++--------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index ed52883697..5caca41988 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1688,7 +1688,15 @@ private async Task ReconnectAsync(int timeout) #endif try { - await ReconnectAsync(ctoken).ConfigureAwait(false); + if (IsProviderRetriable) + { + await InternalOpenWithRetryAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); + } + else + { + await InternalOpenAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); + } + // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. _reconnectCount = unchecked(_reconnectCount + 1); #if DEBUG @@ -1912,40 +1920,6 @@ public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancel InternalOpenWithRetryAsync(overrides, false, cancellationToken) : InternalOpenAsync(overrides, false, cancellationToken); - /// - /// Re-establishes the underlying physical connection for an already-open , - /// replacing its current (broken) inner connection with a fresh one while preserving the outer - /// object. Used by the connection-resiliency reconnect path. - /// - /// A token that cancels the reconnect attempt. - /// A task that completes when the connection has been re-established. - /// - /// Unlike , this forces a new underlying connection (it must - /// only be called when the connection is already open), so the replacement flows through - /// with forceNewConnection set to . - /// - internal Task ReconnectAsync(CancellationToken cancellationToken) - => ReconnectAsync(SqlConnectionOverrides.None, cancellationToken); - - /// - /// Re-establishes the underlying physical connection for an already-open , - /// replacing its current (broken) inner connection with a fresh one while preserving the outer - /// object. Used by the connection-resiliency reconnect path. - /// - /// Options that relax some of the checks performed when opening the connection. - /// A token that cancels the reconnect attempt. - /// A task that completes when the connection has been re-established. - /// - /// Unlike , this forces a new - /// underlying connection (it must only be called when the connection is already open), so the - /// replacement flows through with forceNewConnection set to - /// . The provider retry logic is applied when . - /// - internal Task ReconnectAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) - => IsProviderRetriable ? - InternalOpenWithRetryAsync(overrides, true, cancellationToken) : - InternalOpenAsync(overrides, true, cancellationToken); - private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, forceNewConnection, cancellationToken), cancellationToken); From e350b42d1d3d3b99ec55c4071a1fc3c011713f5f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 15:51:09 -0700 Subject: [PATCH 12/39] Expose param. --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 5caca41988..d30b636442 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1595,8 +1595,8 @@ public override void EnlistTransaction(System.Transactions.Transaction transacti public override void Open() => Open(SqlConnectionOverrides.None); - private bool TryOpenWithRetry(TaskCompletionSource retry, SqlConnectionOverrides overrides) - => RetryLogicProvider.Execute(this, () => TryOpen(retry, false, overrides)); + private bool TryOpenWithRetry(TaskCompletionSource retry, bool forceNewConnection,SqlConnectionOverrides overrides) + => RetryLogicProvider.Execute(this, () => TryOpen(retry, forceNewConnection, overrides)); /// public void Open(SqlConnectionOverrides overrides) @@ -1616,7 +1616,7 @@ public void Open(SqlConnectionOverrides overrides) { statistics = SqlStatistics.StartTimer(Statistics); - if (!(IsProviderRetriable ? TryOpenWithRetry(null, overrides) : TryOpen(null, false, overrides))) + if (!(IsProviderRetriable ? TryOpenWithRetry(null, false, overrides) : TryOpen(null, false, overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -1696,7 +1696,7 @@ private async Task ReconnectAsync(int timeout) { await InternalOpenAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); } - + // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. _reconnectCount = unchecked(_reconnectCount + 1); #if DEBUG From e2e1a72e9d5fa3cd6b21cf771852138b2423b368 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 16:08:21 -0700 Subject: [PATCH 13/39] Address broken test and copilot comments. --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 4 ++-- .../Data/SqlClient/SqlConnectionConcurrentOpenTests.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index d30b636442..c6ac2b0658 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2259,8 +2259,8 @@ private bool TryOpen(TaskCompletionSource retry, bool forc /// The inner connection is snapshotted after the open call so downstream parser access uses a single observed /// instance and does not rely on a second racy read of . /// - /// forceNewConnection may only be true when the connection is already open (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 + /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never + /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is /// currently disconnected. If the connection is currently open, passing false will result in an exception. See SqlConnection state /// transitions and subclasses for more details. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs index 17ac619a3e..0ad965913e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs @@ -86,7 +86,7 @@ public void TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState Exception ex = Assert.ThrowsAny(() => { - connection.TryOpenInner(completedRetry, forceNewConnection: false); + connection.TryOpenInner(completedRetry, false); }); Assert.True( From 63edefe969dd8e8667fb9053c2a576149428a190 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 29 Jun 2026 16:12:23 -0700 Subject: [PATCH 14/39] WIP --- .../ConnectionPool/ChannelDbConnectionPool.cs | 107 +++- .../ConnectionPool/ConnectionPoolSlots.cs | 20 + .../ConnectionPool/IDbConnectionPool.cs | 2 +- ...elDbConnectionPoolReplaceConnectionTest.cs | 502 ++++++++++++++++++ .../ChannelDbConnectionPoolTest.cs | 14 +- 5 files changed, 637 insertions(+), 8 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 8a71cf3d00..0d8c2e0c54 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 @@ -265,12 +265,109 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) } /// - public DbConnectionInternal ReplaceConnection( + public DbConnectionInternal? ReplaceConnection( DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout) { - throw new NotImplementedException(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, replacing connection.", Id); + + // Acquire a replacement that reuses the broken connection's pool slot so capacity is preserved + // (FR-005). Prefers an idle connection (Story 6); otherwise establishes a new one. Throws (after + // disposing the old connection and freeing its slot) if establishing a new connection fails (FR-006). + DbConnectionInternal? newConnection = GetReplacementConnection(owningObject, oldConnection, timeout); + if (newConnection is null) + { + // No slot could be secured for a new connection; let the caller's retry logic handle it. + return null; + } + + // Activate the replacement under the old connection's ambient transaction (FR-002/FR-003). If + // activation fails, PrepareConnection returns the new connection to the pool and rethrows (FR-007). + // TODO: Full transaction enlistment support (Story 2). + PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); + + // FR-004: Deactivate and dispose the old connection now that the replacement is active. + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); + + // FR-008: Record a soft connect metric since the caller's connection object is reused. + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, connection replaced successfully.", Id); + + return newConnection; + } + + /// + /// Obtains a connection to replace , reusing its pool slot so the + /// pool's connection count is unchanged (FR-005). An idle connection is preferred (Story 6); + /// otherwise a new physical connection is established. + /// + /// The connection that will own the replacement. + /// The broken connection being replaced. + /// The overall timeout budget for establishing a new connection. + /// + /// The replacement connection, already placed in a pool slot, or if a new + /// connection was established but no slot could be secured for it. + /// + /// + /// Propagated when establishing a new connection fails. Before rethrowing, the old connection is + /// deactivated and removed from the pool so its slot is not leaked (FR-006 / Story 4). + /// + private DbConnectionInternal? GetReplacementConnection( + DbConnection owningObject, + DbConnectionInternal oldConnection, + TimeoutTimer timeout) + { + // Story 6: prefer an idle connection over establishing a new physical one. + DbConnectionInternal? idleConnection = GetIdleConnection(); + if (idleConnection is not null) + { + // The idle connection already holds its own slot, so just release the broken connection's + // slot to keep the pool count correct (FR-005 / Story 6). + _connectionSlots.TryRemove(oldConnection); + return idleConnection; + } + + DbConnectionInternal newConnection; + try + { + newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + } + catch + { + // FR-006 / Story 4: Establishing the replacement failed (e.g., the server is still + // unreachable). Deactivate and remove the broken connection so its slot is not leaked, then + // propagate. RemoveConnection disposes it and clears its Pool reference, so the caller's + // eventual Close() follows the non-pooled path and will not touch the freed slot again. + oldConnection.DeactivateConnection(); + RemoveConnection(oldConnection); + throw; + } + + // Stamp the new connection with the current generation so a later Clear() can discard it. (Idle + // connections already carry their birth generation, so only newly established ones are stamped.) + newConnection.ClearGeneration = _clearGeneration; + + // Atomically swap the new connection into the broken connection's slot so the reservation count + // is unchanged (FR-005). + if (!_connectionSlots.TryReplace(oldConnection, newConnection)) + { + // The old slot was already removed (e.g., by a concurrent Clear). Reserve a fresh slot + // instead; if the pool is now full, give up and dispose the orphaned connection. + if (_connectionSlots.Add( + createCallback: () => newConnection, + cleanupCallback: (conn) => conn?.Dispose()) is null) + { + newConnection.Dispose(); + return null; + } + } + + return newConnection; } /// @@ -888,10 +985,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) { @@ -901,8 +999,7 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c try { - //TODO: pass through transaction - connection.ActivateConnection(null); + connection.ActivateConnection(transaction); } catch { diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs index 55eb88f02c..c9d268fd29 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs @@ -170,6 +170,26 @@ internal bool TryRemove(DbConnectionInternal connection) return false; } + /// + /// Atomically replaces an existing connection with a new one in the same slot. + /// The reservation count is unchanged because the slot is reused. + /// + /// The connection currently occupying the slot. + /// The connection to place into the slot. + /// True if the old connection was found and replaced; otherwise, false. + internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInternal newConnection) + { + for (int i = 0; i < _connections.Length; i++) + { + if (Interlocked.CompareExchange(ref _connections[i], newConnection, oldConnection) == oldConnection) + { + return true; + } + } + + return false; + } + /// /// Attempts to reserve a spot in the collection. /// diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs index 3799cf54ea..25185b2cc3 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 @@ -133,7 +133,7 @@ internal interface IDbConnectionPool /// The internal connection currently associated with the owning object. /// The overall timeout budget for this connection request. /// A reference to the new DbConnectionInternal. - DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); + DbConnectionInternal? ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); /// /// Returns an internal connection to the pool. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs new file mode 100644 index 0000000000..44015a76ea --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -0,0 +1,502 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Data.Common; +using System.Threading; +using System.Transactions; +using Microsoft.Data.Common; +using Microsoft.Data.Common.ConnectionString; +using Microsoft.Data.ProviderBase; +using Microsoft.Data.SqlClient.ConnectionPool; +using Microsoft.Data.SqlClient.Tests.Common; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + public class ChannelDbConnectionPoolReplaceConnectionTest + { + private static readonly SqlConnectionFactory SuccessfulConnectionFactory = new SuccessfulSqlConnectionFactory(); + + private ChannelDbConnectionPool ConstructPool( + SqlConnectionFactory connectionFactory, + DbConnectionPoolGroupOptions? poolGroupOptions = null) + { + poolGroupOptions ??= new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 50, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions("Data Source=localhost;"), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions + ); + return new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo() + ); + } + + #region Story 1 — Transparent Replacement + + [Fact] + public void ReplaceConnection_ReturnsNewConnection() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Act + var newConnection = pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); + } + + [Fact] + public void ReplaceConnection_OldConnectionIsDisposed() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Act + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — the old connection should be disposed (not poolable) + Assert.False(oldConnection.CanBePooled); + } + + #endregion + + #region Story 3 — Pool Capacity Preservation (new physical connection path) + + [Fact] + public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() + { + // Arrange — single connection, no idle connections available + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + Assert.Equal(0, pool.IdleCount); + int countBefore = pool.Count; + + // Act + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — slot was reused, count unchanged + Assert.Equal(countBefore, pool.Count); + } + + [Fact] + public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() + { + // Arrange — fill pool to max capacity, no idle connections + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 3, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions); + + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + SqlConnection owner3 = new(); + + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + pool.TryGetConnection(owner3, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn3); + + Assert.Equal(3, pool.Count); + + // Act — replace connection in a full pool + var newConnection = pool.ReplaceConnection( + owner1, + conn1!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — pool count must not exceed max + Assert.NotNull(newConnection); + Assert.Equal(3, pool.Count); + } + + #endregion + + #region Story 4 — Replacement Failure Propagation + + [Fact] + public void ReplaceConnection_CreationFails_ExceptionPropagated() + { + // Arrange — use a factory that succeeds initially then fails + var switchableFactory = new SwitchableSqlConnectionFactory(); + var pool = ConstructPool(switchableFactory); + SqlConnection owner = new(); + + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Switch to failing mode + switchableFactory.ShouldFail = true; + + // Act & Assert — exception from factory is propagated + Assert.Throws(() => + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + } + + [Fact] + public void ReplaceConnection_CreationFails_OldSlotReleased() + { + // Arrange — fill the pool to capacity so a leaked slot would be observable. + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: 2, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true, + idleTimeout: 0 + ); + var switchableFactory = new SwitchableSqlConnectionFactory(); + var pool = ConstructPool(switchableFactory, poolGroupOptions); + + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? oldConnection); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? otherConnection); + + Assert.NotNull(oldConnection); + Assert.Equal(2, pool.Count); + + // Switch to failing mode so the replacement creation throws. + switchableFactory.ShouldFail = true; + + // Act — replacement fails + Assert.Throws(() => + pool.ReplaceConnection( + owner1, + oldConnection!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // Assert — FR-006: the old connection's slot was released (no capacity leak). + Assert.Equal(1, pool.Count); + // Old connection was disposed as part of the failure cleanup. + Assert.False(oldConnection!.CanBePooled); + // FR-006 scenario 3: the pool is not left in an error state. + Assert.False(pool.ErrorOccurred); + + // The freed slot is usable again — a fresh request can succeed. + switchableFactory.ShouldFail = false; + pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? replacementForSlot); + Assert.NotNull(replacementForSlot); + Assert.Equal(2, pool.Count); + } + + #endregion + + #region Story 5 — Activation Failure Rollback + + [Fact] + public void ReplaceConnection_ActivationFails_ExceptionPropagated() + { + // Arrange + var factory = new ActivationFailSqlConnectionFactory(); + var pool = ConstructPool(factory); + SqlConnection owner = new(); + + factory.FailOnActivate = false; + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + + // Now make activation fail for the replacement + factory.FailOnActivate = true; + + // Act & Assert + Assert.Throws(() => + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + } + + [Fact] + public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() + { + // Arrange + var factory = new ActivationFailSqlConnectionFactory(); + var pool = ConstructPool(factory); + SqlConnection owner = new(); + + factory.FailOnActivate = false; + pool.TryGetConnection( + owner, + taskCompletionSource: null, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), + out DbConnectionInternal? oldConnection); + + Assert.NotNull(oldConnection); + int countBefore = pool.Count; + + // Make activation fail + factory.FailOnActivate = true; + + // Act + try + { + pool.ReplaceConnection( + owner, + oldConnection, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + } + catch (InvalidOperationException) + { + // Expected + } + + // Assert — the new connection was returned to pool (not leaked). + // Pool count stays same because the new connection replaced the old one's slot + // and was then returned to idle. + Assert.Equal(countBefore, pool.Count); + } + + #endregion + + #region Story 6 — Prefer Idle Connection + + [Fact] + public void ReplaceConnection_PrefersIdleOverNewConnection() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + + Assert.NotNull(conn1); + Assert.NotNull(conn2); + + // Return conn2 to make it idle + pool.ReturnInternalConnection(conn2, owner2); + Assert.Equal(1, pool.IdleCount); + + // Act — replace conn1; should prefer the idle conn2 + var newConnection = pool.ReplaceConnection( + owner1, + conn1, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert — should reuse the idle connection + Assert.NotNull(newConnection); + Assert.Same(conn2, newConnection); + Assert.Equal(0, pool.IdleCount); + } + + [Fact] + public void ReplaceConnection_NoIdleConnection_CreatesNew() + { + // Arrange + var pool = ConstructPool(SuccessfulConnectionFactory); + SqlConnection owner = new(); + + pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + Assert.NotNull(conn1); + Assert.Equal(0, pool.IdleCount); + + // Act — no idle connections available, should create new + var newConnection = pool.ReplaceConnection( + owner, + conn1, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + + // Assert + Assert.NotNull(newConnection); + Assert.NotSame(conn1, newConnection); + Assert.Equal(1, pool.Count); + } + + #endregion + + #region Test Helper Classes + + internal class SuccessfulSqlConnectionFactory : SqlConnectionFactory + { + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new StubDbConnectionInternal(); + } + } + + internal class SwitchableSqlConnectionFactory : SqlConnectionFactory + { + internal bool ShouldFail { get; set; } + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + if (ShouldFail) + { + throw new InvalidOperationException("Simulated connection failure"); + } + return new StubDbConnectionInternal(); + } + } + + internal class ActivationFailSqlConnectionFactory : SqlConnectionFactory + { + internal bool FailOnActivate { get; set; } + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection, + TimeoutTimer timeout) + { + return new ActivationFailDbConnectionInternal(this); + } + } + + internal class StubDbConnectionInternal : DbConnectionInternal + { + public override string ServerVersion => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction transaction) + { + return; + } + + protected override void Activate(Transaction transaction) + { + return; + } + + protected override void Deactivate() + { + return; + } + + internal override void ResetConnection() + { + return; + } + } + + internal class ActivationFailDbConnectionInternal : DbConnectionInternal + { + private readonly ActivationFailSqlConnectionFactory _factory; + + internal ActivationFailDbConnectionInternal(ActivationFailSqlConnectionFactory factory) + { + _factory = factory; + } + + public override string ServerVersion => throw new NotImplementedException(); + + public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) + { + throw new NotImplementedException(); + } + + public override void EnlistTransaction(Transaction transaction) + { + return; + } + + protected override void Activate(Transaction transaction) + { + if (_factory.FailOnActivate) + { + throw new InvalidOperationException("Simulated activation failure"); + } + } + + protected override void Deactivate() + { + return; + } + + internal override void ResetConnection() + { + return; + } + } + + #endregion + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index b200e6ffc5..0f5d802750 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -880,9 +880,19 @@ public void TestReplaceConnection() { // Arrange var pool = ConstructPool(SuccessfulConnectionFactory); + 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); + + Assert.NotNull(oldConnection); + + var newConnection = pool.ReplaceConnection(owner, oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); } /// From 949d9b2cc9716f455b239d50ff6508c319fdd447 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 7 Jul 2026 18:46:17 -0700 Subject: [PATCH 15/39] Add unit tests. --- ...elDbConnectionPoolReplaceConnectionTest.cs | 41 +++++ .../ConnectionPool/ConnectionPoolSlotsTest.cs | 151 ++++++++++++++++++ 2 files changed, 192 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 44015a76ea..3b66813339 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -47,6 +47,10 @@ private ChannelDbConnectionPool ConstructPool( #region Story 1 — Transparent Replacement + /// + /// Verifies that returns a + /// non-null connection that is a different instance from the one being replaced. + /// [Fact] public void ReplaceConnection_ReturnsNewConnection() { @@ -73,6 +77,10 @@ public void ReplaceConnection_ReturnsNewConnection() Assert.NotSame(oldConnection, newConnection); } + /// + /// Verifies that after a replacement the old connection is disposed and can no longer + /// be pooled. + /// [Fact] public void ReplaceConnection_OldConnectionIsDisposed() { @@ -102,6 +110,10 @@ public void ReplaceConnection_OldConnectionIsDisposed() #region Story 3 — Pool Capacity Preservation (new physical connection path) + /// + /// Verifies that replacing a connection when no idle connections are available reuses + /// the old connection's slot so the pool's total count remains unchanged. + /// [Fact] public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() { @@ -129,6 +141,10 @@ public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() Assert.Equal(countBefore, pool.Count); } + /// + /// Verifies that replacing a connection in a pool that is already filled to its maximum + /// capacity succeeds without exceeding the maximum pool size. + /// [Fact] public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() { @@ -169,6 +185,10 @@ public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() #region Story 4 — Replacement Failure Propagation + /// + /// Verifies that when creating the replacement connection fails, the exception thrown by + /// the connection factory is propagated to the caller. + /// [Fact] public void ReplaceConnection_CreationFails_ExceptionPropagated() { @@ -196,6 +216,11 @@ public void ReplaceConnection_CreationFails_ExceptionPropagated() TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); } + /// + /// Verifies FR-006: when creating the replacement connection fails, the old connection's + /// slot is released (no capacity leak), the old connection is disposed, the pool is not + /// left in an error state, and the freed slot can be reused by a subsequent request. + /// [Fact] public void ReplaceConnection_CreationFails_OldSlotReleased() { @@ -248,6 +273,10 @@ public void ReplaceConnection_CreationFails_OldSlotReleased() #region Story 5 — Activation Failure Rollback + /// + /// Verifies that when activating the replacement connection fails, the exception is + /// propagated to the caller. + /// [Fact] public void ReplaceConnection_ActivationFails_ExceptionPropagated() { @@ -276,6 +305,10 @@ public void ReplaceConnection_ActivationFails_ExceptionPropagated() TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); } + /// + /// Verifies that when activating the replacement connection fails, the newly created + /// connection is returned to the pool rather than leaked, keeping the pool count stable. + /// [Fact] public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() { @@ -320,6 +353,10 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() #region Story 6 — Prefer Idle Connection + /// + /// Verifies that when an idle connection is available, replacement reuses that idle + /// connection instead of creating a new physical connection. + /// [Fact] public void ReplaceConnection_PrefersIdleOverNewConnection() { @@ -350,6 +387,10 @@ public void ReplaceConnection_PrefersIdleOverNewConnection() Assert.Equal(0, pool.IdleCount); } + /// + /// Verifies that when no idle connection is available, replacement creates a new + /// physical connection distinct from the one being replaced. + /// [Fact] public void ReplaceConnection_NoIdleConnection_CreatesNew() { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs index 28e59e7a5d..011533f773 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs @@ -483,5 +483,156 @@ public void Constructor_EdgeCase_CapacityOfOne_WorksCorrectly() Assert.Null(connection2); Assert.Equal(1, poolSlots.ReservationCount); } + + /// + /// Verifies that replacing an existing connection returns and leaves + /// the reservation count unchanged, since the replacement reuses the same slot. + /// + [Fact] + public void TryReplace_ExistingConnection_ReturnsTrueAndKeepsReservationCount() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + var reservationCountBeforeReplace = poolSlots.ReservationCount; + + // Act + var replaced = poolSlots.TryReplace(oldConnection!, newConnection); + + // Assert - the slot is reused, so the reservation count is unchanged + Assert.True(replaced); + Assert.Equal(1, reservationCountBeforeReplace); + Assert.Equal(1, poolSlots.ReservationCount); + } + + /// + /// Verifies that after a successful replace, the new connection occupies the slot (and can + /// be removed) while the old connection is no longer present in the collection. + /// + [Fact] + public void TryReplace_ExistingConnection_NewConnectionOccupiesSlot() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + + // Act + poolSlots.TryReplace(oldConnection!, newConnection); + + // Assert - the new connection now occupies the slot and can be removed, + // while the old connection is no longer present. + Assert.False(poolSlots.TryRemove(oldConnection!)); + Assert.True(poolSlots.TryRemove(newConnection)); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// Verifies that attempting to replace a connection that is not in the collection returns + /// , does not change the reservation count, and does not insert the + /// new connection. + /// + [Fact] + public void TryReplace_NonExistentConnection_ReturnsFalseAndDoesNotAddNewConnection() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var existingConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var missingConnection = new MockDbConnectionInternal(); + var newConnection = new MockDbConnectionInternal(); + var reservationCountBeforeReplace = poolSlots.ReservationCount; + + // Act + var replaced = poolSlots.TryReplace(missingConnection, newConnection); + + // Assert - nothing was replaced and the new connection was not inserted + Assert.False(replaced); + Assert.Equal(1, reservationCountBeforeReplace); + Assert.Equal(1, poolSlots.ReservationCount); + Assert.False(poolSlots.TryRemove(newConnection)); + } + + /// + /// Verifies that replacing a connection in an empty collection returns + /// and leaves the reservation count at zero. + /// + [Fact] + public void TryReplace_EmptyCollection_ReturnsFalse() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = new MockDbConnectionInternal(); + var newConnection = new MockDbConnectionInternal(); + + // Act + var replaced = poolSlots.TryReplace(oldConnection, newConnection); + + // Assert + Assert.False(replaced); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// Verifies that when multiple connections are present, replace swaps only the targeted + /// connection and leaves the others untouched. + /// + [Fact] + public void TryReplace_MultipleConnections_ReplacesOnlyTargetConnection() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var connection1 = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var connection2 = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + + // Act - replace only connection2 + var replaced = poolSlots.TryReplace(connection2!, newConnection); + + // Assert - the untouched connection remains, the target was swapped out + Assert.True(replaced); + Assert.Equal(2, poolSlots.ReservationCount); + Assert.True(poolSlots.TryRemove(connection1!)); + Assert.False(poolSlots.TryRemove(connection2!)); + Assert.True(poolSlots.TryRemove(newConnection)); + Assert.Equal(0, poolSlots.ReservationCount); + } + + /// + /// Verifies that replacing the same connection twice succeeds on the first attempt but + /// fails on the second, because the original connection is no longer in the slot. + /// + [Fact] + public void TryReplace_SameConnectionTwice_ReturnsFalseOnSecondAttempt() + { + // Arrange + var poolSlots = new ConnectionPoolSlots(5); + var oldConnection = poolSlots.Add( + createCallback: () => new MockDbConnectionInternal(), + cleanupCallback: (conn) => { }); + var newConnection = new MockDbConnectionInternal(); + var newerConnection = new MockDbConnectionInternal(); + + // Act + var firstReplace = poolSlots.TryReplace(oldConnection!, newConnection); + var secondReplace = poolSlots.TryReplace(oldConnection!, newerConnection); + + // Assert - the old connection is gone after the first replace, so the second fails + Assert.True(firstReplace); + Assert.False(secondReplace); + Assert.Equal(1, poolSlots.ReservationCount); + Assert.True(poolSlots.TryRemove(newConnection)); + Assert.False(poolSlots.TryRemove(newerConnection)); + } } } From d684191a4002e399000b9f5f8263972c9158540a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 7 Jul 2026 18:50:11 -0700 Subject: [PATCH 16/39] Wording --- .../Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 0d8c2e0c54..a959fab782 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 @@ -326,7 +326,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) DbConnectionInternal? idleConnection = GetIdleConnection(); if (idleConnection is not null) { - // The idle connection already holds its own slot, so just release the broken connection's + // The idle connection already holds its own slot, so release the broken connection's // slot to keep the pool count correct (FR-005 / Story 6). _connectionSlots.TryRemove(oldConnection); return idleConnection; From 23cfa38ec898b896d2cc3b65fe60729f5b569fa3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 9 Jul 2026 16:50:31 -0700 Subject: [PATCH 17/39] improve error handling --- .../ConnectionPool/ChannelDbConnectionPool.cs | 121 ++++++------------ ...elDbConnectionPoolReplaceConnectionTest.cs | 69 ++++------ 2 files changed, 62 insertions(+), 128 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index a959fab782..57e92b86ae 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 @@ -273,100 +273,59 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); - // Acquire a replacement that reuses the broken connection's pool slot so capacity is preserved - // (FR-005). Prefers an idle connection (Story 6); otherwise establishes a new one. Throws (after - // disposing the old connection and freeing its slot) if establishing a new connection fails (FR-006). - DbConnectionInternal? newConnection = GetReplacementConnection(owningObject, oldConnection, timeout); - if (newConnection is null) - { - // No slot could be secured for a new connection; let the caller's retry logic handle it. - return null; - } - - // Activate the replacement under the old connection's ambient transaction (FR-002/FR-003). If - // activation fails, PrepareConnection returns the new connection to the pool and rethrows (FR-007). - // TODO: Full transaction enlistment support (Story 2). - PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); - - // FR-004: Deactivate and dispose the old connection now that the replacement is active. - oldConnection.DeactivateConnection(); - oldConnection.Dispose(); + // We replace oldConnection with a brand-new physical connection. oldConnection is left completely + // untouched until the replacement has been successfully activated, so any failure leaves it reusable + // by the caller's reconnect retry loop (SqlConnection.ReconnectAsync). oldConnection is checked out, + // so its pool slot is stable: nothing else can remove or steal it (Clear/Shutdown/Prune only drain + // idle connections). We only touch the slots once the replacement is live. While the new connection + // is activating the pool may briefly hold one physical connection over MaxPoolSize (the dead old one + // plus the new one), but oldConnection is dead anyway and the reserved slot count never exceeds the + // maximum. + // TODO: Prefer reusing an idle connection before establishing a new one + DbConnectionInternal newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); - // FR-008: Record a soft connect metric since the caller's connection object is reused. - SqlClientDiagnostics.Metrics.SoftConnectRequest(); + try + { + // Stamp with the current generation so a later Clear() can discard it. + newConnection.ClearGeneration = _clearGeneration; - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, connection replaced successfully.", Id); + lock (newConnection) + { + // PostPop requires a lock on the connection. + newConnection.PostPop(owningObject); + } - return newConnection; - } + // Activate under the old connection's ambient transaction (FR-002/FR-003). + // TODO: Full transaction enlistment support (Story 2). + newConnection.ActivateConnection(oldConnection.EnlistedTransaction); - /// - /// Obtains a connection to replace , reusing its pool slot so the - /// pool's connection count is unchanged (FR-005). An idle connection is preferred (Story 6); - /// otherwise a new physical connection is established. - /// - /// The connection that will own the replacement. - /// The broken connection being replaced. - /// The overall timeout budget for establishing a new connection. - /// - /// The replacement connection, already placed in a pool slot, or if a new - /// connection was established but no slot could be secured for it. - /// - /// - /// Propagated when establishing a new connection fails. Before rethrowing, the old connection is - /// deactivated and removed from the pool so its slot is not leaked (FR-006 / Story 4). - /// - private DbConnectionInternal? GetReplacementConnection( - DbConnection owningObject, - DbConnectionInternal oldConnection, - TimeoutTimer timeout) - { - // Story 6: prefer an idle connection over establishing a new physical one. - DbConnectionInternal? idleConnection = GetIdleConnection(); - if (idleConnection is not null) - { - // The idle connection already holds its own slot, so release the broken connection's - // slot to keep the pool count correct (FR-005 / Story 6). - _connectionSlots.TryRemove(oldConnection); - return idleConnection; - } + bool replaced = _connectionSlots.TryReplace(oldConnection, newConnection); - DbConnectionInternal newConnection; - try - { - newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + if (!replaced) + { + // This should never happen because oldConnection is checked out and its slot is stable. + // Still, we need to check or we could vend a connection that is not tracked by the pool. + // TODO: error types and localization + throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced."); + } } catch { - // FR-006 / Story 4: Establishing the replacement failed (e.g., the server is still - // unreachable). Deactivate and remove the broken connection so its slot is not leaked, then - // propagate. RemoveConnection disposes it and clears its Pool reference, so the caller's - // eventual Close() follows the non-pooled path and will not touch the freed slot again. - oldConnection.DeactivateConnection(); - RemoveConnection(oldConnection); + // Dispose the brand-new connection; it never took a slot. oldConnection is untouched. + newConnection.DeactivateConnection(); + newConnection.Dispose(); throw; } - // Stamp the new connection with the current generation so a later Clear() can discard it. (Idle - // connections already carry their birth generation, so only newly established ones are stamped.) - newConnection.ClearGeneration = _clearGeneration; + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); - // Atomically swap the new connection into the broken connection's slot so the reservation count - // is unchanged (FR-005). - if (!_connectionSlots.TryReplace(oldConnection, newConnection)) - { - // The old slot was already removed (e.g., by a concurrent Clear). Reserve a fresh slot - // instead; if the pool is now full, give up and dispose the orphaned connection. - if (_connectionSlots.Add( - createCallback: () => newConnection, - cleanupCallback: (conn) => conn?.Dispose()) is null) - { - newConnection.Dispose(); - return null; - } - } + // A hard connect is already recorded by the connection factory. + SqlClientDiagnostics.Metrics.SoftConnectRequest(); + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, connection replaced successfully.", Id); + return newConnection; } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 3b66813339..0228d6f2db 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -217,14 +217,14 @@ public void ReplaceConnection_CreationFails_ExceptionPropagated() } /// - /// Verifies FR-006: when creating the replacement connection fails, the old connection's - /// slot is released (no capacity leak), the old connection is disposed, the pool is not - /// left in an error state, and the freed slot can be reused by a subsequent request. + /// Verifies that when creating the replacement connection fails, the old connection is left fully + /// intact - it keeps its pool slot and stays poolable - so the caller's reconnect retry loop can reuse + /// it on a subsequent attempt. The pool count is unchanged and the pool is not left in an error state. /// [Fact] - public void ReplaceConnection_CreationFails_OldSlotReleased() + public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() { - // Arrange — fill the pool to capacity so a leaked slot would be observable. + // Arrange — fill the pool to capacity so a leaked or prematurely released slot would be observable. var poolGroupOptions = new DbConnectionPoolGroupOptions( poolByIdentity: false, minPoolSize: 0, @@ -255,17 +255,23 @@ public void ReplaceConnection_CreationFails_OldSlotReleased() oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); - // Assert — FR-006: the old connection's slot was released (no capacity leak). - Assert.Equal(1, pool.Count); - // Old connection was disposed as part of the failure cleanup. - Assert.False(oldConnection!.CanBePooled); - // FR-006 scenario 3: the pool is not left in an error state. + // Assert — the old connection is left intact so the caller can retry with it: its slot is retained + // (no premature release) ... + Assert.Equal(2, pool.Count); + // ... it is not disposed and remains poolable ... + Assert.True(oldConnection!.CanBePooled); + // ... and the pool is not left in an error state. Assert.False(pool.ErrorOccurred); - // The freed slot is usable again — a fresh request can succeed. + // The reconnect retry loop reuses the SAME old connection: a subsequent successful replacement + // succeeds, reusing the retained slot and keeping the pool count unchanged. switchableFactory.ShouldFail = false; - pool.TryGetConnection(new SqlConnection(), null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? replacementForSlot); - Assert.NotNull(replacementForSlot); + var newConnection = pool.ReplaceConnection( + owner1, + oldConnection!, + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); + Assert.NotNull(newConnection); + Assert.NotSame(oldConnection, newConnection); Assert.Equal(2, pool.Count); } @@ -351,41 +357,10 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() #endregion - #region Story 6 — Prefer Idle Connection - - /// - /// Verifies that when an idle connection is available, replacement reuses that idle - /// connection instead of creating a new physical connection. - /// - [Fact] - public void ReplaceConnection_PrefersIdleOverNewConnection() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - SqlConnection owner1 = new(); - SqlConnection owner2 = new(); - - pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); - pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); - - Assert.NotNull(conn1); - Assert.NotNull(conn2); - - // Return conn2 to make it idle - pool.ReturnInternalConnection(conn2, owner2); - Assert.Equal(1, pool.IdleCount); + #region Story 6 — New Physical Connection - // Act — replace conn1; should prefer the idle conn2 - var newConnection = pool.ReplaceConnection( - owner1, - conn1, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); - - // Assert — should reuse the idle connection - Assert.NotNull(newConnection); - Assert.Same(conn2, newConnection); - Assert.Equal(0, pool.IdleCount); - } + // NOTE: Preferring an idle connection over a new physical one is being added in a follow-up PR + // (branch dev/automation/replace-conn-idle-path), along with its ReplaceConnection_PrefersIdleOverNewConnection test. /// /// Verifies that when no idle connection is available, replacement creates a new From ae7592593f2f19988c6defc346a4da0cda83d4d3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 9 Jul 2026 17:01:54 -0700 Subject: [PATCH 18/39] Address copilot comments. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 4 ++-- .../Data/SqlClient/ConnectionPool/IDbConnectionPool.cs | 2 +- .../ChannelDbConnectionPoolReplaceConnectionTest.cs | 10 ++++++---- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 57e92b86ae..b7787694c0 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 @@ -265,7 +265,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) } /// - public DbConnectionInternal? ReplaceConnection( + public DbConnectionInternal ReplaceConnection( DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout) @@ -325,7 +325,7 @@ public void PutObjectFromTransactedPool(DbConnectionInternal connection) SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, connection replaced successfully.", Id); - + return newConnection; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs index 25185b2cc3..3799cf54ea 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 @@ -133,7 +133,7 @@ internal interface IDbConnectionPool /// The internal connection currently associated with the owning object. /// The overall timeout budget for this connection request. /// A reference to the new DbConnectionInternal. - DbConnectionInternal? ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); + DbConnectionInternal ReplaceConnection(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout); /// /// Returns an internal connection to the pool. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 0228d6f2db..8ab4d12a9f 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -4,13 +4,11 @@ using System; using System.Data.Common; -using System.Threading; using System.Transactions; using Microsoft.Data.Common; using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; -using Microsoft.Data.SqlClient.Tests.Common; using Xunit; namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool @@ -258,8 +256,12 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() // Assert — the old connection is left intact so the caller can retry with it: its slot is retained // (no premature release) ... Assert.Equal(2, pool.Count); - // ... it is not disposed and remains poolable ... - Assert.True(oldConnection!.CanBePooled); + // ... it is not doomed, so it remains usable for the retry ... + Assert.False(oldConnection!.IsConnectionDoomed); + // ... it is still owned by the same caller (not released back to the pool) ... + Assert.Same(owner1, oldConnection!.Owner); + // ... it keeps its reference to the pool, which is what enables the caller's retry ... + Assert.Same(pool, oldConnection!.Pool); // ... and the pool is not left in an error state. Assert.False(pool.ErrorOccurred); From b2664021e2c0e753ec44edd3b514f70a73f6e67e Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 14:17:58 -0700 Subject: [PATCH 19/39] Fix malformed XML doc comment in TryOpenInner remarks A blank line inside the block was missing its '///' prefix, causing CS1570 and breaking the build. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index c6ac2b0658..c545e1d44d 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2261,7 +2261,7 @@ private bool TryOpen(TaskCompletionSource retry, bool forc /// /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is - + /// /// currently disconnected. If the connection is currently open, passing false will result in an exception. See SqlConnection state /// transitions and subclasses for more details. /// From 88aaffdc7be6505b3d532db07f79599798288144 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 14:18:06 -0700 Subject: [PATCH 20/39] Prefer reusing an idle connection in ChannelDbConnectionPool.ReplaceConnection ReplaceConnection now tries GetIdleConnection() before establishing a new physical connection. When a live idle connection is available it is checked out and activated under the old connection's ambient transaction, then the replaced connection's slot is freed and it is disposed. This avoids an unnecessary physical connect and keeps the reserved slot count strictly decreasing, so the pool never exceeds MaxPoolSize. When no idle connection is available the previous create-and-swap path is used unchanged. In both paths the old connection is left untouched until the replacement is activated, so a failure leaves it reusable by the caller's reconnect retry loop. Adds ReplaceConnection_PrefersIdleOverNewConnection and ReplaceConnection_IdleReuse_AtMaxCapacity_FreesOldSlot unit tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 115 ++++++++++++------ ...elDbConnectionPoolReplaceConnectionTest.cs | 90 +++++++++++++- 2 files changed, 166 insertions(+), 39 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index b7787694c0..67f9f94e6e 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 @@ -273,54 +273,97 @@ public DbConnectionInternal ReplaceConnection( SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); - // We replace oldConnection with a brand-new physical connection. oldConnection is left completely - // untouched until the replacement has been successfully activated, so any failure leaves it reusable - // by the caller's reconnect retry loop (SqlConnection.ReconnectAsync). oldConnection is checked out, - // so its pool slot is stable: nothing else can remove or steal it (Clear/Shutdown/Prune only drain - // idle connections). We only touch the slots once the replacement is live. While the new connection - // is activating the pool may briefly hold one physical connection over MaxPoolSize (the dead old one - // plus the new one), but oldConnection is dead anyway and the reserved slot count never exceeds the - // maximum. - // TODO: Prefer reusing an idle connection before establishing a new one - DbConnectionInternal newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + // Prefer reusing a live idle connection before paying for a brand-new physical connect. + // GetIdleConnection() is non-blocking: it returns an already-pooled, validated connection + // that holds its own slot, or null if none is immediately available. + DbConnectionInternal? newConnection = GetIdleConnection(); - try + if (newConnection is not null) { - // Stamp with the current generation so a later Clear() can discard it. - newConnection.ClearGeneration = _clearGeneration; + // Reuse path. The idle connection already occupies its own pool slot, so we do not + // reserve a new one. oldConnection keeps its own slot until the replacement is live, so + // any failure here leaves oldConnection untouched and reusable by the caller's reconnect + // retry loop (SqlConnection.ReconnectAsync). Once the replacement is activated we free + // oldConnection's slot, so the reserved slot count strictly decreases and the pool is + // never left over MaxPoolSize. + try + { + lock (newConnection) + { + // PostPop requires a lock on the connection. + newConnection.PostPop(owningObject); + } - lock (newConnection) + // Activate under the old connection's ambient transaction (FR-002/FR-003). + // TODO: Full transaction enlistment support (Story 2). + newConnection.ActivateConnection(oldConnection.EnlistedTransaction); + } + catch { - // PostPop requires a lock on the connection. - newConnection.PostPop(owningObject); + // The reused connection was checked out (PostPop) but could not be activated. + // Deactivate and remove it from the pool, freeing its slot and disposing it. + // oldConnection is untouched. + newConnection.DeactivateConnection(); + RemoveConnection(newConnection); + throw; } - // Activate under the old connection's ambient transaction (FR-002/FR-003). - // TODO: Full transaction enlistment support (Story 2). - newConnection.ActivateConnection(oldConnection.EnlistedTransaction); + // Replacement is live. Free oldConnection's slot and dispose it. + oldConnection.DeactivateConnection(); + RemoveConnection(oldConnection); + } + else + { + // Create path. No idle connection was available, so establish a brand-new physical + // connection. oldConnection is left completely untouched until the replacement has been + // successfully activated, so any failure leaves it reusable by the caller's reconnect + // retry loop (SqlConnection.ReconnectAsync). oldConnection is checked out, so its pool + // slot is stable: nothing else can remove or steal it (Clear/Shutdown/Prune only drain + // idle connections). We only touch the slots once the replacement is live. While the new + // connection is activating the pool may briefly hold one physical connection over + // MaxPoolSize (the dead old one plus the new one), but oldConnection is dead anyway and + // the reserved slot count never exceeds the maximum. + newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); - bool replaced = _connectionSlots.TryReplace(oldConnection, newConnection); + try + { + // Stamp with the current generation so a later Clear() can discard it. + newConnection.ClearGeneration = _clearGeneration; - if (!replaced) + lock (newConnection) + { + // PostPop requires a lock on the connection. + newConnection.PostPop(owningObject); + } + + // Activate under the old connection's ambient transaction (FR-002/FR-003). + // TODO: Full transaction enlistment support (Story 2). + newConnection.ActivateConnection(oldConnection.EnlistedTransaction); + + bool replaced = _connectionSlots.TryReplace(oldConnection, newConnection); + + if (!replaced) + { + // This should never happen because oldConnection is checked out and its slot is stable. + // Still, we need to check or we could vend a connection that is not tracked by the pool. + // TODO: error types and localization + throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced."); + } + } + catch { - // This should never happen because oldConnection is checked out and its slot is stable. - // Still, we need to check or we could vend a connection that is not tracked by the pool. - // TODO: error types and localization - throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced."); + // Dispose the brand-new connection; it never took a slot. oldConnection is untouched. + newConnection.DeactivateConnection(); + newConnection.Dispose(); + throw; } - } - catch - { - // Dispose the brand-new connection; it never took a slot. oldConnection is untouched. - newConnection.DeactivateConnection(); - newConnection.Dispose(); - throw; - } - oldConnection.DeactivateConnection(); - oldConnection.Dispose(); + oldConnection.DeactivateConnection(); + oldConnection.Dispose(); + } - // A hard connect is already recorded by the connection factory. + // We vended a connection from the pool. In the create path a hard connect was already + // recorded by the connection factory; the reuse path performs no physical connect. SqlClientDiagnostics.Metrics.SoftConnectRequest(); SqlClientEventSource.Log.TryPoolerTraceEvent( diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 8ab4d12a9f..351811af94 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -359,10 +359,94 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() #endregion - #region Story 6 — New Physical Connection + #region Story 6 — Prefer Idle Connection Reuse - // NOTE: Preferring an idle connection over a new physical one is being added in a follow-up PR - // (branch dev/automation/replace-conn-idle-path), along with its ReplaceConnection_PrefersIdleOverNewConnection test. + /// + /// Verifies that when 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(SuccessfulConnectionFactory); + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + Assert.NotNull(conn1); + Assert.NotNull(conn2); + + 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(SuccessfulConnectionFactory, poolGroupOptions); + + SqlConnection owner1 = new(); + SqlConnection owner2 = new(); + SqlConnection owner3 = new(); + + pool.TryGetConnection(owner1, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); + pool.TryGetConnection(owner2, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn2); + pool.TryGetConnection(owner3, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn3); + Assert.Equal(3, pool.Count); + + 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); + } + + #endregion + + #region Story 7 — New Physical Connection Fallback /// /// Verifies that when no idle connection is available, replacement creates a new From 19933e7cd65f697af57d6216876b1f345811d527 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 14:23:38 -0700 Subject: [PATCH 21/39] Use named forceNewConnection arguments at literal call sites Passing the boolean forceNewConnection flag positionally as a bare true/false obscures intent at the call site. Name the argument at every literal call site so the open/reconnect paths read clearly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index c545e1d44d..6c2a68e006 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,7 @@ public void Open(SqlConnectionOverrides overrides) { statistics = SqlStatistics.StartTimer(Statistics); - if (!(IsProviderRetriable ? TryOpenWithRetry(null, false, overrides) : TryOpen(null, false, overrides))) + if (!(IsProviderRetriable ? TryOpenWithRetry(null, forceNewConnection: false, overrides) : TryOpen(null, forceNewConnection: false, overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -1690,11 +1690,11 @@ private async Task ReconnectAsync(int timeout) { if (IsProviderRetriable) { - await InternalOpenWithRetryAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); + await InternalOpenWithRetryAsync(SqlConnectionOverrides.None, forceNewConnection: true, ctoken).ConfigureAwait(false); } else { - await InternalOpenAsync(SqlConnectionOverrides.None, true, ctoken).ConfigureAwait(false); + await InternalOpenAsync(SqlConnectionOverrides.None, forceNewConnection: true, ctoken).ConfigureAwait(false); } // On success, increment the reconnect count - we don't really care if it rolls over since it is approx. @@ -1917,8 +1917,8 @@ public override Task OpenAsync(CancellationToken cancellationToken) /// public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancellationToken) => IsProviderRetriable ? - InternalOpenWithRetryAsync(overrides, false, cancellationToken) : - InternalOpenAsync(overrides, false, cancellationToken); + InternalOpenWithRetryAsync(overrides, forceNewConnection: false, cancellationToken) : + InternalOpenAsync(overrides, forceNewConnection: false, cancellationToken); private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, forceNewConnection, cancellationToken), cancellationToken); From ea77190f330d26bf5ca4bbdbfc9e64b2de8a772f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 14:30:57 -0700 Subject: [PATCH 22/39] Return reused connection to the pool on activation failure via PrepareConnection The idle-reuse branch of ReplaceConnection previously deactivated and removed the reused connection if activation failed, unconditionally discarding a connection that was healthy moments earlier. Route the failure through ReturnInternalConnection instead so a still-healthy connection is re-pooled and only a genuinely dead one is removed, matching the normal get path. Since the reuse branch's check-out + activate + return-on-failure is now identical to PrepareConnection, call PrepareConnection directly to remove the duplication. Adds ReplaceConnection_IdleReuse_ActivationFails_ReturnedToPool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 38 +++++------------ ...elDbConnectionPoolReplaceConnectionTest.cs | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+), 28 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 67f9f94e6e..7347bf6b03 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 @@ -281,34 +281,16 @@ public DbConnectionInternal ReplaceConnection( if (newConnection is not null) { // Reuse path. The idle connection already occupies its own pool slot, so we do not - // reserve a new one. oldConnection keeps its own slot until the replacement is live, so - // any failure here leaves oldConnection untouched and reusable by the caller's reconnect - // retry loop (SqlConnection.ReconnectAsync). Once the replacement is activated we free - // oldConnection's slot, so the reserved slot count strictly decreases and the pool is - // never left over MaxPoolSize. - try - { - lock (newConnection) - { - // PostPop requires a lock on the connection. - newConnection.PostPop(owningObject); - } - - // Activate under the old connection's ambient transaction (FR-002/FR-003). - // TODO: Full transaction enlistment support (Story 2). - newConnection.ActivateConnection(oldConnection.EnlistedTransaction); - } - catch - { - // The reused connection was checked out (PostPop) but could not be activated. - // Deactivate and remove it from the pool, freeing its slot and disposing it. - // oldConnection is untouched. - newConnection.DeactivateConnection(); - RemoveConnection(newConnection); - throw; - } - - // Replacement is live. Free oldConnection's slot and dispose it. + // reserve a new one. PrepareConnection checks it out and activates it under the old + // connection's ambient transaction, exactly like the normal get path; on failure it + // returns the connection to the pool (re-pooling it when still healthy) and rethrows, + // leaving oldConnection untouched and reusable by the caller's reconnect retry loop + // (SqlConnection.ReconnectAsync). + // TODO: Full transaction enlistment support (Story 2). + PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); + + // Replacement is live. Free oldConnection's slot and dispose it. The reserved slot + // count strictly decreases, so the pool is never left over MaxPoolSize. oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 351811af94..388a37e7c7 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -444,6 +444,48 @@ public void ReplaceConnection_IdleReuse_AtMaxCapacity_FreesOldSlot() 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 ActivationFailSqlConnectionFactory(); + 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 From e573441e7da63776acf8eedd377c3bf183a9eb6f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 14:33:56 -0700 Subject: [PATCH 23/39] Condense block comments in ReplaceConnection Trim the large explanatory comments in ReplaceConnection so they no longer dominate the method, keeping the non-obvious rationale (slot accounting, reuse-on-failure, never over MaxPoolSize) in a couple of lines each. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 7347bf6b03..9750e88aad 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 @@ -273,38 +273,27 @@ public DbConnectionInternal ReplaceConnection( SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); - // Prefer reusing a live idle connection before paying for a brand-new physical connect. - // GetIdleConnection() is non-blocking: it returns an already-pooled, validated connection - // that holds its own slot, or null if none is immediately available. + // Prefer reusing a live idle connection (non-blocking) before opening a new physical one. DbConnectionInternal? newConnection = GetIdleConnection(); if (newConnection is not null) { - // Reuse path. The idle connection already occupies its own pool slot, so we do not - // reserve a new one. PrepareConnection checks it out and activates it under the old - // connection's ambient transaction, exactly like the normal get path; on failure it - // returns the connection to the pool (re-pooling it when still healthy) and rethrows, - // leaving oldConnection untouched and reusable by the caller's reconnect retry loop - // (SqlConnection.ReconnectAsync). + // Reuse path: the idle connection keeps its own slot. PrepareConnection checks it out + // and activates it like the normal get path, returning it to the pool if activation + // throws and leaving oldConnection reusable. // TODO: Full transaction enlistment support (Story 2). PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); - // Replacement is live. Free oldConnection's slot and dispose it. The reserved slot - // count strictly decreases, so the pool is never left over MaxPoolSize. + // Replacement is live; free oldConnection's slot. Net slots drop, never over max. oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); } else { - // Create path. No idle connection was available, so establish a brand-new physical - // connection. oldConnection is left completely untouched until the replacement has been - // successfully activated, so any failure leaves it reusable by the caller's reconnect - // retry loop (SqlConnection.ReconnectAsync). oldConnection is checked out, so its pool - // slot is stable: nothing else can remove or steal it (Clear/Shutdown/Prune only drain - // idle connections). We only touch the slots once the replacement is live. While the new - // connection is activating the pool may briefly hold one physical connection over - // MaxPoolSize (the dead old one plus the new one), but oldConnection is dead anyway and - // the reserved slot count never exceeds the maximum. + // Create path: no idle connection available, so open a new physical one. oldConnection + // stays checked out (its slot is stable) and untouched until the replacement is live, + // so a failure leaves it reusable. TryReplace later swaps the new connection into + // oldConnection's slot, so the reserved slot count never exceeds MaxPoolSize. newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); try @@ -318,7 +307,7 @@ public DbConnectionInternal ReplaceConnection( newConnection.PostPop(owningObject); } - // Activate under the old connection's ambient transaction (FR-002/FR-003). + // Activate under the old connection's ambient transaction. // TODO: Full transaction enlistment support (Story 2). newConnection.ActivateConnection(oldConnection.EnlistedTransaction); @@ -326,8 +315,8 @@ public DbConnectionInternal ReplaceConnection( if (!replaced) { - // This should never happen because oldConnection is checked out and its slot is stable. - // Still, we need to check or we could vend a connection that is not tracked by the pool. + // Should never happen (oldConnection is checked out, so its slot is stable), + // but guard against vending a connection the pool isn't tracking. // TODO: error types and localization throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced."); } @@ -344,8 +333,7 @@ public DbConnectionInternal ReplaceConnection( oldConnection.Dispose(); } - // We vended a connection from the pool. In the create path a hard connect was already - // recorded by the connection factory; the reuse path performs no physical connect. + // Vended a connection from the pool (the create path already recorded a hard connect). SqlClientDiagnostics.Metrics.SoftConnectRequest(); SqlClientEventSource.Log.TryPoolerTraceEvent( From 594231c08c857fd2eb646dfdfd31bfea3e344ede Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 15:41:15 -0700 Subject: [PATCH 24/39] Document ReplaceConnection design rationale in one header block Consolidate the scattered per-branch rationale in ReplaceConnection into a single header comment explaining the two invariants that shape the method (forward progress under pool saturation via atomic reservation handoff, and oldConnection as the failure anchor) and why the create branch cannot delegate to PrepareConnection. Slim the inline branch comments to short pointers so the control flow reads cleanly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9750e88aad..45cea11edf 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 @@ -273,27 +273,51 @@ public DbConnectionInternal ReplaceConnection( SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); + // Swaps the physical connection backing an already-open SqlConnection during + // reconnection (idle connection resiliency); the outer SqlConnection stays open + // throughout. Two invariants drive the design and keep the two branches from + // collapsing into a single PrepareConnection call: + // + // 1. Progress under saturation. This runs synchronously and never waits for a slot. + // It reuses oldConnection's existing reservation, handing it to the replacement + // via an atomic TryReplace (the reservation count is unchanged), so a reconnect + // always gets its replacement even when the pool is full (MaxPoolSize-1 checked + // out and busy). Releasing old's slot and re-reserving could let another thread + // steal the freed slot and stall the reconnect until timeout. + // + // 2. oldConnection is the failure anchor. On any failure it must stay slotted and + // usable, because the outer reconnect loop retries against it; it is retired only + // after the replacement is fully activated. + // + // Reuse branch (an idle connection is available): it already owns a slot, so it goes + // through PrepareConnection like a normal checkout -- if activation throws, + // PrepareConnection safely returns it to the pool. We then free old's slot, so the + // net slot count drops (never over max). + // + // Create branch (no idle connection): we open a new connection directly, bypassing + // the reserve-a-slot path, and leave it UNSLOTTED until TryReplace swaps it into old's + // slot *after* activation succeeds. This is why the branch cannot delegate to + // PrepareConnection: PrepareConnection returns a failed connection to the idle channel, + // but one that never took a slot would be published untracked -- letting another caller + // vend a connection the pool isn't counting (over max / accounting skew). Staying + // unslotted keeps failure trivial: dispose only the new connection and leave old intact. + // Prefer reusing a live idle connection (non-blocking) before opening a new physical one. DbConnectionInternal? newConnection = GetIdleConnection(); if (newConnection is not null) { - // Reuse path: the idle connection keeps its own slot. PrepareConnection checks it out - // and activates it like the normal get path, returning it to the pool if activation - // throws and leaving oldConnection reusable. + // Reuse branch (see header): newConnection already owns its slot. // TODO: Full transaction enlistment support (Story 2). PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); - // Replacement is live; free oldConnection's slot. Net slots drop, never over max. + // Replacement is live; retire oldConnection's slot. oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); } else { - // Create path: no idle connection available, so open a new physical one. oldConnection - // stays checked out (its slot is stable) and untouched until the replacement is live, - // so a failure leaves it reusable. TryReplace later swaps the new connection into - // oldConnection's slot, so the reserved slot count never exceeds MaxPoolSize. + // Create branch (see header): open unslotted, swap into old's slot on success. newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); try From 69b08b5f696f077ffa55586321b951c38acab010 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 16:38:15 -0700 Subject: [PATCH 25/39] Restore named forceNewConnection arguments at test call sites Revert the named-to-positional reversions that crept into the TryOpenInner call sites in SqlConnectionConcurrentOpenTests and SqlConnectionStateTransitionTests, restoring the readable forceNewConnection: false/true form to match the rest of the branch and the call sites on main. Also restore the accidental missing space after the comma in the TryOpenWithRetry parameter list. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/SqlClient/SqlConnection.cs | 2 +- .../Data/SqlClient/SqlConnectionConcurrentOpenTests.cs | 2 +- .../Data/SqlClient/SqlConnectionStateTransitionTests.cs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 6c2a68e006..9027d1a550 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -1595,7 +1595,7 @@ public override void EnlistTransaction(System.Transactions.Transaction transacti public override void Open() => Open(SqlConnectionOverrides.None); - private bool TryOpenWithRetry(TaskCompletionSource retry, bool forceNewConnection,SqlConnectionOverrides overrides) + private bool TryOpenWithRetry(TaskCompletionSource retry, bool forceNewConnection, SqlConnectionOverrides overrides) => RetryLogicProvider.Execute(this, () => TryOpen(retry, forceNewConnection, overrides)); /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs index 0ad965913e..17ac619a3e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionConcurrentOpenTests.cs @@ -86,7 +86,7 @@ public void TryOpenInner_WhenInnerConnectionRacesToNonSqlConnectionInternalState Exception ex = Assert.ThrowsAny(() => { - connection.TryOpenInner(completedRetry, false); + connection.TryOpenInner(completedRetry, forceNewConnection: false); }); Assert.True( diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs index bd47826de5..2955a6825e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConnectionStateTransitionTests.cs @@ -27,7 +27,7 @@ public void TryOpenInner_WhenInnerConnectionChangesAfterTryOpen_ThrowsInvalidOpe bool initialized = connection.SetInnerConnectionFrom(new TestRaceDbConnectionInternal(), DbConnectionClosedNeverOpened.SingletonInstance); Assert.True(initialized); - InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, false)); + InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, forceNewConnection: false)); Assert.NotNull(exception); Assert.Same(DbConnectionClosedPreviouslyOpened.SingletonInstance, connection.InnerConnection); } @@ -42,7 +42,7 @@ public void TryOpenInner_WhenInnerConnectionChangesAfterTryReplace_ThrowsInvalid bool initialized = connection.SetInnerConnectionFrom(new TestRaceDbConnectionInternal(), DbConnectionClosedNeverOpened.SingletonInstance); Assert.True(initialized); - InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, true)); + InvalidOperationException exception = Assert.Throws(() => connection.TryOpenInner(null, forceNewConnection: true)); Assert.NotNull(exception); Assert.Same(DbConnectionClosedPreviouslyOpened.SingletonInstance, connection.InnerConnection); } From fbf37d73b4061fd6aa0eb483e89cc78b702022de Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 16:45:42 -0700 Subject: [PATCH 26/39] clean up comments --- .../ConnectionPool/ChannelDbConnectionPool.cs | 35 +++++-------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 45cea11edf..60b7aebd29 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 @@ -273,56 +273,42 @@ public DbConnectionInternal ReplaceConnection( SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); - // Swaps the physical connection backing an already-open SqlConnection during - // reconnection (idle connection resiliency); the outer SqlConnection stays open - // throughout. Two invariants drive the design and keep the two branches from - // collapsing into a single PrepareConnection call: + // Two invariants drive the design and keep the two branches from + // collapsing into a single shared path: // - // 1. Progress under saturation. This runs synchronously and never waits for a slot. - // It reuses oldConnection's existing reservation, handing it to the replacement - // via an atomic TryReplace (the reservation count is unchanged), so a reconnect - // always gets its replacement even when the pool is full (MaxPoolSize-1 checked - // out and busy). Releasing old's slot and re-reserving could let another thread + // 1. Progress under saturation. Releasing old's slot and re-reserving could let another thread // steal the freed slot and stall the reconnect until timeout. // - // 2. oldConnection is the failure anchor. On any failure it must stay slotted and - // usable, because the outer reconnect loop retries against it; it is retired only - // after the replacement is fully activated. + // 2. the outer reconnect loop retries assuming oldConnection is not disposed; it may be retired only + // after the replacement is fully activated and we know we won't fail. // - // Reuse branch (an idle connection is available): it already owns a slot, so it goes + // Reuse branch: the idle connection already owns a slot, so it goes // through PrepareConnection like a normal checkout -- if activation throws, // PrepareConnection safely returns it to the pool. We then free old's slot, so the - // net slot count drops (never over max). + // net slot count drops. // - // Create branch (no idle connection): we open a new connection directly, bypassing + // Create branch: we open a new connection directly, bypassing // the reserve-a-slot path, and leave it UNSLOTTED until TryReplace swaps it into old's - // slot *after* activation succeeds. This is why the branch cannot delegate to - // PrepareConnection: PrepareConnection returns a failed connection to the idle channel, + // slot *after* activation succeeds. PrepareConnection returns a failed connection to the idle channel, // but one that never took a slot would be published untracked -- letting another caller // vend a connection the pool isn't counting (over max / accounting skew). Staying // unslotted keeps failure trivial: dispose only the new connection and leave old intact. - // Prefer reusing a live idle connection (non-blocking) before opening a new physical one. DbConnectionInternal? newConnection = GetIdleConnection(); if (newConnection is not null) { - // Reuse branch (see header): newConnection already owns its slot. // TODO: Full transaction enlistment support (Story 2). PrepareConnection(owningObject, newConnection, oldConnection.EnlistedTransaction); - - // Replacement is live; retire oldConnection's slot. oldConnection.DeactivateConnection(); RemoveConnection(oldConnection); } else { - // Create branch (see header): open unslotted, swap into old's slot on success. newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); try { - // Stamp with the current generation so a later Clear() can discard it. newConnection.ClearGeneration = _clearGeneration; lock (newConnection) @@ -331,7 +317,6 @@ public DbConnectionInternal ReplaceConnection( newConnection.PostPop(owningObject); } - // Activate under the old connection's ambient transaction. // TODO: Full transaction enlistment support (Story 2). newConnection.ActivateConnection(oldConnection.EnlistedTransaction); @@ -347,7 +332,6 @@ public DbConnectionInternal ReplaceConnection( } catch { - // Dispose the brand-new connection; it never took a slot. oldConnection is untouched. newConnection.DeactivateConnection(); newConnection.Dispose(); throw; @@ -357,7 +341,6 @@ public DbConnectionInternal ReplaceConnection( oldConnection.Dispose(); } - // Vended a connection from the pool (the create path already recorded a hard connect). SqlClientDiagnostics.Metrics.SoftConnectRequest(); SqlClientEventSource.Log.TryPoolerTraceEvent( From f430f806690a5e087e2228145c04091fdf3bcf79 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 15 Jul 2026 17:22:23 -0700 Subject: [PATCH 27/39] Address Copilot review: fix doc comments for ReplaceConnection - Remove stray blank doc line that split the forceNewConnection sentence into two paragraphs in generated docs. - Correct the TestReplaceConnection summary (it no longer asserts NotImplementedException) and move it out of the 'Not Implemented Method Tests' region into a dedicated 'Replace Connection Tests' region. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 1 - .../ChannelDbConnectionPoolTest.cs | 36 ++++++++++--------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 9027d1a550..57c372122e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2261,7 +2261,6 @@ private bool TryOpen(TaskCompletionSource retry, bool forc /// /// forceNewConnection may only be true when the connection is already open and needs to be replaced. If the connection has never /// been opened, passing true will result in an exception. It may only be false when the connection has never been opened or is - /// /// currently disconnected. If the connection is currently open, passing false will result in an exception. See SqlConnection state /// transitions and subclasses for more details. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 0f5d802750..34f6236a6d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -855,25 +855,11 @@ 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() @@ -895,6 +881,24 @@ public void TestReplaceConnection() Assert.NotSame(oldConnection, newConnection); } + #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!)); + } + /// /// Verifies that /// remains unimplemented and throws . From 81f695822d2a3f6018a007ca3ab7b2c7a75d4510 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 11:00:46 -0700 Subject: [PATCH 28/39] Address Copilot review: test summary + explicit Assert.Throws - Add a class-level XML summary to ChannelDbConnectionPoolReplaceConnectionTest describing the behavior under test. - Replace the try/catch that swallowed the expected InvalidOperationException in ReplaceConnection_ActivationFails_NewConnectionReturnedToPool with an explicit Assert.Throws, matching the sibling failure-path tests and making the intent fail-safe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...hannelDbConnectionPoolReplaceConnectionTest.cs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 388a37e7c7..04e726c8ed 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -13,6 +13,11 @@ 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 { private static readonly SqlConnectionFactory SuccessfulConnectionFactory = new SuccessfulSqlConnectionFactory(); @@ -339,17 +344,11 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() factory.FailOnActivate = true; // Act - try - { + Assert.Throws(() => pool.ReplaceConnection( owner, oldConnection, - TimeoutTimer.StartNew(TimeSpan.FromSeconds(15))); - } - catch (InvalidOperationException) - { - // Expected - } + TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); // Assert — the new connection was returned to pool (not leaked). // Pool count stays same because the new connection replaced the old one's slot From a0659c63c6aa0b6701f8e4bae7abc14ee061df99 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 11:54:04 -0700 Subject: [PATCH 29/39] Respect blocking period in ReplaceConnection new-physical-connection path The create branch of ReplaceConnection now honors the pool's blocking-period error state (ThrowIfActive) before opening a new physical connection and clears the backoff ramp on a successful open, mirroring OpenNewInternalConnection. Idle reuse stays exempt, and a reconnect failure still does not enter the error state by design, so a targeted reconnect cannot poison the pool. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 11 +++++ ...elDbConnectionPoolReplaceConnectionTest.cs | 41 +++++++++++++++++++ 2 files changed, 52 insertions(+) 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 60b7aebd29..384c4967ae 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 @@ -305,6 +305,12 @@ public DbConnectionInternal ReplaceConnection( } else { + // Respect the pool's blocking period: a replacement in this branch opens a brand-new + // physical connection, so honor the same backoff the normal create path does + // (OpenNewInternalConnection) and fast-fail instead of hammering an unhealthy server. + // Idle reuse above is deliberately exempt, matching the normal acquire path. + _errorState?.ThrowIfActive(); + newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); try @@ -337,6 +343,11 @@ public DbConnectionInternal ReplaceConnection( throw; } + // A successful physical open proves the server is reachable, so clear any + // blocking-period backoff (resetting its ramp) exactly as OpenNewInternalConnection + // does, letting other callers stop fast-failing sooner. + _errorState?.Clear(); + oldConnection.DeactivateConnection(); oldConnection.Dispose(); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 04e726c8ed..3a62863684 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -518,6 +518,47 @@ public void ReplaceConnection_NoIdleConnection_CreatesNew() #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 SwitchableSqlConnectionFactory(); + var pool = ConstructPool(factory); + SqlConnection owner = new(); + + // Check out a connection to later replace (creation succeeds). + factory.ShouldFail = 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.ShouldFail = true; + 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. + Assert.Throws(() => + pool.ReplaceConnection(owner, oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); + + // 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); + } + + #endregion + #region Test Helper Classes internal class SuccessfulSqlConnectionFactory : SqlConnectionFactory From 21c99a094e4d44eebc5b00d8f49557c56460e00f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 11:57:23 -0700 Subject: [PATCH 30/39] Condense blocking-period comments in ReplaceConnection Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 384c4967ae..86eaf609b8 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 @@ -305,10 +305,8 @@ public DbConnectionInternal ReplaceConnection( } else { - // Respect the pool's blocking period: a replacement in this branch opens a brand-new - // physical connection, so honor the same backoff the normal create path does - // (OpenNewInternalConnection) and fast-fail instead of hammering an unhealthy server. - // Idle reuse above is deliberately exempt, matching the normal acquire path. + // Honor the blocking period before opening a new connection, mirroring + // OpenNewInternalConnection. Idle reuse above is intentionally exempt. _errorState?.ThrowIfActive(); newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); @@ -343,9 +341,7 @@ public DbConnectionInternal ReplaceConnection( throw; } - // A successful physical open proves the server is reachable, so clear any - // blocking-period backoff (resetting its ramp) exactly as OpenNewInternalConnection - // does, letting other callers stop fast-failing sooner. + // A successful open clears the blocking period, mirroring OpenNewInternalConnection. _errorState?.Clear(); oldConnection.DeactivateConnection(); From 5ca5fc865279210b23ad724a571330615d23cbbb Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 27 Jul 2026 12:15:05 -0700 Subject: [PATCH 31/39] Address Copilot review feedback on ReplaceConnection - Localize the "slot could not be replaced" guard in ChannelDbConnectionPool.ReplaceConnection: replace the hard-coded InvalidOperationException with ADP.InternalError using a new InternalErrorCode.ConnectionSlotReplacementFailed (still an InvalidOperationException, so behavior is unchanged). - Correct the TryOpenInner forceNewConnection XML remarks: the flag is also valid when the connection was previously opened and is now disconnected (the reconnect path via DbConnectionClosedPreviouslyOpened / DbConnectionClosedConnecting), not only when already open. Also removes a stray blank doc line by using blocks. - Fix the activation-failure test so its name, summary, and inline comment match the implementation: the new connection is disposed (never slotted) and the old connection is left intact, so pool count is unchanged. - Remove unused usings (Microsoft.Data.Common, Microsoft.Data.Common.ConnectionString) from the ReplaceConnection tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/Common/AdapterUtil.cs | 1 + .../ConnectionPool/ChannelDbConnectionPool.cs | 3 +-- .../Microsoft/Data/SqlClient/SqlConnection.cs | 18 ++++++++++++------ ...nelDbConnectionPoolReplaceConnectionTest.cs | 13 ++++++------- 4 files changed, 20 insertions(+), 15 deletions(-) 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..154d6a3333 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs @@ -1096,6 +1096,7 @@ internal enum InternalErrorCode UnexpectedWaitAnyResult = 16, SynchronousConnectReturnedPending = 17, CompletedConnectReturnedPending = 18, + ConnectionSlotReplacementFailed = 19, NameValuePairNext = 20, InvalidParserState1 = 21, 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 7c07debadd..c7c41d82c9 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 @@ -379,8 +379,7 @@ public DbConnectionInternal ReplaceConnection( { // Should never happen (oldConnection is checked out, so its slot is stable), // but guard against vending a connection the pool isn't tracking. - // TODO: error types and localization - throw new InvalidOperationException("Connection is no longer in the pool and cannot be replaced."); + throw ADP.InternalError(ADP.InternalErrorCode.ConnectionSlotReplacementFailed); } } catch 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 57c372122e..63c446b02f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2253,16 +2253,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 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/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 3a62863684..130add1ab6 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -5,8 +5,6 @@ using System; using System.Data.Common; using System.Transactions; -using Microsoft.Data.Common; -using Microsoft.Data.Common.ConnectionString; using Microsoft.Data.ProviderBase; using Microsoft.Data.SqlClient.ConnectionPool; using Xunit; @@ -320,10 +318,11 @@ public void ReplaceConnection_ActivationFails_ExceptionPropagated() /// /// Verifies that when activating the replacement connection fails, the newly created - /// connection is returned to the pool rather than leaked, keeping the pool count stable. + /// 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_NewConnectionReturnedToPool() + public void ReplaceConnection_ActivationFails_NewConnectionDisposed_PoolCountStable() { // Arrange var factory = new ActivationFailSqlConnectionFactory(); @@ -350,9 +349,9 @@ public void ReplaceConnection_ActivationFails_NewConnectionReturnedToPool() oldConnection, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); - // Assert — the new connection was returned to pool (not leaked). - // Pool count stays same because the new connection replaced the old one's slot - // and was then returned to idle. + // Assert — 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); } From ef4754ffd284b720048f6f5879d0b3d2c373b116 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 28 Jul 2026 08:17:54 -0700 Subject: [PATCH 32/39] Throw localized message when pool connection replacement fails Replace the opaque ADP.InternalError(ConnectionSlotReplacementFailed) at the ReplaceConnection !replaced guard with a localized InvalidOperationException (SQL_ConnectionPoolReplaceConnectionFailed). Removes the now-unused InternalErrorCode.ConnectionSlotReplacementFailed enum value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/Microsoft/Data/Common/AdapterUtil.cs | 1 - .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 2 +- .../src/Resources/Strings.Designer.cs | 9 +++++++++ src/Microsoft.Data.SqlClient/src/Resources/Strings.resx | 3 +++ 4 files changed, 13 insertions(+), 2 deletions(-) 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 154d6a3333..22aa0360ec 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs @@ -1096,7 +1096,6 @@ internal enum InternalErrorCode UnexpectedWaitAnyResult = 16, SynchronousConnectReturnedPending = 17, CompletedConnectReturnedPending = 18, - ConnectionSlotReplacementFailed = 19, NameValuePairNext = 20, InvalidParserState1 = 21, 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 c7c41d82c9..ebb59e1158 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 @@ -379,7 +379,7 @@ public DbConnectionInternal ReplaceConnection( { // Should never happen (oldConnection is checked out, so its slot is stable), // but guard against vending a connection the pool isn't tracking. - throw ADP.InternalError(ADP.InternalErrorCode.ConnectionSlotReplacementFailed); + throw new InvalidOperationException(StringsHelper.GetString(Strings.SQL_ConnectionPoolReplaceConnectionFailed)); } } catch 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. From c50865c06934e7b8df91949239dbe81eb7338f8e Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 28 Jul 2026 08:22:02 -0700 Subject: [PATCH 33/39] Explain why replacement bypasses the connection-creation rate limiter Document that the ReplaceConnection create branch intentionally skips _connectionCreationRateLimiter: a replacement is a 1-for-1 swap (not pool growth) and must make forward progress for an already checked-out caller's reconnect, so the limiter's fast-fail-then-wait-for-idle contract does not apply. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 8 ++++++++ 1 file changed, 8 insertions(+) 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 ebb59e1158..402264d89d 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 @@ -358,6 +358,14 @@ public DbConnectionInternal ReplaceConnection( // OpenNewInternalConnection. Idle reuse above is intentionally exempt. _errorState?.ThrowIfActive(); + // Unlike OpenNewInternalConnection, this direct create intentionally bypasses + // _connectionCreationRateLimiter. That limiter paces bursts of pool-growth opens, + // relying on a fast-fail-then-wait-for-idle fallback when a permit isn't available. + // A replacement fits neither assumption: it is a 1-for-1 swap (oldConnection is + // disposed once the new one activates below) so it does not grow the pool, and it + // must produce a fresh connection so an already checked-out caller's reconnect can + // make forward progress -- the reuse branch above already found no idle connection + // to satisfy it. newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); try From 36c309b56c4e7f93e46623672900dddde6356f97 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 28 Jul 2026 08:35:05 -0700 Subject: [PATCH 34/39] Inject FakeTimeProvider in new pool tests to prevent background races The new ReplaceConnection tests built pools on TimeProvider.System, letting time-driven background maintenance (idle-timeout pruning, warmup/replenishment, blocking-period expiry) advance in real time and potentially race the assertions. Thread a frozen FakeTimeProvider through the replacement test helper (default) and the TestReplaceConnection case so the pool clock only moves when a test drives it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...ChannelDbConnectionPoolReplaceConnectionTest.cs | 14 ++++++++++++-- .../ConnectionPool/ChannelDbConnectionPoolTest.cs | 3 ++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 130add1ab6..7e7b57335d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -7,6 +7,7 @@ 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 @@ -20,9 +21,17 @@ public class ChannelDbConnectionPoolReplaceConnectionTest { private static readonly SqlConnectionFactory SuccessfulConnectionFactory = new SuccessfulSqlConnectionFactory(); + /// + /// 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) + DbConnectionPoolGroupOptions? poolGroupOptions = null, + TimeProvider? timeProvider = null) { poolGroupOptions ??= new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -42,7 +51,8 @@ private ChannelDbConnectionPool ConstructPool( connectionFactory, dbConnectionPoolGroup, DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo() + new DbConnectionPoolProviderInfo(), + timeProvider: timeProvider ?? new FakeTimeProvider() ); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index cee5c23560..604964927e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -868,7 +868,8 @@ public void TestUseLoadBalancing() public void TestReplaceConnection() { // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); + var fakeTime = new FakeTimeProvider(); + var pool = ConstructPool(SuccessfulConnectionFactory, timeProvider: fakeTime); SqlConnection owner = new(); pool.TryGetConnection( From 8736037aef2d428f91564714041770000fbd242a Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 28 Jul 2026 16:50:29 -0700 Subject: [PATCH 35/39] Enter blocking-period error state on replacement open failure Mirror WaitHandleDbConnectionPool: when the physical open of a replacement connection fails, enter the blocking-period error state so subsequent opens fast-fail until it expires. Activation failures are excluded (the server proved reachable), matching the WaitHandle pool where PrepareConnection runs outside CreateObject's error-state catch. Adds two tests and updates the creation-failure retry test to reflect that the failed open now enters the blocking period. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 56 ++++++------- ...elDbConnectionPoolReplaceConnectionTest.cs | 83 +++++++++++++++++-- 2 files changed, 102 insertions(+), 37 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 9cecf5e19a..b2d4427b47 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 @@ -325,27 +325,8 @@ public DbConnectionInternal ReplaceConnection( SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, replacing connection.", Id); - // Two invariants drive the design and keep the two branches from - // collapsing into a single shared path: - // - // 1. Progress under saturation. Releasing old's slot and re-reserving could let another thread - // steal the freed slot and stall the reconnect until timeout. - // - // 2. the outer reconnect loop retries assuming oldConnection is not disposed; it may be retired only - // after the replacement is fully activated and we know we won't fail. - // - // Reuse branch: the idle connection already owns a slot, so it goes - // through PrepareConnection like a normal checkout -- if activation throws, - // PrepareConnection safely returns it to the pool. We then free old's slot, so the - // net slot count drops. - // - // Create branch: we open a new connection directly, bypassing - // the reserve-a-slot path, and leave it UNSLOTTED until TryReplace swaps it into old's - // slot *after* activation succeeds. PrepareConnection returns a failed connection to the idle channel, - // but one that never took a slot would be published untracked -- letting another caller - // vend a connection the pool isn't counting (over max / accounting skew). Staying - // unslotted keeps failure trivial: dispose only the new connection and leave old intact. - + // 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) @@ -357,19 +338,30 @@ public DbConnectionInternal ReplaceConnection( } else { - // Honor the blocking period before opening a new connection, mirroring - // OpenNewInternalConnection. Idle reuse above is intentionally exempt. _errorState?.ThrowIfActive(); // Unlike OpenNewInternalConnection, this direct create intentionally bypasses - // _connectionCreationRateLimiter. That limiter paces bursts of pool-growth opens, - // relying on a fast-fail-then-wait-for-idle fallback when a permit isn't available. - // A replacement fits neither assumption: it is a 1-for-1 swap (oldConnection is - // disposed once the new one activates below) so it does not grow the pool, and it - // must produce a fresh connection so an already checked-out caller's reconnect can - // make forward progress -- the reuse branch above already found no idle connection - // to satisfy it. - newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + // _connectionCreationRateLimiter. This mirrors the behavior in WaitHandleDbConnectionPool.ReplaceConnection. + try + { + newConnection = ConnectionFactory.CreatePooledConnection(owningObject, this, timeout); + } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex) && ex is not OperationCanceledException) + { + // A failed physical open means the server is unreachable, so enter the blocking + // period exactly as OpenNewInternalConnection and WaitHandleDbConnectionPool.CreateObject + // do: subsequent opens fast-fail until the period expires. Activation failures in the + // try below are intentionally excluded -- the server proved reachable -- matching the + // WaitHandle pool, where PrepareConnection runs outside CreateObject's error-state catch. + // We exclude OperationCanceledException (caller-side timeout/cancellation, not a physical + // failure) and only enter while Running, mirroring OpenNewInternalConnection. + if (State == Running) + { + _errorState?.Enter(ex); + } + + throw; + } try { @@ -384,6 +376,7 @@ public DbConnectionInternal ReplaceConnection( // TODO: Full transaction enlistment support (Story 2). newConnection.ActivateConnection(oldConnection.EnlistedTransaction); + // Place new into old's slot bool replaced = _connectionSlots.TryReplace(oldConnection, newConnection); if (!replaced) @@ -403,6 +396,7 @@ public DbConnectionInternal ReplaceConnection( // A successful open clears the blocking period, mirroring OpenNewInternalConnection. _errorState?.Clear(); + // Only retire the old connection after the replacement is fully activated and we know we won't fail. oldConnection.DeactivateConnection(); oldConnection.Dispose(); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 7e7b57335d..56d1a200a7 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -230,7 +230,9 @@ public void ReplaceConnection_CreationFails_ExceptionPropagated() /// /// Verifies that when creating the replacement connection fails, the old connection is left fully /// intact - it keeps its pool slot and stays poolable - so the caller's reconnect retry loop can reuse - /// it on a subsequent attempt. The pool count is unchanged and the pool is not left in an error state. + /// 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() @@ -246,7 +248,8 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() idleTimeout: 0 ); var switchableFactory = new SwitchableSqlConnectionFactory(); - var pool = ConstructPool(switchableFactory, poolGroupOptions); + var fakeTime = new FakeTimeProvider(); + var pool = ConstructPool(switchableFactory, poolGroupOptions, timeProvider: fakeTime); SqlConnection owner1 = new(); SqlConnection owner2 = new(); @@ -275,12 +278,16 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() Assert.Same(owner1, oldConnection!.Owner); // ... it keeps its reference to the pool, which is what enables the caller's retry ... Assert.Same(pool, oldConnection!.Pool); - // ... and the pool is not left in an error state. - Assert.False(pool.ErrorOccurred); + // ... 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: a subsequent successful replacement - // succeeds, reusing the retained slot and keeping the pool count unchanged. + // 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. switchableFactory.ShouldFail = false; + fakeTime.Advance(TimeSpan.FromSeconds(5)); + Assert.False(pool.ErrorOccurred); var newConnection = pool.ReplaceConnection( owner1, oldConnection!, @@ -566,6 +573,70 @@ public void ReplaceConnection_NewPhysicalConnection_RespectsBlockingPeriod() 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 SwitchableSqlConnectionFactory(); + 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.ShouldFail = 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.ShouldFail = 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 ActivationFailSqlConnectionFactory(); + 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 From 1f1e766c62d5256338a3eb9bd63503f20d5aac9c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 09:20:43 -0700 Subject: [PATCH 36/39] Address Paul's review feedback - ConnectionPoolSlotsTest: move null-forgiveness to the Add() assignment instead of every use site; confirm the untouched occupant survives a failed TryReplace; add a self-replace test (benign no-op). - SqlConnection: name all args in the Open overrides ternary; drop a stray blank line. - ReplaceConnection tests: assert the replacement is not the old connection; assert the blocking-period throw is the same cached exception instance (with the factory flipped back to succeeding to prove the create path never ran). - Collapse the three test factories into one TunableSqlConnectionFactory (FailOnCreate/FailOnActivate) and fold ActivationFailDbConnectionInternal into StubDbConnectionInternal, which now reads the factory's flag live so idle-reuse tests can toggle it after creation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Microsoft/Data/SqlClient/SqlConnection.cs | 5 +- ...elDbConnectionPoolReplaceConnectionTest.cs | 146 +++++++----------- .../ConnectionPool/ConnectionPoolSlotsTest.cs | 37 ++++- 3 files changed, 88 insertions(+), 100 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 63c446b02f..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, forceNewConnection: false, overrides) : TryOpen(null, forceNewConnection: false, overrides))) + if (!(IsProviderRetriable ? + TryOpenWithRetry(retry: null, forceNewConnection: false, overrides: overrides) : + TryOpen(retry: null, forceNewConnection: false, overrides: overrides))) { throw ADP.InternalError(ADP.InternalErrorCode.SynchronousConnectReturnedPending); } @@ -1923,7 +1925,6 @@ public Task OpenAsync(SqlConnectionOverrides overrides, CancellationToken cancel private Task InternalOpenWithRetryAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) => RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(overrides, forceNewConnection, cancellationToken), cancellationToken); - private Task InternalOpenAsync(SqlConnectionOverrides overrides, bool forceNewConnection, CancellationToken cancellationToken) { long scopeID = SqlClientEventSource.Log.TryPoolerScopeEnterEvent("SqlConnection.InternalOpenAsync | API | Object Id {0}", ObjectID); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs index 56d1a200a7..1b22372de2 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolReplaceConnectionTest.cs @@ -19,7 +19,6 @@ namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool /// public class ChannelDbConnectionPoolReplaceConnectionTest { - private static readonly SqlConnectionFactory SuccessfulConnectionFactory = new SuccessfulSqlConnectionFactory(); /// /// Builds a for the replacement tests. A frozen @@ -66,7 +65,7 @@ private ChannelDbConnectionPool ConstructPool( public void ReplaceConnection_ReturnsNewConnection() { // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); + var pool = ConstructPool(new TunableSqlConnectionFactory()); SqlConnection owner = new(); pool.TryGetConnection( @@ -96,7 +95,7 @@ public void ReplaceConnection_ReturnsNewConnection() public void ReplaceConnection_OldConnectionIsDisposed() { // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); + var pool = ConstructPool(new TunableSqlConnectionFactory()); SqlConnection owner = new(); pool.TryGetConnection( @@ -129,7 +128,7 @@ public void ReplaceConnection_OldConnectionIsDisposed() public void ReplaceConnection_NewPhysicalConnection_PoolCountUnchanged() { // Arrange — single connection, no idle connections available - var pool = ConstructPool(SuccessfulConnectionFactory); + var pool = ConstructPool(new TunableSqlConnectionFactory()); SqlConnection owner = new(); pool.TryGetConnection( @@ -169,7 +168,7 @@ public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() hasTransactionAffinity: true, idleTimeout: 0 ); - var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions); + var pool = ConstructPool(new TunableSqlConnectionFactory(), poolGroupOptions); SqlConnection owner1 = new(); SqlConnection owner2 = new(); @@ -189,6 +188,7 @@ public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() // Assert — pool count must not exceed max Assert.NotNull(newConnection); + Assert.NotSame(conn1, newConnection); Assert.Equal(3, pool.Count); } @@ -204,8 +204,8 @@ public void ReplaceConnection_AtMaxCapacity_PoolCountUnchanged() public void ReplaceConnection_CreationFails_ExceptionPropagated() { // Arrange — use a factory that succeeds initially then fails - var switchableFactory = new SwitchableSqlConnectionFactory(); - var pool = ConstructPool(switchableFactory); + var factory = new TunableSqlConnectionFactory(); + var pool = ConstructPool(factory); SqlConnection owner = new(); pool.TryGetConnection( @@ -217,7 +217,7 @@ public void ReplaceConnection_CreationFails_ExceptionPropagated() Assert.NotNull(oldConnection); // Switch to failing mode - switchableFactory.ShouldFail = true; + factory.FailOnCreate = true; // Act & Assert — exception from factory is propagated Assert.Throws(() => @@ -247,9 +247,9 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() hasTransactionAffinity: true, idleTimeout: 0 ); - var switchableFactory = new SwitchableSqlConnectionFactory(); + var factory = new TunableSqlConnectionFactory(); var fakeTime = new FakeTimeProvider(); - var pool = ConstructPool(switchableFactory, poolGroupOptions, timeProvider: fakeTime); + var pool = ConstructPool(factory, poolGroupOptions, timeProvider: fakeTime); SqlConnection owner1 = new(); SqlConnection owner2 = new(); @@ -260,7 +260,7 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() Assert.Equal(2, pool.Count); // Switch to failing mode so the replacement creation throws. - switchableFactory.ShouldFail = true; + factory.FailOnCreate = true; // Act — replacement fails Assert.Throws(() => @@ -285,7 +285,7 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() // 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. - switchableFactory.ShouldFail = false; + factory.FailOnCreate = false; fakeTime.Advance(TimeSpan.FromSeconds(5)); Assert.False(pool.ErrorOccurred); var newConnection = pool.ReplaceConnection( @@ -309,7 +309,7 @@ public void ReplaceConnection_CreationFails_OldConnectionRetainedForRetry() public void ReplaceConnection_ActivationFails_ExceptionPropagated() { // Arrange - var factory = new ActivationFailSqlConnectionFactory(); + var factory = new TunableSqlConnectionFactory(); var pool = ConstructPool(factory); SqlConnection owner = new(); @@ -342,7 +342,7 @@ public void ReplaceConnection_ActivationFails_ExceptionPropagated() public void ReplaceConnection_ActivationFails_NewConnectionDisposed_PoolCountStable() { // Arrange - var factory = new ActivationFailSqlConnectionFactory(); + var factory = new TunableSqlConnectionFactory(); var pool = ConstructPool(factory); SqlConnection owner = new(); @@ -386,7 +386,7 @@ public void ReplaceConnection_ActivationFails_NewConnectionDisposed_PoolCountSta public void ReplaceConnection_PrefersIdleOverNewConnection() { // Arrange — open two connections, then return one so it becomes an idle connection. - var pool = ConstructPool(SuccessfulConnectionFactory); + var pool = ConstructPool(new TunableSqlConnectionFactory()); SqlConnection owner1 = new(); SqlConnection owner2 = new(); @@ -432,7 +432,7 @@ public void ReplaceConnection_IdleReuse_AtMaxCapacity_FreesOldSlot() hasTransactionAffinity: true, idleTimeout: 0 ); - var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions); + var pool = ConstructPool(new TunableSqlConnectionFactory(), poolGroupOptions); SqlConnection owner1 = new(); SqlConnection owner2 = new(); @@ -468,7 +468,7 @@ public void ReplaceConnection_IdleReuse_AtMaxCapacity_FreesOldSlot() public void ReplaceConnection_IdleReuse_ActivationFails_ReturnedToPool() { // Arrange — open two connections, then return one so it becomes an idle connection. - var factory = new ActivationFailSqlConnectionFactory(); + var factory = new TunableSqlConnectionFactory(); var pool = ConstructPool(factory); SqlConnection owner1 = new(); SqlConnection owner2 = new(); @@ -513,7 +513,7 @@ public void ReplaceConnection_IdleReuse_ActivationFails_ReturnedToPool() public void ReplaceConnection_NoIdleConnection_CreatesNew() { // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); + var pool = ConstructPool(new TunableSqlConnectionFactory()); SqlConnection owner = new(); pool.TryGetConnection(owner, null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out DbConnectionInternal? conn1); @@ -547,25 +547,30 @@ public void ReplaceConnection_NoIdleConnection_CreatesNew() public void ReplaceConnection_NewPhysicalConnection_RespectsBlockingPeriod() { // Arrange — localhost is non-Azure, so the pool's blocking period is enabled by default. - var factory = new SwitchableSqlConnectionFactory(); + var factory = new TunableSqlConnectionFactory(); var pool = ConstructPool(factory); SqlConnection owner = new(); // Check out a connection to later replace (creation succeeds). - factory.ShouldFail = false; + 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.ShouldFail = true; - Assert.Throws(() => + 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. - Assert.Throws(() => + // 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); @@ -585,19 +590,19 @@ public void ReplaceConnection_NewPhysicalConnection_RespectsBlockingPeriod() public void ReplaceConnection_NewPhysicalConnectionFails_EntersBlockingPeriod() { // Arrange — localhost is non-Azure, so the pool's blocking period is enabled by default. - var factory = new SwitchableSqlConnectionFactory(); + 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.ShouldFail = false; + 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.ShouldFail = true; + factory.FailOnCreate = true; Assert.Throws(() => pool.ReplaceConnection(owner, oldConnection!, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)))); @@ -619,7 +624,7 @@ public void ReplaceConnection_NewPhysicalConnectionFails_EntersBlockingPeriod() public void ReplaceConnection_ActivationFails_DoesNotEnterBlockingPeriod() { // Arrange - var factory = new ActivationFailSqlConnectionFactory(); + var factory = new TunableSqlConnectionFactory(); var pool = ConstructPool(factory); SqlConnection owner = new(); @@ -641,23 +646,20 @@ public void ReplaceConnection_ActivationFails_DoesNotEnterBlockingPeriod() #region Test Helper Classes - internal class SuccessfulSqlConnectionFactory : SqlConnectionFactory + /// + /// 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 { - protected override DbConnectionInternal CreateConnection( - SqlConnectionOptions options, - ConnectionPoolKey poolKey, - DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, - IDbConnectionPool pool, - DbConnection owningConnection, - TimeoutTimer timeout) - { - return new StubDbConnectionInternal(); - } - } + /// When true, throws instead of returning a connection. + internal bool FailOnCreate { get; set; } - internal class SwitchableSqlConnectionFactory : SqlConnectionFactory - { - internal bool ShouldFail { get; set; } + /// When true, activating any connection from this factory throws. + internal bool FailOnActivate { get; set; } protected override DbConnectionInternal CreateConnection( SqlConnectionOptions options, @@ -667,65 +669,25 @@ protected override DbConnectionInternal CreateConnection( DbConnection owningConnection, TimeoutTimer timeout) { - if (ShouldFail) + if (FailOnCreate) { throw new InvalidOperationException("Simulated connection failure"); } - return new StubDbConnectionInternal(); - } - } - internal class ActivationFailSqlConnectionFactory : SqlConnectionFactory - { - internal bool FailOnActivate { get; set; } - - protected override DbConnectionInternal CreateConnection( - SqlConnectionOptions options, - ConnectionPoolKey poolKey, - DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, - IDbConnectionPool pool, - DbConnection owningConnection, - TimeoutTimer timeout) - { - return new ActivationFailDbConnectionInternal(this); + 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 { - public override string ServerVersion => throw new NotImplementedException(); - - public override DbTransaction BeginTransaction(System.Data.IsolationLevel il) - { - throw new NotImplementedException(); - } - - public override void EnlistTransaction(Transaction transaction) - { - return; - } - - protected override void Activate(Transaction transaction) - { - return; - } - - protected override void Deactivate() - { - return; - } - - internal override void ResetConnection() - { - return; - } - } - - internal class ActivationFailDbConnectionInternal : DbConnectionInternal - { - private readonly ActivationFailSqlConnectionFactory _factory; + private readonly TunableSqlConnectionFactory? _factory; - internal ActivationFailDbConnectionInternal(ActivationFailSqlConnectionFactory factory) + internal StubDbConnectionInternal(TunableSqlConnectionFactory? factory = null) { _factory = factory; } @@ -744,7 +706,7 @@ public override void EnlistTransaction(Transaction transaction) protected override void Activate(Transaction transaction) { - if (_factory.FailOnActivate) + if (_factory?.FailOnActivate == true) { throw new InvalidOperationException("Simulated activation failure"); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs index 011533f773..53390068fb 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ConnectionPoolSlotsTest.cs @@ -495,12 +495,12 @@ public void TryReplace_ExistingConnection_ReturnsTrueAndKeepsReservationCount() var poolSlots = new ConnectionPoolSlots(5); var oldConnection = poolSlots.Add( createCallback: () => new MockDbConnectionInternal(), - cleanupCallback: (conn) => { }); + cleanupCallback: (conn) => { })!; var newConnection = new MockDbConnectionInternal(); var reservationCountBeforeReplace = poolSlots.ReservationCount; // Act - var replaced = poolSlots.TryReplace(oldConnection!, newConnection); + var replaced = poolSlots.TryReplace(oldConnection, newConnection); // Assert - the slot is reused, so the reservation count is unchanged Assert.True(replaced); @@ -519,15 +519,15 @@ public void TryReplace_ExistingConnection_NewConnectionOccupiesSlot() var poolSlots = new ConnectionPoolSlots(5); var oldConnection = poolSlots.Add( createCallback: () => new MockDbConnectionInternal(), - cleanupCallback: (conn) => { }); + cleanupCallback: (conn) => { })!; var newConnection = new MockDbConnectionInternal(); // Act - poolSlots.TryReplace(oldConnection!, newConnection); + 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.False(poolSlots.TryRemove(oldConnection)); Assert.True(poolSlots.TryRemove(newConnection)); Assert.Equal(0, poolSlots.ReservationCount); } @@ -544,7 +544,7 @@ public void TryReplace_NonExistentConnection_ReturnsFalseAndDoesNotAddNewConnect var poolSlots = new ConnectionPoolSlots(5); var existingConnection = poolSlots.Add( createCallback: () => new MockDbConnectionInternal(), - cleanupCallback: (conn) => { }); + cleanupCallback: (conn) => { })!; var missingConnection = new MockDbConnectionInternal(); var newConnection = new MockDbConnectionInternal(); var reservationCountBeforeReplace = poolSlots.ReservationCount; @@ -557,6 +557,31 @@ public void TryReplace_NonExistentConnection_ReturnsFalseAndDoesNotAddNewConnect 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); } /// From c8e7f11e84eb98d118a4f7c7cdaeadb87298b887 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 10:26:54 -0700 Subject: [PATCH 37/39] Implement transaction support in ChannelDbConnectionPool Ports the transacted-pool state machine from WaitHandleDbConnectionPool so the channel pool honors ambient System.Transactions enlistment: - Implement PutObjectFromTransactedPool and TransactionEnded (previously NotImplementedException). - Rewrite ReturnInternalConnection to mirror DeactivateObject: deactivate first, then route the connection to the transacted pool, stasis, the idle channel, or destruction under the connection lock. - Vend connections already enlisted in the ambient transaction via a new GetFromTransactedPool helper, and pass the transaction through to PrepareConnection/ActivateConnection. - Set the ambient transaction on the async acquisition path from the TaskCompletionSource's AsyncState. - Guard RemoveConnection against disposing a transaction root that is still waiting for its delegated transaction to end. Adds ChannelDbConnectionPoolTransactionTest mirroring the WaitHandle pool's transaction test suite, and drops the stale NotImplementedException tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 285 ++++- .../ChannelDbConnectionPoolTest.cs | 31 - .../ChannelDbConnectionPoolTransactionTest.cs | 1079 +++++++++++++++++ 3 files changed, 1337 insertions(+), 58 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs 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 b2d4427b47..682e1ae15e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -5,6 +5,7 @@ using System.Collections.Concurrent; using System.Collections.ObjectModel; using System.Data.Common; +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Channels; @@ -250,6 +251,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 +322,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); + } } /// @@ -331,7 +364,8 @@ public DbConnectionInternal ReplaceConnection( if (newConnection is not null) { - // TODO: Full transaction enlistment support (Story 2). + // 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); @@ -373,7 +407,8 @@ public DbConnectionInternal ReplaceConnection( newConnection.PostPop(owningObject); } - // TODO: Full transaction enlistment support (Story 2). + // 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 @@ -414,6 +449,116 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti { ValidateOwnershipAndSetPoolingState(connection, owningObject); + 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); + } + + // 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 @@ -432,27 +577,12 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti return; } - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Connection {1}, Deactivating.", - 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); - } - } } /// @@ -606,7 +736,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); } /// @@ -674,7 +818,8 @@ public bool TryGetConnection( // We're potentially on a new thread, so we need to properly set the ambient transaction. // We rely on the caller to capture the ambient transaction in the TaskCompletionSource's AsyncState // so that we can access it here. Read: area for improvement. - // TODO: ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); + DbConnectionInternal? connection = null; try @@ -941,6 +1086,19 @@ private bool IsLiveConnection(DbConnectionInternal connection) /// The connection to be closed. private void RemoveConnection(DbConnectionInternal connection) { + // 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; + } + _connectionSlots.TryRemove(connection); // Removing a connection from the pool opens a free slot. @@ -1010,14 +1168,26 @@ private async Task GetInternalConnection( TimeoutTimer timeout) { DbConnectionInternal? connection = null; + Transaction? transaction = null; + + // If automatic transaction enlistment is enabled, we first try to get a connection that + // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot + // vend connections from the transacted pool. + if (HasTransactionAffinity) + { + connection = GetFromTransactedPool(out 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 { @@ -1062,9 +1232,8 @@ private async Task GetInternalConnection( connection = null; } } - while (connection is null); - PrepareConnection(owningConnection, connection); + PrepareConnection(owningConnection, connection, transaction); return connection; } @@ -1133,6 +1302,68 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } } + /// + /// Attempts to retrieve a connection that is already enlisted in the ambient transaction. + /// + /// Receives the ambient transaction, or null when there is none. + /// The caller must enlist whatever connection it ends up using in this transaction, even + /// when no transacted connection was available. + /// A live connection already enlisted in the ambient transaction, or null. + private DbConnectionInternal? GetFromTransactedPool(out Transaction? transaction) + { + transaction = ADP.GetCurrentTransaction(); + if (transaction is null) + { + return null; + } + + 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. /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 604964927e..90417c5e96 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -887,37 +887,6 @@ public void TestReplaceConnection() #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!)); - } - - /// - /// Verifies that - /// remains unimplemented and throws . - /// - [Fact] - public void TestTransactionEnded() - { - // Arrange - var pool = ConstructPool(SuccessfulConnectionFactory); - - // Act & Assert - Assert.Throws(() => pool.TransactionEnded(null!, null!)); - } - #endregion - #region Pool Clear Tests /// 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..7d2bfb95c5 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -0,0 +1,1079 @@ +// 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 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 +} From c5404962c619e3854e688e3987bdb7cfb17d3730 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Wed, 29 Jul 2026 11:57:04 -0700 Subject: [PATCH 38/39] Flow the ambient transaction explicitly on the async open path The async open path ran GetInternalConnection inside a Task.Run and restored the ambient transaction by assigning Transaction.Current on that thread pool thread. That assignment writes to thread-static storage which ExecutionContext does not unwind, so the transaction outlived the open and was observable by unrelated work later scheduled onto the same thread -- including the login-time auto-enlistment that non-pooled connections perform against Transaction.Current. A try/finally restore is not sufficient either, because the continuation may resume on a different thread than the one that was polluted. Instead, capture the ambient transaction on the caller's thread (from the TaskCompletionSource's AsyncState, which is where SqlConnection.OpenAsync puts it) and thread it explicitly through GetInternalConnection into GetFromTransactedPool and PrepareConnection. The sync path passes ADP.GetCurrentTransaction() directly since it runs on the caller's thread. Also gate the transaction on HasTransactionAffinity in one place so a pool without automatic enlistment neither reads from nor writes to the transacted store. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 65 ++++++++------ .../ChannelDbConnectionPoolTransactionTest.cs | 90 +++++++++++++++++++ 2 files changed, 129 insertions(+), 26 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index 682e1ae15e..3f8db0d018 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 @@ -776,10 +776,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 @@ -808,6 +810,22 @@ 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; + Task.Run(async () => { if (taskCompletionSource.Task.IsCompleted) @@ -815,11 +833,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. - ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); - DbConnectionInternal? connection = null; try @@ -827,7 +840,8 @@ public bool TryGetConnection( connection = await GetInternalConnection( owningObject, async: true, - timeout + timeout, + ambientTransaction ).ConfigureAwait(false); if (!taskCompletionSource.TrySetResult(connection)) @@ -1153,6 +1167,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 @@ -1165,17 +1183,20 @@ private void RemoveConnection(DbConnectionInternal connection) private async Task GetInternalConnection( DbConnection owningConnection, bool async, - TimeoutTimer timeout) + TimeoutTimer timeout, + Transaction? ambientTransaction) { DbConnectionInternal? connection = null; - Transaction? transaction = null; - // If automatic transaction enlistment is enabled, we first try to get a connection that - // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot - // vend connections from the transacted pool. - if (HasTransactionAffinity) + // 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(out transaction); + connection = GetFromTransactedPool(transaction); } // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations @@ -1303,20 +1324,12 @@ private void PrepareConnection(DbConnection owningObject, DbConnectionInternal c } /// - /// Attempts to retrieve a connection that is already enlisted in the ambient transaction. + /// Attempts to retrieve a connection that is already enlisted in the given transaction. /// - /// Receives the ambient transaction, or null when there is none. - /// The caller must enlist whatever connection it ends up using in this transaction, even - /// when no transacted connection was available. - /// A live connection already enlisted in the ambient transaction, or null. - private DbConnectionInternal? GetFromTransactedPool(out Transaction? 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) { - transaction = ADP.GetCurrentTransaction(); - if (transaction is null) - { - return null; - } - DbConnectionInternal? connection = TransactedConnectionPool.GetTransactedObject(transaction); if (connection is null) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs index 7d2bfb95c5..53667f418d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs @@ -1021,6 +1021,96 @@ public void ReplaceConnection_CarriesEnlistedTransactionToNewConnection() #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 From ca8079b82bd7e4d1c9911ad2050cf9c230ff0868 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 4 Aug 2026 15:17:25 -0700 Subject: [PATCH 39/39] Reclaim emancipated connections in ChannelDbConnectionPool A SqlConnection that is garbage collected without ever being closed or disposed leaves its internal connection "emancipated": still tracked by the pool, but with no owner that could ever return it. WaitHandleDbConnectionPool sweeps for these before waiting for a free connection; ChannelDbConnectionPool did not, so an emancipated connection permanently occupied a pool slot. At MaxPoolSize that meant every subsequent Open timed out -- forever, not just once. GetInternalConnection now performs the same sweep just before parking on the idle channel. 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. The sweep takes the connection lock with Monitor.TryEnter rather than Enter. IsEmancipated has to be read under that 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, so skipping it costs nothing and keeps the sweep from blocking the caller. Only PrePush happens under the lock; deactivation, which can make server round trips, is deferred until all locks are released. Deactivating and routing a returned connection is now factored out of ReturnInternalConnection into DeactivateAndRouteConnection so reclamation can share it. Reclamation must not go through ReturnInternalConnection itself because it has already performed the PrePush and there is no owning object left to validate against. Tests: - Added ConnectionPoolVersionScope, which flips the pool version switch and clears all pools on both entry and exit. Clearing is required because a pool binds to its implementation at creation time, so without it pools leak across tests. - Parameterized ReclaimEmancipatedOnOpenTest and MaxPoolWaitForConnectionTest by pool version. ReclaimEmancipatedOnOpenTest fails against ChannelDbConnectionPool without this fix. - Three pool-exhaustion unit tests let their owning SqlConnections go out of scope, so reclamation could legitimately hand the "should time out" waiter a connection. They now keep the owners alive, which is what they meant anyway. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 87 +++++++++++++++++++ .../ConnectionPool/ConnectionPoolSlots.cs | 23 +++++ .../Common/ConnectionPoolVersionScope.cs | 62 +++++++++++++ .../ConnectionPoolTest/ConnectionPoolTest.cs | 15 ++-- .../ChannelDbConnectionPoolTest.cs | 31 ++++++- 5 files changed, 209 insertions(+), 9 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs 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 3f8db0d018..7328ee5406 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; using System.Collections.Concurrent; +using System.Collections.Generic; using System.Collections.ObjectModel; using System.Data.Common; using System.Diagnostics; @@ -449,6 +450,19 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti { 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, @@ -1225,6 +1239,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. @@ -1258,6 +1284,67 @@ private async Task GetInternalConnection( 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); + + connection.DetachCurrentTransactionIfEnded(); + DeactivateAndRouteConnection(connection); + } + + return true; + } + /// /// Performs a blocking synchronous read from the idle connection channel. /// diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs index c9d268fd29..27a39df22b 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; @@ -190,6 +191,28 @@ internal bool TryReplace(DbConnectionInternal oldConnection, DbConnectionInterna 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/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/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 90417c5e96..7fda13b24f 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); } ///