From d3e2fe79c7d5b29a67559ead383c568d5df47a62 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 23 Jun 2026 10:32:04 -0700 Subject: [PATCH 01/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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 000531bf42b66b06abfce6b305bf03a49a40ea14 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 10:05:45 -0700 Subject: [PATCH 09/12] Address Paul's review: doc/comment clarity and test consolidation - NoOpAcquiredLease: document that the shared Instance is stateless and its Dispose is an idempotent no-op, safe to dispose repeatedly/concurrently. - ChannelDbConnectionPool: add a class-level pointer to the feature spec so the FR-0xx requirement tags in comments are resolvable; note at lease.Dispose() that the no-limiter case disposes the idempotent singleton; document the faulted flag and how the finally uses it to avoid a redundant idle-channel poke. - Tests: merge the three ErrorOccurred_* blocking-period facts into a single Theory parameterized by the connection string's Pool Blocking Period; comment that a rising TotalFailedLeases is how the wake test detects requestB was denied a permit and parked on the idle channel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 17 ++++ .../ConnectionPool/NoOpAcquiredLease.cs | 9 ++- .../ChannelDbConnectionPoolTest.cs | 79 ++++--------------- 3 files changed, 39 insertions(+), 66 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 8a71cf3d00..afb37a4023 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 @@ -48,6 +48,13 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// /// The trade-off is slightly higher memory overhead per pool instance due to the channel infrastructure, /// but this is generally offset by the performance benefits in async-heavy workloads. + /// + /// + /// Comments in this file reference requirements by tag (e.g. FR-001). These tags are + /// defined in the feature spec at specs/006-pool-rate-limiting/spec.md (see the + /// "Functional Requirements" section); consult it for the authoritative description of each + /// requirement. + /// /// internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable { @@ -581,6 +588,13 @@ public bool TryGetConnection( // FR-001, FR-002, FR-003. RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; + + // Tracks whether we are leaving this block via an exception rather than a + // normal return. It is read in the finally below to decide whether to poke + // the idle channel: every return path sets it to false first, so by the time + // the finally runs it is true only when an exception is propagating. This + // lets us skip the wake on exception paths, where cleanupCallback already + // pokes, and avoid a redundant double wake. bool faulted = true; try { @@ -624,6 +638,9 @@ public bool TryGetConnection( // 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. + // When no limiter is configured this is NoOpAcquiredLease.Instance, whose + // Dispose is an idempotent no-op, so disposing the shared singleton here + // is safe. lease.Dispose(); // After releasing, signal a waiter on the idle channel that they may now 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 index 96785fe0e8..079066b08a 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs @@ -19,7 +19,10 @@ namespace Microsoft.Data.SqlClient.ConnectionPool internal sealed class NoOpAcquiredLease : RateLimitLease { /// - /// The shared singleton instance. + /// The shared singleton instance. Because it is stateless and its + /// releases nothing, the same instance is handed to every caller that opens without a + /// configured limiter and may be disposed any number of times, on any thread, without + /// affecting other callers. /// public static readonly NoOpAcquiredLease Instance = new(); @@ -39,7 +42,9 @@ public override bool TryGetMetadata(string metadataName, out object? metadata) protected override void Dispose(bool disposing) { - // No resources to release. + // No resources to release. This is intentionally idempotent: the shared Instance is + // handed to every no-limiter open, so Dispose may be called repeatedly and + // concurrently with no effect. } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index b200e6ffc5..08b52ea06e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -1610,46 +1610,21 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() /// /// Verifies that a connection creation failure enters the blocking-period error state when - /// blocking is enabled for the pool. + /// blocking is enabled for the pool, and stays out of it when blocking is disabled. The + /// blocking policy is driven by the connection string's Pool Blocking Period: + /// Default/Auto enable blocking for a non-Azure host (localhost), AlwaysBlock forces it on, + /// and NeverBlock suppresses it. FR-006, FR-007. /// - [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() + [Theory] + [InlineData("", true)] // Default (unspecified) => Auto => blocks for localhost + [InlineData("Pool Blocking Period=Auto;", true)] // Auto => blocks for non-Azure host + [InlineData("Pool Blocking Period=NeverBlock;", false)] + [InlineData("Pool Blocking Period=AlwaysBlock;", true)] + public void ErrorOccurred_OnFailure_FollowsBlockingPeriod(string blockingPeriodClause, bool expectErrorOccurred) { // Arrange var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions("Data Source=localhost;Pool Blocking Period=NeverBlock;"), + new SqlConnectionOptions($"Data Source=localhost;{blockingPeriodClause}"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -1662,40 +1637,13 @@ public void ErrorOccurred_FailureWithNeverBlock_StaysFalse() 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); + Assert.Equal(expectErrorOccurred, pool.ErrorOccurred); } /// @@ -1956,6 +1904,9 @@ public async Task RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysical "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. + // A denied AttemptAcquire increments the limiter's TotalFailedLeases, so watching that + // counter rise is how we confirm B was refused a permit and is now parked on the idle + // channel waiting for a wakeup (rather than still racing to acquire). long failedLeasesBefore = rateLimiter.GetStatistics()!.TotalFailedLeases; Task requestB = Open(new SqlConnection()); Assert.True( From 3e82378fbb80997fd5f5313d870e11256732ac77 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 10:15:44 -0700 Subject: [PATCH 10/12] Rewrite pool recovery test using TimeProvider seam Replaces the hollow SuccessfulCreate_AfterFailure_ClearsErrorState test (which called pool.Clear() and made its final assertion vacuous) with Failure_ThenBlockingPeriodExpiry_AllowsSuccessfulCreate. The new test drives the blocking-period exit timer deterministically via an injected FakeTimeProvider, verifying the real recovery path: failure enters the blocking period, in-window requests fast-fail without touching the factory, and once the backoff elapses a create succeeds and clears the error state. Adds an optional TimeProvider parameter to the ChannelDbConnectionPool constructor (forwarded to BlockingPeriodErrorState) and to the ConstructPool test helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../ConnectionPool/ChannelDbConnectionPool.cs | 8 +++- .../ChannelDbConnectionPoolTest.cs | 41 +++++++++++++------ 2 files changed, 34 insertions(+), 15 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 afb37a4023..c3a92a726b 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 @@ -134,7 +134,8 @@ internal ChannelDbConnectionPool( DbConnectionPoolGroup connectionPoolGroup, DbConnectionPoolIdentity identity, DbConnectionPoolProviderInfo connectionPoolProviderInfo, - ConcurrencyLimiter? connectionCreationRateLimiter = null) + ConcurrencyLimiter? connectionCreationRateLimiter = null, + TimeProvider? timeProvider = null) { ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; @@ -150,7 +151,10 @@ internal ChannelDbConnectionPool( _idleChannel = new(); if (PoolGroup.IsBlockingPeriodEnabled()) { - _errorState = new BlockingPeriodErrorState(_instanceId); + // timeProvider is injected only by tests so the blocking-period exit timer can be + // driven deterministically; in production it is null and the error state falls back + // to TimeProvider.System. + _errorState = new BlockingPeriodErrorState(_instanceId, timeProvider: timeProvider); } // Pruning is only useful when the pool can grow beyond MinPoolSize. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 08b52ea06e..505d750b1a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -38,13 +38,15 @@ public class ChannelDbConnectionPoolTest /// Optional pool options override. /// Optional provider info override. /// Optional concurrency limiter controlling physical connection creation. + /// Optional time provider so tests can drive the blocking-period exit timer deterministically. /// A configured instance for testing. private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFactory, DbConnectionPoolIdentity? identity = null, DbConnectionPoolGroup? dbConnectionPoolGroup = null, DbConnectionPoolGroupOptions? poolGroupOptions = null, DbConnectionPoolProviderInfo? connectionPoolProviderInfo = null, - ConcurrencyLimiter? connectionCreationRateLimiter = null) + ConcurrencyLimiter? connectionCreationRateLimiter = null, + TimeProvider? timeProvider = null) { poolGroupOptions ??= new DbConnectionPoolGroupOptions( poolByIdentity: false, @@ -65,7 +67,8 @@ private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFac dbConnectionPoolGroup, identity ?? DbConnectionPoolIdentity.NoIdentity, connectionPoolProviderInfo ?? new DbConnectionPoolProviderInfo(), - connectionCreationRateLimiter + connectionCreationRateLimiter, + timeProvider ); } @@ -1719,14 +1722,19 @@ public void Clear_InErrorState_ResetsErrorOccurred() } /// - /// Verifies that a successful connection creation after a prior failure leaves the pool out - /// of the blocking-period error state. + /// Verifies the pool's blocking-period recovery path end-to-end: a creation failure enters + /// the blocking period (fast-failing subsequent requests without touching the factory), and + /// once the backoff interval elapses the pool leaves the blocking period so a later create + /// succeeds and clears the error state. The blocking-period exit timer is driven with a + /// so the test is deterministic and does not wait on + /// wall-clock time. FR-006, FR-009. /// [Fact] - public void SuccessfulCreate_AfterFailure_ClearsErrorState() + public void Failure_ThenBlockingPeriodExpiry_AllowsSuccessfulCreate() { // Arrange var factory = new ToggleFailureConnectionFactory(); + var fakeTime = new FakeTimeProvider(); var dbConnectionPoolGroup = new DbConnectionPoolGroup( new SqlConnectionOptions("Data Source=localhost;"), new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), @@ -1738,22 +1746,29 @@ public void SuccessfulCreate_AfterFailure_ClearsErrorState() loadBalanceTimeout: 0, hasTransactionAffinity: true, idleTimeout: 0)); - var pool = ConstructPool(factory, dbConnectionPoolGroup: dbConnectionPoolGroup); + var pool = ConstructPool(factory, dbConnectionPoolGroup: dbConnectionPoolGroup, timeProvider: fakeTime); - // First call fails and enters the error state. + // The first create fails and enters the blocking period. 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); - + // Even though creates would now succeed, requests issued inside the blocking window + // still fast-fail with the cached exception without reaching the factory. Flipping the + // factory to succeed and still observing a throw proves the create path never ran. factory.FailNextCreate = false; + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); + Assert.True(pool.ErrorOccurred); - // Act + // Advancing past the initial backoff fires the exit timer, ending the blocking period. + // FakeTimeProvider invokes the timer callback synchronously, so the error state clears + // before the next line runs. + fakeTime.Advance(TimeSpan.FromSeconds(5)); + Assert.False(pool.ErrorOccurred); + + // Act - a create after the blocking period succeeds and leaves the pool healthy. var completed = pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out var conn); // Assert From 08ad22eebad28ad027888c634fee2e62f323c618 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 10:37:28 -0700 Subject: [PATCH 11/12] Clarify rate-limiter is scoped to a single pool The ConcurrencyLimiter field doc previously suggested a limiter could be shared across pools. Because the permit-release wake is signaled on this pool's own idle channel, a permit released by another pool would not wake waiters parked here. Reword the doc to state the limiter is expected to be scoped to a single pool and that cross-pool sharing is unsupported, while keeping the caller-owns-lifetime note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 9 ++++++--- 1 file changed, 6 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 c3a92a726b..10d22eea02 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 @@ -112,9 +112,12 @@ 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. + /// Scope note: the permit-release wake is signaled on this pool's own idle channel, so a + /// limiter is expected to be scoped to a single pool. Sharing one limiter across multiple + /// pools is not supported, because a permit released by another pool would not wake waiters + /// parked here and they could stall until their timeout expires. + /// Lifetime note: the pool does not own this limiter and never disposes it; the caller that + /// constructs the limiter owns its lifetime (it may outlive the pool). /// private readonly ConcurrencyLimiter? _connectionCreationRateLimiter; From a804da82c81877a55c84b1c06e26f791bbc69b7d Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 16 Jul 2026 10:48:41 -0700 Subject: [PATCH 12/12] Capture lease.IsAcquired before disposing the lease The permit-release poke condition read lease.IsAcquired after lease.Dispose() had run. Even though the current ConcurrencyLimiter lease keeps IsAcquired stable post-dispose, using an object after disposal is fragile if the lease implementation changes. Capture the value into a local before disposing and use the captured bool in the poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | 7 ++++++- 1 file changed, 6 insertions(+), 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 10d22eea02..298c38b9db 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 @@ -640,6 +640,11 @@ public bool TryGetConnection( } finally { + // Capture the acquired state before disposing: the poke condition below + // reads it, and accessing a lease after Dispose is fragile even if the + // current ConcurrencyLimiter lease happens to keep IsAcquired stable. + bool leaseAcquired = lease.IsAcquired; + // 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, @@ -661,7 +666,7 @@ public bool TryGetConnection( // immediately has an available permit, but the waiter we wake will fall // back to waiting again if not. if (!faulted && - lease.IsAcquired && + leaseAcquired && _connectionCreationRateLimiter is not null && _connectionSlots.ReservationCount < MaxPoolSize) {