From d3e2fe79c7d5b29a67559ead383c568d5df47a62 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 23 Jun 2026 10:32:04 -0700 Subject: [PATCH 01/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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/30] 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();