From beda49891354b8274904ca3f520f81205ff9be85 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 19 May 2026 16:02:19 -0700 Subject: [PATCH 01/23] initial implementation --- Directory.Packages.props | 1 + specs/006-pool-rate-limiting/spec.md | 146 ++++++++ .../src/Microsoft.Data.SqlClient.csproj | 2 + .../ConnectionPool/ChannelDbConnectionPool.cs | 346 ++++++++++++++++-- .../ChannelDbConnectionPoolTest.cs | 170 +++++++++ 5 files changed, 630 insertions(+), 35 deletions(-) create mode 100644 specs/006-pool-rate-limiting/spec.md diff --git a/Directory.Packages.props b/Directory.Packages.props index 62918f7d65..d318d45b88 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -101,6 +101,7 @@ + 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 cd7fdb4b79..1455e2bc41 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj +++ b/src/Microsoft.Data.SqlClient/src/Microsoft.Data.SqlClient.csproj @@ -276,6 +276,7 @@ + @@ -288,6 +289,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 b83434cac6..310114c285 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 @@ -9,6 +9,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; @@ -92,6 +93,53 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool /// Must be updated using operations to ensure thread safety. /// private volatile int _isClearing; + + /// + /// Throttles the number of concurrent physical connection creation attempts to prevent login + /// storms against the database server. Callers acquire a lease before creating a connection and + /// dispose it when creation completes (success or failure). + /// + private readonly RateLimiter _connectionCreationRateLimiter; + + /// + /// Initial backoff (in milliseconds) used when the pool enters the error state after a + /// connection creation failure. + /// + private const int InitialErrorWaitMs = 5_000; + + /// + /// Maximum backoff (in milliseconds) for the pool error state. + /// + private const int MaxErrorWaitMs = 60_000; + + /// + /// True when the pool is currently in the error (blocking) state. When true, all new + /// connection requests fail fast with until the error timer clears + /// the state. + /// + private volatile bool _errorOccurred; + + /// + /// The cached exception from the last failed connection creation attempt. Used to fail fast + /// while in the error state. + /// + private Exception? _resError; + + /// + /// Timer used to exit the error state after the current backoff interval elapses. + /// + private Timer? _errorTimer; + + /// + /// Current backoff interval (in milliseconds) used the next time the pool enters the error + /// state. Doubles on each failure up to . + /// + private int _errorWaitMs = InitialErrorWaitMs; + + /// + /// Guards mutations to the error state (the error timer, cached exception, and backoff). + /// + private readonly object _errorStateLock = new(); #endregion /// @@ -115,6 +163,20 @@ internal ChannelDbConnectionPool( _connectionSlots = new(MaxPoolSize); _idleChannel = new(); + // Limit concurrent connection creation attempts. The cap is bounded by MaxPoolSize since + // we can never have more in-flight creations than the pool can hold. We use a small but + // non-trivial default so that callers queue (FIFO) rather than stampede the server with + // simultaneous logins. The QueueLimit is set to MaxPoolSize so we never silently reject + // a caller -- a caller that cannot acquire immediately waits until either its own + // ConnectTimeout elapses or capacity is available. + int maxConcurrentCreations = Math.Max(1, Math.Min((int)MaxPoolSize, Environment.ProcessorCount)); + _connectionCreationRateLimiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions + { + PermitLimit = maxConcurrentCreations, + QueueProcessingOrder = QueueProcessingOrder.OldestFirst, + QueueLimit = (int)MaxPoolSize, + }); + State = Running; } @@ -134,8 +196,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 => _errorOccurred; /// public int Id => _instanceId; @@ -177,6 +238,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. + ClearErrorState(); + Interlocked.Increment(ref _clearGeneration); // If another thread is already draining, skip the drain. The generation counter has @@ -368,53 +433,261 @@ 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. + /// Whether the call is in an async context. /// The cancellation token to cancel the operation. - /// 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. /// /// Thrown when the cancellation token is cancelled before the connection operation completes. /// - private DbConnectionInternal? OpenNewInternalConnection( - DbConnection? owningConnection, + private async Task OpenNewInternalConnectionAsync( + DbConnection? owningConnection, + bool async, CancellationToken cancellationToken) { 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. + ThrowIfInErrorState(); + + // Quick (racy) capacity check: if we're at MaxPoolSize there's no point waiting for the + // rate limiter -- the caller will have to block on the idle channel instead. + if (_connectionSlots.ReservationCount >= MaxPoolSize) + { + return null; + } + + // Throttle concurrent creation attempts. Time spent waiting here counts against the + // caller's ConnectTimeout because the cancellation token already encodes the timeout. + // FR-001, FR-002, FR-003. + RateLimitLease lease; + if (async) + { + lease = await _connectionCreationRateLimiter.AcquireAsync(1, cancellationToken).ConfigureAwait(false); + } + else + { + // The lease acquisition completes synchronously when capacity is available and only + // blocks (queues) otherwise. We rely on the surrounding sync-over-async semantics in + // the caller to bound thread pool usage on the sync path. + lease = _connectionCreationRateLimiter.AcquireAsync(1, cancellationToken) + .AsTask().GetAwaiter().GetResult(); + } - return _connectionSlots.Add( - createCallback: () => + try + { + if (!lease.IsAcquired) { - // 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. - var connection = ConnectionFactory.CreatePooledConnection( - owningConnection, - this); + // The rate limiter refused to admit us (e.g. the queue is full) but the + // caller's CancellationToken has not fired, so we have not actually timed out. + // Return null so the outer GetInternalConnection loop falls through to the + // idle-channel wait. That wait observes the real timeout and will wake up + // either when capacity frees (a peer create completes or a connection is + // returned) or when the timeout elapses. The loop then cycles and re-attempts + // lease acquisition. + return null; + } - if (connection is not null) + cancellationToken.ThrowIfCancellationRequested(); + + // Re-check the error state after acquiring the lease -- it may have been set while we + // waited. + ThrowIfInErrorState(); + + DbConnectionInternal? connection; + try + { + connection = _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. + var newConnection = ConnectionFactory.CreatePooledConnection( + owningConnection, + this); + + if (newConnection is not null) + { + newConnection.ClearGeneration = _clearGeneration; + } + + return newConnection; + }, + 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(); + }); + } + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) + { + // Enter the blocking period error state on creation failure if configured. + // FR-006, FR-007. + if (IsBlockingPeriodEnabled()) { - connection.ClearGeneration = _clearGeneration; + EnterErrorState(ex); } - return connection; - }, - cleanupCallback: (newConnection) => + throw; + } + + // A successful creation clears any prior error state and resets backoff. FR-009. + if (connection is not null && _errorOccurred) { - // 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(); - }); + ClearErrorState(); + } + + return connection; + } + finally + { + // Disposing the lease wakes the next FIFO waiter. FR-004. + lease.Dispose(); + } + } + + /// + /// Determines whether the blocking period is enabled for this pool based on the configured + /// and the target data source. + /// + private bool IsBlockingPeriodEnabled() + { + var poolGroupConnectionOptions = PoolGroup?.ConnectionOptions; + if (poolGroupConnectionOptions is null) + { + return true; + } + + switch (poolGroupConnectionOptions.PoolBlockingPeriod) + { + case PoolBlockingPeriod.Auto: + return !ADP.IsAzureSqlServerEndpoint(poolGroupConnectionOptions.DataSource); + case PoolBlockingPeriod.AlwaysBlock: + return true; + case PoolBlockingPeriod.NeverBlock: + return false; + default: + Debug.Fail("Unknown PoolBlockingPeriod. Please specify explicit results in above switch case statement."); + return true; + } + } + + /// + /// Throws the cached error if the pool is currently in the error state. + /// + private void ThrowIfInErrorState() + { + if (!_errorOccurred) + { + return; + } + + Exception? cached = _resError; + if (cached is null) + { + return; + } + + // Clone SqlExceptions so stack traces are not shared across callers; other exception + // types are rethrown as-is. + throw cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; + } + + /// + /// Enters the pool error state, caching the supplied exception and scheduling a timer to + /// exit the state after the current backoff interval. Subsequent failures double the + /// backoff up to . + /// + private void EnterErrorState(Exception ex) + { + int waitMs; + Timer? oldTimer; + Timer newTimer; + + lock (_errorStateLock) + { + _resError = ex; + _errorOccurred = true; + waitMs = _errorWaitMs; + + newTimer = new Timer(ExitErrorStateCallback, null, Timeout.Infinite, Timeout.Infinite); + oldTimer = _errorTimer; + _errorTimer = newTimer; + + // Bump the backoff for the next failure, capped at MaxErrorWaitMs. FR-008. + _errorWaitMs = _errorWaitMs >= MaxErrorWaitMs + ? MaxErrorWaitMs + : Math.Min(_errorWaitMs * 2, MaxErrorWaitMs); + } + + oldTimer?.Dispose(); + newTimer.Change(waitMs, Timeout.Infinite); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Entering blocking period for {1}ms.", + Id, + waitMs); + } + + /// + /// Clears the cached error state and disposes the error timer. + /// + private void ClearErrorState() + { + Timer? oldTimer; + lock (_errorStateLock) + { + if (!_errorOccurred && _resError is null && _errorTimer is null && _errorWaitMs == InitialErrorWaitMs) + { + return; + } + + _errorOccurred = false; + _resError = null; + _errorWaitMs = InitialErrorWaitMs; + oldTimer = _errorTimer; + _errorTimer = null; + } + + oldTimer?.Dispose(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Error state cleared.", Id); + } + + /// + /// Timer callback that exits the error state, allowing the next caller to attempt a fresh + /// connection creation. + /// + private void ExitErrorStateCallback(object? state) + { + // Only the flag and the timer reference are cleared here. The cached exception and + // current backoff are left intact so that, if the very next attempt fails, the backoff + // continues to grow rather than resetting. The cached exception and backoff are reset + // once an attempt actually succeeds (in OpenNewInternalConnectionAsync) or when the + // pool is cleared. + Timer? oldTimer; + lock (_errorStateLock) + { + _errorOccurred = false; + oldTimer = _errorTimer; + _errorTimer = null; + } + + oldTimer?.Dispose(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Exiting blocking period.", Id); } /// @@ -521,10 +794,13 @@ private async Task GetInternalConnection( connection ??= GetIdleConnection(); - // If we didn't find an idle connection, try to open a new one. - connection ??= OpenNewInternalConnection( + // If we didn't find an idle connection, try to open a new one. The async path + // also waits on the rate limiter; the sync path performs the same wait but + // blocks the current thread. + connection ??= await OpenNewInternalConnectionAsync( owningConnection, - cancellationToken); + async, + cancellationToken).ConfigureAwait(false); // If we're at max capacity and couldn't open a connection. Block on the idle channel with a // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 7c1593af14..7c9c6a2f6c 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Concurrent; using System.Data.Common; +using System.Diagnostics; using System.Threading; using System.Threading.Tasks; using System.Transactions; @@ -1081,5 +1082,174 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() Assert.NotNull(pool2); Assert.Equal(0, pool2.Count); } + + #region Rate Limiting And Blocking Period Tests + + private ChannelDbConnectionPool ConstructPoolWithOptions( + SqlConnectionFactory connectionFactory, + string connectionString, + int maxPoolSize = 4) + { + var poolGroupOptions = new DbConnectionPoolGroupOptions( + poolByIdentity: false, + minPoolSize: 0, + maxPoolSize: maxPoolSize, + creationTimeout: 15, + loadBalanceTimeout: 0, + hasTransactionAffinity: true); + var dbConnectionPoolGroup = new DbConnectionPoolGroup( + new SqlConnectionOptions(connectionString), + new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), + poolGroupOptions); + return new ChannelDbConnectionPool( + connectionFactory, + dbConnectionPoolGroup, + DbConnectionPoolIdentity.NoIdentity, + new DbConnectionPoolProviderInfo()); + } + + [Fact] + public void ErrorOccurred_FailureWithBlockingEnabled_BecomesTrue() + { + // Default PoolBlockingPeriod is Auto; localhost is non-Azure so blocking is enabled. + var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); + + Assert.False(pool.ErrorOccurred); + + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + + Assert.True(pool.ErrorOccurred); + } + + [Fact] + public void ErrorOccurred_FailureWithNeverBlock_StaysFalse() + { + var pool = ConstructPoolWithOptions( + TimeoutConnectionFactory, + "Data Source=localhost;Pool Blocking Period=NeverBlock;"); + + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + + // FR-007: NeverBlock must not enter the error state. + Assert.False(pool.ErrorOccurred); + } + + [Fact] + public void ErrorOccurred_FailureWithAlwaysBlock_BecomesTrue() + { + var pool = ConstructPoolWithOptions( + TimeoutConnectionFactory, + "Data Source=localhost;Pool Blocking Period=AlwaysBlock;"); + + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + + Assert.True(pool.ErrorOccurred); + } + + [Fact] + public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() + { + var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); + + var first = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + Assert.True(pool.ErrorOccurred); + + // FR-006: subsequent requests inside the blocking window must fail fast with the + // cached exception. We assert it returns very quickly compared to a fresh attempt. + var sw = Stopwatch.StartNew(); + var second = Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + sw.Stop(); + + Assert.Equal(first.Message, second.Message); + Assert.True(sw.ElapsedMilliseconds < 1000, + $"Expected fast-fail (<1000ms) while in blocking period, took {sw.ElapsedMilliseconds}ms."); + } + + [Fact] + public void Clear_InErrorState_ResetsErrorOccurred() + { + var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); + + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + Assert.True(pool.ErrorOccurred); + + // FR-011: Clear must reset the error state. + pool.Clear(); + Assert.False(pool.ErrorOccurred); + } + + [Fact] + public void SuccessfulCreate_AfterFailure_ClearsErrorState() + { + var factory = new ToggleFailureConnectionFactory(); + var pool = ConstructPoolWithOptions(factory, "Data Source=localhost;"); + + // First call fails and enters the error state. + factory.FailNextCreate = true; + Assert.Throws(() => + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, 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; + var completed = pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out var conn); + Assert.True(completed); + Assert.NotNull(conn); + Assert.False(pool.ErrorOccurred); + } + + [Fact] + 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 pool = ConstructPoolWithOptions( + TimeoutConnectionFactory, + "Data Source=localhost;Pool Blocking Period=NeverBlock;", + maxPoolSize: 4); + + for (int i = 0; i < 8; i++) + { + await Assert.ThrowsAsync(async () => + { + var tcs = new TaskCompletionSource(); + pool.TryGetConnection(new SqlConnection(), tcs, out _); + await tcs.Task; + }); + } + } + + internal class ToggleFailureConnectionFactory : SqlConnectionFactory + { + public bool FailNextCreate { get; set; } + + protected override DbConnectionInternal CreateConnection( + SqlConnectionOptions options, + ConnectionPoolKey poolKey, + DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, + IDbConnectionPool pool, + DbConnection owningConnection) + { + if (FailNextCreate) + { + throw ADP.PooledOpenTimeout(); + } + + return new StubDbConnectionInternal(); + } + } + + #endregion } } From 2b5fff5f4e52e14d0ae857aee637faea25a6a7a5 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 19 May 2026 16:14:17 -0700 Subject: [PATCH 02/23] switch to timespan --- .../ConnectionPool/ChannelDbConnectionPool.cs | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 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 310114c285..9a2f289ddf 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 @@ -102,15 +102,15 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool private readonly RateLimiter _connectionCreationRateLimiter; /// - /// Initial backoff (in milliseconds) used when the pool enters the error state after a - /// connection creation failure. + /// Initial backoff used when the pool enters the error state after a connection creation + /// failure. /// - private const int InitialErrorWaitMs = 5_000; + private static readonly TimeSpan InitialErrorWait = TimeSpan.FromSeconds(5); /// - /// Maximum backoff (in milliseconds) for the pool error state. + /// Maximum backoff for the pool error state. /// - private const int MaxErrorWaitMs = 60_000; + private static readonly TimeSpan MaxErrorWait = TimeSpan.FromSeconds(60); /// /// True when the pool is currently in the error (blocking) state. When true, all new @@ -131,10 +131,10 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool private Timer? _errorTimer; /// - /// Current backoff interval (in milliseconds) used the next time the pool enters the error - /// state. Doubles on each failure up to . + /// Current backoff interval used the next time the pool enters the error state. Doubles on + /// each failure up to . /// - private int _errorWaitMs = InitialErrorWaitMs; + private TimeSpan _errorWait = InitialErrorWait; /// /// Guards mutations to the error state (the error timer, cached exception, and backoff). @@ -606,11 +606,11 @@ private void ThrowIfInErrorState() /// /// Enters the pool error state, caching the supplied exception and scheduling a timer to /// exit the state after the current backoff interval. Subsequent failures double the - /// backoff up to . + /// backoff up to . /// private void EnterErrorState(Exception ex) { - int waitMs; + TimeSpan wait; Timer? oldTimer; Timer newTimer; @@ -618,25 +618,24 @@ private void EnterErrorState(Exception ex) { _resError = ex; _errorOccurred = true; - waitMs = _errorWaitMs; + wait = _errorWait; - newTimer = new Timer(ExitErrorStateCallback, null, Timeout.Infinite, Timeout.Infinite); + newTimer = new Timer(ExitErrorStateCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); oldTimer = _errorTimer; _errorTimer = newTimer; - // Bump the backoff for the next failure, capped at MaxErrorWaitMs. FR-008. - _errorWaitMs = _errorWaitMs >= MaxErrorWaitMs - ? MaxErrorWaitMs - : Math.Min(_errorWaitMs * 2, MaxErrorWaitMs); + // Bump the backoff for the next failure, capped at MaxErrorWait. FR-008. + TimeSpan doubled = _errorWait + _errorWait; + _errorWait = doubled >= MaxErrorWait ? MaxErrorWait : doubled; } oldTimer?.Dispose(); - newTimer.Change(waitMs, Timeout.Infinite); + newTimer.Change(wait, Timeout.InfiniteTimeSpan); SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Entering blocking period for {1}ms.", Id, - waitMs); + (int)wait.TotalMilliseconds); } /// @@ -647,14 +646,14 @@ private void ClearErrorState() Timer? oldTimer; lock (_errorStateLock) { - if (!_errorOccurred && _resError is null && _errorTimer is null && _errorWaitMs == InitialErrorWaitMs) + if (!_errorOccurred && _resError is null && _errorTimer is null && _errorWait == InitialErrorWait) { return; } _errorOccurred = false; _resError = null; - _errorWaitMs = InitialErrorWaitMs; + _errorWait = InitialErrorWait; oldTimer = _errorTimer; _errorTimer = null; } From 53612e9b01f184ac9343f7993e74e2811d764b55 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 19 May 2026 16:31:07 -0700 Subject: [PATCH 03/23] move to encapsulated helper class --- .../ConnectionPool/ChannelDbConnectionPool.cs | 245 +++++++++--------- 1 file changed, 121 insertions(+), 124 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 9a2f289ddf..bdcfdaffa4 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 @@ -102,44 +102,10 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool private readonly RateLimiter _connectionCreationRateLimiter; /// - /// Initial backoff used when the pool enters the error state after a connection creation - /// failure. + /// Encapsulates the blocking-period error state for this pool: cached exception, exponential + /// backoff timer, and synchronization. See . /// - private static readonly TimeSpan InitialErrorWait = TimeSpan.FromSeconds(5); - - /// - /// Maximum backoff for the pool error state. - /// - private static readonly TimeSpan MaxErrorWait = TimeSpan.FromSeconds(60); - - /// - /// True when the pool is currently in the error (blocking) state. When true, all new - /// connection requests fail fast with until the error timer clears - /// the state. - /// - private volatile bool _errorOccurred; - - /// - /// The cached exception from the last failed connection creation attempt. Used to fail fast - /// while in the error state. - /// - private Exception? _resError; - - /// - /// Timer used to exit the error state after the current backoff interval elapses. - /// - private Timer? _errorTimer; - - /// - /// Current backoff interval used the next time the pool enters the error state. Doubles on - /// each failure up to . - /// - private TimeSpan _errorWait = InitialErrorWait; - - /// - /// Guards mutations to the error state (the error timer, cached exception, and backoff). - /// - private readonly object _errorStateLock = new(); + private readonly BlockingPeriodErrorState _errorState; #endregion /// @@ -162,6 +128,7 @@ internal ChannelDbConnectionPool( _connectionSlots = new(MaxPoolSize); _idleChannel = new(); + _errorState = new BlockingPeriodErrorState(_instanceId); // Limit concurrent connection creation attempts. The cap is bounded by MaxPoolSize since // we can never have more in-flight creations than the pool can hold. We use a small but @@ -196,7 +163,7 @@ public ConcurrentDictionary< public int IdleCount => _idleChannel.Count; /// - public bool ErrorOccurred => _errorOccurred; + public bool ErrorOccurred => _errorState.HasError; /// public int Id => _instanceId; @@ -240,7 +207,7 @@ public void Clear() // Clearing the pool implies the caller wants a clean slate, so abandon any cached // error state. FR-011. - ClearErrorState(); + _errorState.Clear(); Interlocked.Increment(ref _clearGeneration); @@ -450,7 +417,7 @@ public bool TryGetConnection( cancellationToken.ThrowIfCancellationRequested(); // Fast-fail in the error state. FR-006. - ThrowIfInErrorState(); + _errorState.ThrowIfActive(); // Quick (racy) capacity check: if we're at MaxPoolSize there's no point waiting for the // rate limiter -- the caller will have to block on the idle channel instead. @@ -494,7 +461,7 @@ public bool TryGetConnection( // Re-check the error state after acquiring the lease -- it may have been set while we // waited. - ThrowIfInErrorState(); + _errorState.ThrowIfActive(); DbConnectionInternal? connection; try @@ -535,16 +502,16 @@ public bool TryGetConnection( // FR-006, FR-007. if (IsBlockingPeriodEnabled()) { - EnterErrorState(ex); + _errorState.Enter(ex); } throw; } // A successful creation clears any prior error state and resets backoff. FR-009. - if (connection is not null && _errorOccurred) + if (connection is not null && _errorState.HasError) { - ClearErrorState(); + _errorState.Clear(); } return connection; @@ -583,110 +550,140 @@ private bool IsBlockingPeriodEnabled() } /// - /// Throws the cached error if the pool is currently in the error state. + /// Encapsulates the pool's blocking-period error state: cached exception, exponential + /// backoff timer, and synchronization. Kept as a private nested class so the pool's + /// connection-acquisition path remains focused on capacity/queue concerns and stays + /// decoupled from the (independent) rate limiting policy. /// - private void ThrowIfInErrorState() + private sealed class BlockingPeriodErrorState { - if (!_errorOccurred) + // Mirrors the values used by WaitHandleDbConnectionPool (5s initial, 60s cap). + private static readonly TimeSpan InitialWait = TimeSpan.FromSeconds(5); + private static readonly TimeSpan MaxWait = TimeSpan.FromSeconds(60); + + private readonly int _ownerPoolId; + private readonly object _lock = new(); + private volatile bool _hasError; + private Exception? _cachedException; + private Timer? _exitTimer; + private TimeSpan _nextWait = InitialWait; + + internal BlockingPeriodErrorState(int ownerPoolId) { - return; + _ownerPoolId = ownerPoolId; } - Exception? cached = _resError; - if (cached is null) + /// + /// True while the pool is in the blocking period. Subsequent acquisition attempts + /// should fast-fail with the cached exception. + /// + internal bool HasError => _hasError; + + /// + /// Throws the cached error if the pool is currently in the blocking period. + /// + internal void ThrowIfActive() { - return; - } + if (!_hasError) + { + return; + } - // Clone SqlExceptions so stack traces are not shared across callers; other exception - // types are rethrown as-is. - throw cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; - } + Exception? cached = _cachedException; + if (cached is null) + { + return; + } - /// - /// Enters the pool error state, caching the supplied exception and scheduling a timer to - /// exit the state after the current backoff interval. Subsequent failures double the - /// backoff up to . - /// - private void EnterErrorState(Exception ex) - { - TimeSpan wait; - Timer? oldTimer; - Timer newTimer; + // Clone SqlExceptions so stack traces are not shared across callers; other + // exception types are rethrown as-is. + throw cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; + } - lock (_errorStateLock) + /// + /// Enters the blocking period, caching the supplied exception and scheduling a timer + /// to exit the period after the current backoff interval. Subsequent failures double + /// the backoff up to . + /// + internal void Enter(Exception ex) { - _resError = ex; - _errorOccurred = true; - wait = _errorWait; + TimeSpan wait; + Timer? oldTimer; + Timer newTimer; - newTimer = new Timer(ExitErrorStateCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); - oldTimer = _errorTimer; - _errorTimer = newTimer; + lock (_lock) + { + _cachedException = ex; + _hasError = true; + wait = _nextWait; - // Bump the backoff for the next failure, capped at MaxErrorWait. FR-008. - TimeSpan doubled = _errorWait + _errorWait; - _errorWait = doubled >= MaxErrorWait ? MaxErrorWait : doubled; - } + newTimer = new Timer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + oldTimer = _exitTimer; + _exitTimer = newTimer; - oldTimer?.Dispose(); - newTimer.Change(wait, Timeout.InfiniteTimeSpan); + // Bump the backoff for the next failure, capped at MaxWait. FR-008. + TimeSpan doubled = _nextWait + _nextWait; + _nextWait = doubled >= MaxWait ? MaxWait : doubled; + } - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Entering blocking period for {1}ms.", - Id, - (int)wait.TotalMilliseconds); - } + oldTimer?.Dispose(); + newTimer.Change(wait, Timeout.InfiniteTimeSpan); - /// - /// Clears the cached error state and disposes the error timer. - /// - private void ClearErrorState() - { - Timer? oldTimer; - lock (_errorStateLock) + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Entering blocking period for {1}ms.", + _ownerPoolId, + (int)wait.TotalMilliseconds); + } + + /// + /// Clears the cached error state, disposes the exit timer, and resets the backoff to + /// its initial value. + /// + internal void Clear() { - if (!_errorOccurred && _resError is null && _errorTimer is null && _errorWait == InitialErrorWait) + Timer? oldTimer; + lock (_lock) { - return; + if (!_hasError && _cachedException is null && _exitTimer is null && _nextWait == InitialWait) + { + return; + } + + _hasError = false; + _cachedException = null; + _nextWait = InitialWait; + oldTimer = _exitTimer; + _exitTimer = null; } - _errorOccurred = false; - _resError = null; - _errorWait = InitialErrorWait; - oldTimer = _errorTimer; - _errorTimer = null; + oldTimer?.Dispose(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Error state cleared.", _ownerPoolId); } - oldTimer?.Dispose(); + /// + /// Timer callback that exits the blocking period, allowing the next caller to attempt + /// a fresh connection creation. The cached exception and current backoff are left + /// intact so that, if the very next attempt fails, the backoff continues to grow + /// rather than resetting. They are reset only on a successful creation or on + /// . + /// + private void ExitCallback(object? state) + { + Timer? oldTimer; + lock (_lock) + { + _hasError = false; + oldTimer = _exitTimer; + _exitTimer = null; + } - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Error state cleared.", Id); - } + oldTimer?.Dispose(); - /// - /// Timer callback that exits the error state, allowing the next caller to attempt a fresh - /// connection creation. - /// - private void ExitErrorStateCallback(object? state) - { - // Only the flag and the timer reference are cleared here. The cached exception and - // current backoff are left intact so that, if the very next attempt fails, the backoff - // continues to grow rather than resetting. The cached exception and backoff are reset - // once an attempt actually succeeds (in OpenNewInternalConnectionAsync) or when the - // pool is cleared. - Timer? oldTimer; - lock (_errorStateLock) - { - _errorOccurred = false; - oldTimer = _errorTimer; - _errorTimer = null; + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Exiting blocking period.", _ownerPoolId); } - - oldTimer?.Dispose(); - - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Exiting blocking period.", Id); } /// From 978dfe458ecf9a11b619f53a020999a0b628dcfe Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 16 Jun 2026 14:30:43 -0700 Subject: [PATCH 04/23] add diagrams --- specs/006-pool-rate-limiting/diagrams.md | 41 ++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 specs/006-pool-rate-limiting/diagrams.md 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 From 8b2ec413a91f715be7de62993429984b83c8bd07 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 16 Jun 2026 15:01:23 -0700 Subject: [PATCH 05/23] Refactor lease ordering. Refactor blocking period check. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 212 +++++++----------- .../ConnectionPool/DbConnectionPoolGroup.cs | 22 ++ .../ConnectionPool/NoOpAcquiredLease.cs | 45 ++++ .../WaitHandleDbConnectionPool.cs | 35 +-- 4 files changed, 149 insertions(+), 165 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/NoOpAcquiredLease.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs index fe2030e9ca..0fcdb1b51d 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 @@ -95,11 +95,12 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable private volatile int _isClearing; /// - /// Throttles the number of concurrent physical connection creation attempts to prevent login - /// storms against the database server. Callers acquire a lease before creating a connection and - /// dispose it when creation completes (success or failure). + /// 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; + private readonly RateLimiter? _connectionCreationRateLimiter; /// /// Encapsulates the blocking-period error state for this pool: cached exception, exponential @@ -130,20 +131,6 @@ internal ChannelDbConnectionPool( _idleChannel = new(); _errorState = new BlockingPeriodErrorState(_instanceId); - // Limit concurrent connection creation attempts. The cap is bounded by MaxPoolSize since - // we can never have more in-flight creations than the pool can hold. We use a small but - // non-trivial default so that callers queue (FIFO) rather than stampede the server with - // simultaneous logins. The QueueLimit is set to MaxPoolSize so we never silently reject - // a caller -- a caller that cannot acquire immediately waits until either its own - // ConnectTimeout elapses or capacity is available. - int maxConcurrentCreations = Math.Max(1, Math.Min((int)MaxPoolSize, Environment.ProcessorCount)); - _connectionCreationRateLimiter = new ConcurrencyLimiter(new ConcurrencyLimiterOptions - { - PermitLimit = maxConcurrentCreations, - QueueProcessingOrder = QueueProcessingOrder.OldestFirst, - QueueLimit = (int)MaxPoolSize, - }); - // Pruning is only useful when the pool can grow beyond MinPoolSize. // If min >= max, the pool is fixed-size and pruning would never activate. if (MinPoolSize < MaxPoolSize) @@ -418,17 +405,18 @@ public bool TryGetConnection( /// Opens a new internal connection to the database, throttled by the pool's rate limiter. /// /// The owning connection. - /// Whether the call is in an async context. /// 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. - /// The new internal connection, or null if the pool has no available slot. + /// 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. /// - private async Task OpenNewInternalConnectionAsync( + private DbConnectionInternal? OpenNewInternalConnection( DbConnection? owningConnection, - bool async, CancellationToken cancellationToken, TimeoutTimer timeout) { @@ -437,56 +425,38 @@ public bool TryGetConnection( // Fast-fail in the error state. FR-006. _errorState.ThrowIfActive(); - // Quick (racy) capacity check: if we're at MaxPoolSize there's no point waiting for the - // rate limiter -- the caller will have to block on the idle channel instead. - if (_connectionSlots.ReservationCount >= MaxPoolSize) - { - return null; - } - - // Throttle concurrent creation attempts. Time spent waiting here counts against the - // caller's ConnectTimeout because the cancellation token already encodes the timeout. - // FR-001, FR-002, FR-003. - RateLimitLease lease; - if (async) - { - lease = await _connectionCreationRateLimiter.AcquireAsync(1, cancellationToken).ConfigureAwait(false); - } - else - { - // The lease acquisition completes synchronously when capacity is available and only - // blocks (queues) otherwise. We rely on the surrounding sync-over-async semantics in - // the caller to bound thread pool usage on the sync path. - lease = _connectionCreationRateLimiter.AcquireAsync(1, cancellationToken) - .AsTask().GetAwaiter().GetResult(); - } - - if (!lease.IsAcquired) - { - // The rate limiter refused to admit us (e.g. the queue is full) but the - // caller's CancellationToken has not fired, so we have not actually timed out. - // Return null so the outer GetInternalConnection loop falls through to the - // idle-channel wait. That wait observes the real timeout and will wake up - // either when capacity frees (a peer create completes or a connection is - // returned) or when the timeout elapses. The loop then cycles and re-attempts - // lease acquisition. - return null; - } - - DbConnectionInternal? connection; try { - cancellationToken.ThrowIfCancellationRequested(); + // 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: () => + { + // 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). When no limiter is + // configured we substitute a no-op acquired lease. + // FR-001, FR-002, FR-003. + try + { + using RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; + + if (!lease.IsAcquired) + { + // 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; + } - // Re-check the error state after acquiring the lease -- it may have been set while we - // waited. - _errorState.ThrowIfActive(); + cancellationToken.ThrowIfCancellationRequested(); - try - { - connection = _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 @@ -507,72 +477,53 @@ public bool TryGetConnection( } return newConnection; - }, - cleanupCallback: (newConnection) => + } + finally { - // 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(); - }); - } - catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) - { - // Enter the blocking period error state on creation failure if configured. - // FR-006, FR-007. - if (IsBlockingPeriodEnabled()) + // Disposing the lease releases the permit back to the limiter (no-op + // for the default lease). After releasing, signal a waiter on the + // idle channel that they may now retry an open. We only do this if + // the pool can still grow; if we're at MaxPoolSize, only a connection + // return can satisfy a waiter. FR-004. + if (lease.IsAcquired && _connectionSlots.ReservationCount < MaxPoolSize) + { + _idleChannel.TryWrite(null); + } + } + }, + cleanupCallback: (newConnection) => { - _errorState.Enter(ex); - } + // 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(); + }); - throw; + if (connection 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(); } - } - finally - { - // Disposing the lease wakes the next FIFO waiter. FR-004. - lease.Dispose(); - } - if (connection 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 any prior error state and resets backoff. FR-009. + if (connection is not null && _errorState.HasError) + { + _errorState.Clear(); + } - // A successful creation clears any prior error state and resets backoff. FR-009. - if (connection is not null && _errorState.HasError) - { - _errorState.Clear(); + return connection; } - - return connection; - } - - /// - /// Determines whether the blocking period is enabled for this pool based on the configured - /// and the target data source. - /// - private bool IsBlockingPeriodEnabled() - { - var poolGroupConnectionOptions = PoolGroup?.ConnectionOptions; - if (poolGroupConnectionOptions is null) + catch (Exception ex) when (ADP.IsCatchableExceptionType(ex)) { - return true; - } + // Enter the blocking period error state on creation failure if configured. + // FR-006, FR-007. + if (PoolGroup.IsBlockingPeriodEnabled()) + { + _errorState.Enter(ex); + } - switch (poolGroupConnectionOptions.PoolBlockingPeriod) - { - case PoolBlockingPeriod.Auto: - return !ADP.IsAzureSqlServerEndpoint(poolGroupConnectionOptions.DataSource); - case PoolBlockingPeriod.AlwaysBlock: - return true; - case PoolBlockingPeriod.NeverBlock: - return false; - default: - Debug.Fail("Unknown PoolBlockingPeriod. Please specify explicit results in above switch case statement."); - return true; + throw; } } @@ -824,14 +775,13 @@ private async Task GetInternalConnection( connection ??= GetIdleConnection(); - // If we didn't find an idle connection, try to open a new one. The async path - // also waits on the rate limiter; the sync path performs the same wait but - // blocks the current thread. - connection ??= await OpenNewInternalConnectionAsync( + // 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, - async, cancellationToken, - timeout).ConfigureAwait(false); + timeout); // If we're at max capacity and couldn't open a connection. Block on the idle channel with a // timeout. Note that Channels guarantee fair FIFO behavior to callers of ReadAsync diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs index 8f62787e30..b71fbc48e3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs @@ -93,6 +93,28 @@ internal DbConnectionPoolGroupProviderInfo ProviderInfo internal SqlMetaDataFactory MetaDataFactory { get; set; } + /// + /// Determines whether the blocking period is enabled for this pool group based on the + /// configured and the target data source. Returns true + /// when no connection options are available so callers fail safe into the blocking + /// behavior. + /// + internal bool IsBlockingPeriodEnabled() + { + switch (_connectionOptions.PoolBlockingPeriod) + { + case PoolBlockingPeriod.Auto: + return !ADP.IsAzureSqlServerEndpoint(_connectionOptions.DataSource); + case PoolBlockingPeriod.AlwaysBlock: + return true; + case PoolBlockingPeriod.NeverBlock: + return false; + default: + Debug.Fail("Unknown PoolBlockingPeriod. Please specify explicit results in above switch case statement."); + return true; + } + } + internal int Clear() { // must be multi-thread safe with competing calls by Clear and Prune via background thread 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/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index ecf8840ebb..f7e039f763 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -489,39 +489,6 @@ private Timer CreateCleanupTimer() => _cleanupWait, _cleanupWait); - private bool IsBlockingPeriodEnabled() - { - var poolGroupConnectionOptions = _connectionPoolGroup.ConnectionOptions; - if (poolGroupConnectionOptions == null) - { - return true; - } - - var policy = poolGroupConnectionOptions.PoolBlockingPeriod; - - switch (policy) - { - case PoolBlockingPeriod.Auto: - { - return !ADP.IsAzureSqlServerEndpoint(poolGroupConnectionOptions.DataSource); - } - case PoolBlockingPeriod.AlwaysBlock: - { - return true; //Enabled - } - case PoolBlockingPeriod.NeverBlock: - { - return false; //Disabled - } - default: - { - //we should never get into this path. - Debug.Fail("Unknown PoolBlockingPeriod. Please specify explicit results in above switch case statement."); - return true; - } - } - } - private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectionInternal oldConnection, TimeoutTimer timeout) { DbConnectionInternal newObj = null; @@ -554,7 +521,7 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio { ADP.TraceExceptionWithoutRethrow(e); - if (!IsBlockingPeriodEnabled()) + if (!_connectionPoolGroup.IsBlockingPeriodEnabled()) { throw; } From 336da1a39c8d146165b0e3a14b08bb1cb30ae0d3 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 16 Jun 2026 15:03:51 -0700 Subject: [PATCH 06/23] Move blocking period error state to different file. --- .../BlockingPeriodErrorState.cs | 148 ++++++++++++++++++ .../ConnectionPool/ChannelDbConnectionPool.cs | 137 ---------------- 2 files changed, 148 insertions(+), 137 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs new file mode 100644 index 0000000000..0d4a5205dc --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs @@ -0,0 +1,148 @@ +// 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.Threading; + +#nullable enable + +namespace Microsoft.Data.SqlClient.ConnectionPool +{ + /// + /// Encapsulates a connection pool's blocking-period error state: cached exception, exponential + /// backoff timer, and synchronization. Kept as a separate class so the pool's + /// connection-acquisition path remains focused on capacity/queue concerns and stays + /// decoupled from the (independent) rate limiting policy. + /// + internal sealed class BlockingPeriodErrorState + { + // Mirrors the values used by WaitHandleDbConnectionPool (5s initial, 60s cap). + private static readonly TimeSpan InitialWait = TimeSpan.FromSeconds(5); + private static readonly TimeSpan MaxWait = TimeSpan.FromSeconds(60); + + private readonly int _ownerPoolId; + private readonly object _lock = new(); + private volatile bool _hasError; + private Exception? _cachedException; + private Timer? _exitTimer; + private TimeSpan _nextWait = InitialWait; + + internal BlockingPeriodErrorState(int ownerPoolId) + { + _ownerPoolId = ownerPoolId; + } + + /// + /// True while the pool is in the blocking period. Subsequent acquisition attempts + /// should fast-fail with the cached exception. + /// + internal bool HasError => _hasError; + + /// + /// Throws the cached error if the pool is currently in the blocking period. + /// + internal void ThrowIfActive() + { + if (!_hasError) + { + return; + } + + Exception? cached = _cachedException; + if (cached is null) + { + return; + } + + // Clone SqlExceptions so stack traces are not shared across callers; other + // exception types are rethrown as-is. + throw cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; + } + + /// + /// Enters the blocking period, caching the supplied exception and scheduling a timer + /// to exit the period after the current backoff interval. Subsequent failures double + /// the backoff up to . + /// + internal void Enter(Exception ex) + { + TimeSpan wait; + Timer? oldTimer; + Timer newTimer; + + lock (_lock) + { + _cachedException = ex; + _hasError = true; + wait = _nextWait; + + newTimer = new Timer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + oldTimer = _exitTimer; + _exitTimer = newTimer; + + // Bump the backoff for the next failure, capped at MaxWait. FR-008. + TimeSpan doubled = _nextWait + _nextWait; + _nextWait = doubled >= MaxWait ? MaxWait : doubled; + } + + oldTimer?.Dispose(); + newTimer.Change(wait, Timeout.InfiniteTimeSpan); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Entering blocking period for {1}ms.", + _ownerPoolId, + (int)wait.TotalMilliseconds); + } + + /// + /// Clears the cached error state, disposes the exit timer, and resets the backoff to + /// its initial value. + /// + internal void Clear() + { + Timer? oldTimer; + lock (_lock) + { + if (!_hasError && _cachedException is null && _exitTimer is null && _nextWait == InitialWait) + { + return; + } + + _hasError = false; + _cachedException = null; + _nextWait = InitialWait; + oldTimer = _exitTimer; + _exitTimer = null; + } + + oldTimer?.Dispose(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Error state cleared.", _ownerPoolId); + } + + /// + /// Timer callback that exits the blocking period, allowing the next caller to attempt + /// a fresh connection creation. The cached exception and current backoff are left + /// intact so that, if the very next attempt fails, the backoff continues to grow + /// rather than resetting. They are reset only on a successful creation or on + /// . + /// + private void ExitCallback(object? state) + { + Timer? oldTimer; + lock (_lock) + { + _hasError = false; + oldTimer = _exitTimer; + _exitTimer = null; + } + + oldTimer?.Dispose(); + + SqlClientEventSource.Log.TryPoolerTraceEvent( + " {0}, Exiting blocking period.", _ownerPoolId); + } + } +} 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 0fcdb1b51d..ec5ede0666 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 @@ -527,143 +527,6 @@ public bool TryGetConnection( } } - /// - /// Encapsulates the pool's blocking-period error state: cached exception, exponential - /// backoff timer, and synchronization. Kept as a private nested class so the pool's - /// connection-acquisition path remains focused on capacity/queue concerns and stays - /// decoupled from the (independent) rate limiting policy. - /// - private sealed class BlockingPeriodErrorState - { - // Mirrors the values used by WaitHandleDbConnectionPool (5s initial, 60s cap). - private static readonly TimeSpan InitialWait = TimeSpan.FromSeconds(5); - private static readonly TimeSpan MaxWait = TimeSpan.FromSeconds(60); - - private readonly int _ownerPoolId; - private readonly object _lock = new(); - private volatile bool _hasError; - private Exception? _cachedException; - private Timer? _exitTimer; - private TimeSpan _nextWait = InitialWait; - - internal BlockingPeriodErrorState(int ownerPoolId) - { - _ownerPoolId = ownerPoolId; - } - - /// - /// True while the pool is in the blocking period. Subsequent acquisition attempts - /// should fast-fail with the cached exception. - /// - internal bool HasError => _hasError; - - /// - /// Throws the cached error if the pool is currently in the blocking period. - /// - internal void ThrowIfActive() - { - if (!_hasError) - { - return; - } - - Exception? cached = _cachedException; - if (cached is null) - { - return; - } - - // Clone SqlExceptions so stack traces are not shared across callers; other - // exception types are rethrown as-is. - throw cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; - } - - /// - /// Enters the blocking period, caching the supplied exception and scheduling a timer - /// to exit the period after the current backoff interval. Subsequent failures double - /// the backoff up to . - /// - internal void Enter(Exception ex) - { - TimeSpan wait; - Timer? oldTimer; - Timer newTimer; - - lock (_lock) - { - _cachedException = ex; - _hasError = true; - wait = _nextWait; - - newTimer = new Timer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); - oldTimer = _exitTimer; - _exitTimer = newTimer; - - // Bump the backoff for the next failure, capped at MaxWait. FR-008. - TimeSpan doubled = _nextWait + _nextWait; - _nextWait = doubled >= MaxWait ? MaxWait : doubled; - } - - oldTimer?.Dispose(); - newTimer.Change(wait, Timeout.InfiniteTimeSpan); - - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Entering blocking period for {1}ms.", - _ownerPoolId, - (int)wait.TotalMilliseconds); - } - - /// - /// Clears the cached error state, disposes the exit timer, and resets the backoff to - /// its initial value. - /// - internal void Clear() - { - Timer? oldTimer; - lock (_lock) - { - if (!_hasError && _cachedException is null && _exitTimer is null && _nextWait == InitialWait) - { - return; - } - - _hasError = false; - _cachedException = null; - _nextWait = InitialWait; - oldTimer = _exitTimer; - _exitTimer = null; - } - - oldTimer?.Dispose(); - - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Error state cleared.", _ownerPoolId); - } - - /// - /// Timer callback that exits the blocking period, allowing the next caller to attempt - /// a fresh connection creation. The cached exception and current backoff are left - /// intact so that, if the very next attempt fails, the backoff continues to grow - /// rather than resetting. They are reset only on a successful creation or on - /// . - /// - private void ExitCallback(object? state) - { - Timer? oldTimer; - lock (_lock) - { - _hasError = false; - oldTimer = _exitTimer; - _exitTimer = null; - } - - oldTimer?.Dispose(); - - SqlClientEventSource.Log.TryPoolerTraceEvent( - " {0}, Exiting blocking period.", _ownerPoolId); - } - } - /// /// Checks that the provided connection is live and unexpired and closes it if needed. /// From ebc3f4932082140525e39b8cabba748a1337530c Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 16 Jun 2026 15:18:07 -0700 Subject: [PATCH 07/23] Refactor WaitHandleDbConnectionPool to use encapsulated error state class. --- .../BlockingPeriodErrorState.cs | 32 ++++++++- .../WaitHandleDbConnectionPool.cs | 71 ++++--------------- 2 files changed, 46 insertions(+), 57 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs index 0d4a5205dc..3fa2dece53 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs @@ -22,15 +22,28 @@ internal sealed class BlockingPeriodErrorState private static readonly TimeSpan MaxWait = TimeSpan.FromSeconds(60); private readonly int _ownerPoolId; + private readonly Action? _onEnter; + private readonly Action? _onExit; private readonly object _lock = new(); private volatile bool _hasError; private Exception? _cachedException; private Timer? _exitTimer; private TimeSpan _nextWait = InitialWait; - internal BlockingPeriodErrorState(int ownerPoolId) + /// + /// Creates a new instance. + /// + /// Identifier of the owning pool, used in trace events. + /// Optional callback invoked (outside the internal lock) after the + /// state transitions into the blocking period. Used by the legacy wait-handle pool to + /// signal its error wait handle. + /// Optional callback invoked (outside the internal lock) after the + /// state transitions out of the blocking period via the exit timer or . + internal BlockingPeriodErrorState(int ownerPoolId, Action? onEnter = null, Action? onExit = null) { _ownerPoolId = ownerPoolId; + _onEnter = onEnter; + _onExit = onExit; } /// @@ -39,6 +52,17 @@ internal BlockingPeriodErrorState(int ownerPoolId) /// internal bool HasError => _hasError; + /// + /// Returns a throwable copy of the cached exception, cloning + /// instances so stack traces are not shared across callers. Returns null if no + /// exception is currently cached. + /// + internal Exception? CloneCachedException() + { + Exception? cached = _cachedException; + return cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; + } + /// /// Throws the cached error if the pool is currently in the blocking period. /// @@ -89,6 +113,8 @@ internal void Enter(Exception ex) oldTimer?.Dispose(); newTimer.Change(wait, Timeout.InfiniteTimeSpan); + _onEnter?.Invoke(); + SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Entering blocking period for {1}ms.", _ownerPoolId, @@ -118,6 +144,8 @@ internal void Clear() oldTimer?.Dispose(); + _onExit?.Invoke(); + SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Error state cleared.", _ownerPoolId); } @@ -141,6 +169,8 @@ private void ExitCallback(object? state) oldTimer?.Dispose(); + _onExit?.Invoke(); + SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Exiting blocking period.", _ownerPoolId); } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index f7e039f763..3567b9be7f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -29,7 +29,7 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// Transaction-Aware Pooling: Tracks connections enlisted in using TransactedConnectionPool and TransactedConnectionList, ensuring proper context reuse. /// Concurrency and Synchronization: Uses wait handles and semaphores via PoolWaitHandles to coordinate safe multi-threaded access. /// Connection Lifecycle Management: Manages creation (CreateObject), deactivation (DeactivateObject), destruction (DestroyObject), and reclamation (ReclaimEmancipatedObjects) of internal connections. - /// Error Handling and Resilience: Implements retry and exponential backoff in TryGetConnection and handles transient errors using _errorWait. + /// Error Handling and Resilience: Implements retry and exponential backoff in TryGetConnection and delegates blocking-period bookkeeping (cached exception, exit timer) to . /// Minimum Pool Size Enforcement: Maintains the MinPoolSize by spawning background tasks to create new connections when needed. /// Load Balancing Support: Honors LoadBalanceTimeout to clean up idle connections and distribute load evenly. /// Telemetry and Tracing: Uses SqlClientEventSource for extensive diagnostic tracing of connection lifecycle events. @@ -165,8 +165,6 @@ public void Dispose() private const int WAIT_ABANDONED = 0x80; - private const int ERROR_WAIT_DEFAULT = 5 * 1000; // 5 seconds - // we do want a testable, repeatable set of generated random numbers private static readonly Random s_random = new Random(5101977); // Value obtained from Dave Driver @@ -194,11 +192,7 @@ public void Dispose() private int _waitCount; private readonly PoolWaitHandles _waitHandles; - private Exception _resError; - private volatile bool _errorOccurred; - - private int _errorWait; - private Timer _errorTimer; + private readonly BlockingPeriodErrorState _errorState; private Timer _cleanupTimer; @@ -235,8 +229,12 @@ internal WaitHandleDbConnectionPool( _waitHandles = new PoolWaitHandles(); - _errorWait = ERROR_WAIT_DEFAULT; - _errorTimer = null; // No error yet. + // Hook the wait-handle event so any thread blocked in WaitAny over the pool's + // handles wakes up immediately when the blocking period is entered/exited. + _errorState = new BlockingPeriodErrorState( + Id, + onEnter: () => _waitHandles.ErrorEvent.Set(), + onExit: () => _waitHandles.ErrorEvent.Reset()); _objectList = new List(MaxPoolSize); @@ -266,7 +264,7 @@ private int CreationTimeout public SqlConnectionFactory ConnectionFactory => _connectionFactory; - public bool ErrorOccurred => _errorOccurred; + public bool ErrorOccurred => _errorState.HasError; private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity; @@ -514,8 +512,8 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Connection {1}, Added to pool.", Id, newObj?.ObjectID); - // Reset the error wait: - _errorWait = ERROR_WAIT_DEFAULT; + // A successful creation clears any prior error state and resets backoff. + _errorState.Clear(); } catch (Exception e) when (ADP.IsCatchableExceptionType(e)) { @@ -534,33 +532,10 @@ private DbConnectionInternal CreateObject(DbConnection owningObject, DbConnectio newObj = null; // set to null, so we do not return bad new object - // Failed to create instance - _resError = e; - - // Make sure the timer starts even if ThreadAbort occurs after setting the ErrorEvent. - Timer t = new Timer(new TimerCallback(this.ErrorCallback), null, Timeout.Infinite, Timeout.Infinite); - - bool timerIsNotDisposed; - - _waitHandles.ErrorEvent.Set(); - _errorOccurred = true; + // Enter the blocking period: caches the exception, schedules the exit timer, + // and signals the wait-handle error event via the onEnter callback. + _errorState.Enter(e); - // Enable the timer. - // Note that the timer is created to allow periodic invocation. If ThreadAbort occurs in the middle of ErrorCallback, - // the timer will restart. Otherwise, the timer callback (ErrorCallback) destroys the timer after resetting the error to avoid second callback. - _errorTimer = t; - timerIsNotDisposed = t.Change(_errorWait, _errorWait); - - Debug.Assert(timerIsNotDisposed, "ErrorCallback timer has been disposed"); - - if (30000 < _errorWait) - { - _errorWait = 60000; - } - else - { - _errorWait *= 2; - } throw; } return newObj; @@ -729,27 +704,11 @@ private void DestroyObject(DbConnectionInternal obj) } } - private void ErrorCallback(object state) - { - SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Resetting Error handling.", Id); - _errorOccurred = false; - _waitHandles.ErrorEvent.Reset(); - - // the error state is cleaned, destroy the timer to avoid periodic invocation - Timer t = _errorTimer; - _errorTimer = null; - if (t != null) - { - t.Dispose(); // Cancel timer request. - } - } - - private Exception TryCloneCachedException() // Cached exception can be of any type, so is not always cloneable. // This functions clones SqlException // OleDb and Odbc connections are not passing throw this code - => _resError is SqlException sqlEx ? sqlEx.InternalClone() : _resError; + => _errorState.CloneCachedException(); private void WaitForPendingOpen() { From e03324ff7e48927e5fda64aebf9f8db033a2ab5f Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 16 Jun 2026 15:28:11 -0700 Subject: [PATCH 08/23] Refactor exception caching. --- .../BlockingPeriodErrorState.cs | 40 ++++++------------- .../WaitHandleDbConnectionPool.cs | 38 ++++++++---------- 2 files changed, 28 insertions(+), 50 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs index 3fa2dece53..e0165ae55b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs @@ -25,8 +25,10 @@ internal sealed class BlockingPeriodErrorState private readonly Action? _onEnter; private readonly Action? _onExit; private readonly object _lock = new(); - private volatile bool _hasError; - private Exception? _cachedException; + // Non-null while the pool is in the blocking period. Doubles as the "has error" + // flag, so callers don't need a separate bool. Volatile so other threads observe + // entry/exit transitions without acquiring _lock. + private volatile Exception? _cachedException; private Timer? _exitTimer; private TimeSpan _nextWait = InitialWait; @@ -50,29 +52,13 @@ internal BlockingPeriodErrorState(int ownerPoolId, Action? onEnter = null, Actio /// True while the pool is in the blocking period. Subsequent acquisition attempts /// should fast-fail with the cached exception. /// - internal bool HasError => _hasError; - - /// - /// Returns a throwable copy of the cached exception, cloning - /// instances so stack traces are not shared across callers. Returns null if no - /// exception is currently cached. - /// - internal Exception? CloneCachedException() - { - Exception? cached = _cachedException; - return cached is SqlException sqlEx ? sqlEx.InternalClone() : cached; - } + internal bool HasError => _cachedException is not null; /// /// Throws the cached error if the pool is currently in the blocking period. /// internal void ThrowIfActive() { - if (!_hasError) - { - return; - } - Exception? cached = _cachedException; if (cached is null) { @@ -98,7 +84,6 @@ internal void Enter(Exception ex) lock (_lock) { _cachedException = ex; - _hasError = true; wait = _nextWait; newTimer = new Timer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); @@ -130,12 +115,11 @@ internal void Clear() Timer? oldTimer; lock (_lock) { - if (!_hasError && _cachedException is null && _exitTimer is null && _nextWait == InitialWait) + if (_cachedException is null && _exitTimer is null && _nextWait == InitialWait) { return; } - _hasError = false; _cachedException = null; _nextWait = InitialWait; oldTimer = _exitTimer; @@ -151,18 +135,18 @@ internal void Clear() } /// - /// Timer callback that exits the blocking period, allowing the next caller to attempt - /// a fresh connection creation. The cached exception and current backoff are left - /// intact so that, if the very next attempt fails, the backoff continues to grow - /// rather than resetting. They are reset only on a successful creation or on - /// . + /// Timer callback that exits the blocking period by clearing the cached exception, + /// allowing the next caller to attempt a fresh connection creation. The current + /// backoff is left intact so that, if the next attempt fails, the backoff continues + /// to grow rather than resetting. The backoff is reset only on a successful creation + /// or on . /// private void ExitCallback(object? state) { Timer? oldTimer; lock (_lock) { - _hasError = false; + _cachedException = null; oldTimer = _exitTimer; _exitTimer = null; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs index 3567b9be7f..146bbca363 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs @@ -704,12 +704,6 @@ private void DestroyObject(DbConnectionInternal obj) } } - private Exception TryCloneCachedException() - // Cached exception can be of any type, so is not always cloneable. - // This functions clones SqlException - // OleDb and Odbc connections are not passing throw this code - => _errorState.CloneCachedException(); - private void WaitForPendingOpen() { Debug.Assert(!Thread.CurrentThread.IsThreadPoolThread, "This thread may block for a long time. Threadpool threads should not be used."); @@ -933,7 +927,12 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj // Throw the error that PoolCreateRequest stashed. SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Errors are set.", Id); Interlocked.Decrement(ref _waitCount); - throw TryCloneCachedException(); + _errorState.ThrowIfActive(); + // Narrow race: error state cleared between WaitAny observing + // the signal and this check. Re-balance _waitCount and let the + // outer do/while loop retry. + Interlocked.Increment(ref _waitCount); + break; case CREATION_HANDLE: SqlClientEventSource.Log.TryPoolerTraceEvent(" {0}, Creating new connection.", Id); @@ -1481,25 +1480,20 @@ private DbConnectionInternal UserCreateRequest(DbConnection owningObject, Timeou // instead obtained creation mutex DbConnectionInternal obj = null; - if (ErrorOccurred) - { - throw TryCloneCachedException(); - } - else + _errorState.ThrowIfActive(); + + if ((oldConnection != null) || (Count < MaxPoolSize) || (0 == MaxPoolSize)) { - if ((oldConnection != null) || (Count < MaxPoolSize) || (0 == MaxPoolSize)) - { - // If we have an odd number of total objects, reclaim any dead objects. - // If we did not find any objects to reclaim, create a new one. + // If we have an odd number of total objects, reclaim any dead objects. + // If we did not find any objects to reclaim, create a new one. - // TODO: Consider implement a control knob here; why do we only check for dead objects ever other time? why not every 10th time or every time? - if ((oldConnection != null) || (Count & 0x1) == 0x1 || !ReclaimEmancipatedObjects()) - { - obj = CreateObject(owningObject, oldConnection, timeout); - } + // TODO: Consider implement a control knob here; why do we only check for dead objects ever other time? why not every 10th time or every time? + if ((oldConnection != null) || (Count & 0x1) == 0x1 || !ReclaimEmancipatedObjects()) + { + obj = CreateObject(owningObject, oldConnection, timeout); } - return obj; } + return obj; } } } From 98cbd4ba2d7db5054b155b8490e46735c6df8cb1 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 16 Jun 2026 15:39:31 -0700 Subject: [PATCH 09/23] Fix test file --- .../UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index bef7fd52e8..27b01d64f7 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -1296,6 +1296,9 @@ protected override DbConnectionInternal CreateConnection( return new StubDbConnectionInternal(); } + } + + #endregion #region Connection Timeout Awareness Tests @@ -1423,7 +1426,4 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } - - #endregion - } } From 746617ce971fc2389048c105987b767f4752f34b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 18 Jun 2026 14:51:36 -0700 Subject: [PATCH 10/23] Fix compilation --- .../ConnectionPool/ChannelDbConnectionPool.cs | 5 +++-- .../ChannelDbConnectionPoolTest.cs | 22 ++++++++++--------- 2 files changed, 15 insertions(+), 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 ec5ede0666..852538c405 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 @@ -100,7 +100,9 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// 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. /// +#pragma warning disable CS0649 // Field is never assigned — rate-limiter wiring is in-progress (FR-001) private readonly RateLimiter? _connectionCreationRateLimiter; +#pragma warning restore CS0649 /// /// Encapsulates the blocking-period error state for this pool: cached exception, exponential @@ -443,10 +445,9 @@ public bool TryGetConnection( // its rate-limit lease (see finally below). When no limiter is // configured we substitute a no-op acquired lease. // FR-001, FR-002, FR-003. + using RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; try { - using RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; - if (!lease.IsAcquired) { // TODO: When we fail to acquire a lease, surface the lease metadata diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 27b01d64f7..a72b1e34f3 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -1165,7 +1165,7 @@ public void ErrorOccurred_FailureWithBlockingEnabled_BecomesTrue() Assert.False(pool.ErrorOccurred); Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred); } @@ -1178,7 +1178,7 @@ public void ErrorOccurred_FailureWithNeverBlock_StaysFalse() "Data Source=localhost;Pool Blocking Period=NeverBlock;"); Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); // FR-007: NeverBlock must not enter the error state. Assert.False(pool.ErrorOccurred); @@ -1192,7 +1192,7 @@ public void ErrorOccurred_FailureWithAlwaysBlock_BecomesTrue() "Data Source=localhost;Pool Blocking Period=AlwaysBlock;"); Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred); } @@ -1203,14 +1203,14 @@ public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); var first = Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred); // FR-006: subsequent requests inside the blocking window must fail fast with the // cached exception. We assert it returns very quickly compared to a fresh attempt. var sw = Stopwatch.StartNew(); var second = Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); sw.Stop(); Assert.Equal(first.Message, second.Message); @@ -1224,7 +1224,7 @@ public void Clear_InErrorState_ResetsErrorOccurred() var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred); // FR-011: Clear must reset the error state. @@ -1241,7 +1241,7 @@ public void SuccessfulCreate_AfterFailure_ClearsErrorState() // First call fails and enters the error state. factory.FailNextCreate = true; Assert.Throws(() => - pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out _)); + 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 @@ -1250,7 +1250,7 @@ public void SuccessfulCreate_AfterFailure_ClearsErrorState() Assert.False(pool.ErrorOccurred); factory.FailNextCreate = false; - var completed = pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, out var conn); + var completed = pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out var conn); Assert.True(completed); Assert.NotNull(conn); Assert.False(pool.ErrorOccurred); @@ -1272,7 +1272,7 @@ public async Task RateLimiter_LeaseDisposedOnFailure_DoesNotStarvePool() await Assert.ThrowsAsync(async () => { var tcs = new TaskCompletionSource(); - pool.TryGetConnection(new SqlConnection(), tcs, out _); + pool.TryGetConnection(new SqlConnection(), tcs, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _); await tcs.Task; }); } @@ -1287,7 +1287,8 @@ protected override DbConnectionInternal CreateConnection( ConnectionPoolKey poolKey, DbConnectionPoolGroupProviderInfo poolGroupProviderInfo, IDbConnectionPool pool, - DbConnection owningConnection) + DbConnection owningConnection, + TimeoutTimer timeout) { if (FailNextCreate) { @@ -1427,3 +1428,4 @@ public void GetConnection_TimeoutTimerReflectsPoolWaitTime() #endregion } } + From 5b04967feedfd7766dd71b946565829e1fb32095 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 18 Jun 2026 15:15:28 -0700 Subject: [PATCH 11/23] Improve copilot testing instructions. --- .github/copilot-instructions.md | 18 +++++++ .github/instructions/testing.instructions.md | 51 ++++++++++++++------ 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index c0bbc25457..b0b7a88dde 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -150,6 +150,24 @@ When a new issue is created, follow these steps: - Do not modify the `CODEOWNERS` file directly. - Do not modify `CHANGELOG.md` unless executing a release workflow (see `release-notes` prompt). - Do not close issues without a fix or without providing a clear reason. +- **Check `.github/instructions/` for matching `applyTo` patterns BEFORE starting any file creation or modification task.** Apply all conventions from those files from the start—do not refactor later. + +## Instruction File Lookup (Required Workflow) + +Before creating or modifying any file, check if matching instruction files exist: + +1. Scan `.github/instructions/` for files with `applyTo` patterns that match your file path +2. Read **all** matching instruction files completely before writing code +3. Apply **all** conventions, patterns, and requirements from the start +4. Do not skip instructions or plan to refactor later + +**Example:** Creating `src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/NewTest.cs` +- File matches `applyTo: "**/tests/**,**/*Test*.cs"` from `testing.instructions.md` +- Must read `testing.instructions.md` before writing the first test method +- Must apply AAA pattern, XML doc comments, and all testing conventions from the start +- Do not create tests first and add documentation later + +This ensures consistency across the codebase and prevents rework. ## Terminal Execution Safety - Treat any non-zero shell exit code as a failed step that requires correction before proceeding. diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md index df6b909f8a..025d0fe5f1 100644 --- a/.github/instructions/testing.instructions.md +++ b/.github/instructions/testing.instructions.md @@ -189,10 +189,15 @@ class and method level. #### What the Comments Must Explain - The behavior/contract being tested (not just restating the method name). - Why the scenario matters (for example: regression guard, parsing contract, sync/async parity, - isolation requirement). + isolation requirement, thread-safety validation, edge case handling). - For helper methods, what side effects occur (for example console redirection, file system copying, process execution) and why they are needed. +#### Implementation Guidance +- Always use the **Arrange-Act-Assert (AAA)** pattern with explicit section comments (see [Test Structure](#test-structure-arrange-act-assert-aaa-pattern)). +- Keep the test body focused on a single logical assertion; avoid testing multiple unrelated behaviors. +- Extract complex setup into helper methods or fixtures rather than embedding it in the test. + #### Style Guidance - Keep comments concise and factual. - Prefer behavior-focused wording over implementation trivia. @@ -212,25 +217,41 @@ public void AppRunWithMalformedConnectionStringReturnsOneAndWritesParseError() } ``` -### Test Structure +### Test Structure: Arrange-Act-Assert (AAA) Pattern + +All test methods **must** follow the **Arrange-Act-Assert (AAA)** pattern with explicit section comments. +This pattern improves readability and maintainability by clearly delineating setup, execution, and validation. + +**Pattern:** ```csharp -public class FeatureNameTests +[Fact] +public void MethodName_Scenario_ExpectedResult() { - [Fact] - public void MethodName_Scenario_ExpectedResult() - { - // Arrange - var sut = new SystemUnderTest(); - - // Act - var result = sut.PerformAction(); - - // Assert - Assert.Equal(expected, result); - } + // Arrange + // Set up test fixtures, initial state, dependencies, and test data + var sut = new SystemUnderTest(); + var input = new TestData(); + + // Act + // Execute the code under test + var result = sut.PerformAction(input); + + // Assert + // Validate outcomes and expectations + Assert.Equal(expected, result); } ``` +**Benefits:** +- Clear visual separation of setup, execution, and validation +- Easier to identify test logic flow +- Simpler to debug failing tests (know which section failed) +- Consistent convention across codebase +- Improves AI agent comprehension and test generation + +**Guideline:** Each section should be visually distinct. If AAA sections become too large (e.g., Arrange > 20 lines), +consider extracting helper methods or fixtures rather than embedding complexity in the test method itself. + ### Naming Conventions - Test class: `{ClassName}Tests` - Test method: `{Method}_{Scenario}_{ExpectedBehavior}` From 0ee1dd7cc2dff164b3476a78bfca5121bac76925 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 18 Jun 2026 15:29:59 -0700 Subject: [PATCH 12/23] Add testing and use TimeProvider to fake time. --- .../BlockingPeriodErrorState.cs | 48 +- .../BlockingPeriodErrorStateTest.cs | 682 ++++++++++++++++++ 2 files changed, 722 insertions(+), 8 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs index e0165ae55b..dad2019fbe 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs @@ -4,6 +4,7 @@ using System; using System.Threading; +using Microsoft.Data.SqlClient.Internal; #nullable enable @@ -15,7 +16,7 @@ namespace Microsoft.Data.SqlClient.ConnectionPool /// connection-acquisition path remains focused on capacity/queue concerns and stays /// decoupled from the (independent) rate limiting policy. /// - internal sealed class BlockingPeriodErrorState + internal sealed class BlockingPeriodErrorState : IDisposable { // Mirrors the values used by WaitHandleDbConnectionPool (5s initial, 60s cap). private static readonly TimeSpan InitialWait = TimeSpan.FromSeconds(5); @@ -24,13 +25,15 @@ internal sealed class BlockingPeriodErrorState private readonly int _ownerPoolId; private readonly Action? _onEnter; private readonly Action? _onExit; + private readonly TimeProvider _timeProvider; private readonly object _lock = new(); // Non-null while the pool is in the blocking period. Doubles as the "has error" // flag, so callers don't need a separate bool. Volatile so other threads observe // entry/exit transitions without acquiring _lock. private volatile Exception? _cachedException; - private Timer? _exitTimer; + private ITimer? _exitTimer; private TimeSpan _nextWait = InitialWait; + private bool _disposed; /// /// Creates a new instance. @@ -41,11 +44,16 @@ internal sealed class BlockingPeriodErrorState /// signal its error wait handle. /// Optional callback invoked (outside the internal lock) after the /// state transitions out of the blocking period via the exit timer or . - internal BlockingPeriodErrorState(int ownerPoolId, Action? onEnter = null, Action? onExit = null) + /// The time provider used to create the exit timer. Defaults to + /// . Inject a test double (e.g. + /// Microsoft.Extensions.TimeProvider.Testing.FakeTimeProvider) in unit tests to + /// control timer scheduling deterministically. + internal BlockingPeriodErrorState(int ownerPoolId, Action? onEnter = null, Action? onExit = null, TimeProvider? timeProvider = null) { _ownerPoolId = ownerPoolId; _onEnter = onEnter; _onExit = onExit; + _timeProvider = timeProvider ?? TimeProvider.System; } /// @@ -78,15 +86,15 @@ internal void ThrowIfActive() internal void Enter(Exception ex) { TimeSpan wait; - Timer? oldTimer; - Timer newTimer; + ITimer? oldTimer; + ITimer newTimer; lock (_lock) { _cachedException = ex; wait = _nextWait; - newTimer = new Timer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + newTimer = _timeProvider.CreateTimer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); oldTimer = _exitTimer; _exitTimer = newTimer; @@ -112,7 +120,7 @@ internal void Enter(Exception ex) /// internal void Clear() { - Timer? oldTimer; + ITimer? oldTimer; lock (_lock) { if (_cachedException is null && _exitTimer is null && _nextWait == InitialWait) @@ -143,7 +151,7 @@ internal void Clear() /// private void ExitCallback(object? state) { - Timer? oldTimer; + ITimer? oldTimer; lock (_lock) { _cachedException = null; @@ -158,5 +166,29 @@ private void ExitCallback(object? state) SqlClientEventSource.Log.TryPoolerTraceEvent( " {0}, Exiting blocking period.", _ownerPoolId); } + + /// + /// Disposes the instance, releasing the exit timer if one is active. Clears the + /// error state so that any waiting callers do not observe a stale exception after + /// the owning pool is torn down. + /// + public void Dispose() + { + ITimer? timerToDispose; + lock (_lock) + { + if (_disposed) + { + return; + } + + _disposed = true; + _cachedException = null; + timerToDispose = _exitTimer; + _exitTimer = null; + } + + timerToDispose?.Dispose(); + } } } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs new file mode 100644 index 0000000000..e4a031c2e9 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs @@ -0,0 +1,682 @@ +// 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.Threading; +using Microsoft.Extensions.TimeProvider.Testing; +using Microsoft.Data.SqlClient.ConnectionPool; +using Xunit; + +#nullable enable + +namespace Microsoft.Data.SqlClient.UnitTests.ConnectionPool +{ + /// + /// Comprehensive unit tests for covering: + /// - Initial state and error caching + /// - and exception handling + /// - and state reset + /// - Exponential backoff progression (verified with ) + /// - Timer-driven exit behavior (verified with ) + /// - implementation and timer cleanup + /// - Callback invocation and re-entrancy safety + /// + public class BlockingPeriodErrorStateTest + { + #region HasError / initial state + + /// + /// Verifies that a newly constructed has + /// set to false. + /// + [Fact] + public void HasError_InitialState_IsFalse() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + + // Act & Assert + Assert.False(state.HasError); + } + + /// + /// Verifies that does not throw + /// when called on a newly constructed instance with no cached error. + /// + [Fact] + public void ThrowIfActive_InitialState_DoesNotThrow() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + + // Act & Assert + state.ThrowIfActive(); // Should complete without throwing + } + + #endregion + + #region Enter + + /// + /// Verifies that calling sets + /// to true. + /// + [Fact] + public void Enter_SetsHasErrorToTrue() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + + // Act + state.Enter(new InvalidOperationException("test")); + + // Assert + Assert.True(state.HasError); + } + + /// + /// Verifies that throws + /// the exact exception type that was cached by . + /// + [Fact] + public void Enter_ThrowIfActive_ThrowsCachedExceptionType() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + var exception = new InvalidOperationException("boom"); + + // Act + state.Enter(exception); + + // Assert + var ex = Assert.Throws(() => state.ThrowIfActive()); + Assert.Equal("boom", ex.Message); + } + + /// + /// Verifies that when a is cached, + /// throws a cloned instance rather than the original, to avoid sharing stack traces across callers. + /// + [Fact] + public void Enter_WithSqlException_ThrowsClonedInstance() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + var original = SqlExceptionHelper.CreateSqlException("connection failed"); + + // Act + state.Enter(original); + var thrown = Assert.Throws(() => state.ThrowIfActive()); + + // Assert + Assert.NotSame(original, thrown); + Assert.Equal(original.Message, thrown.Message); + } + + /// + /// Verifies that invokes the optional + /// onEnter callback after entering the blocking period. + /// + [Fact] + public void Enter_InvokesOnEnterCallback() + { + // Arrange + int callCount = 0; + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, onEnter: () => callCount++); + + // Act + state.Enter(new Exception()); + + // Assert + Assert.Equal(1, callCount); + } + + /// + /// Verifies that calling a second time + /// replaces the cached exception, invokes the callback again, and the new exception is thrown. + /// + [Fact] + public void Enter_CalledTwice_ReplacesExceptionAndInvokesOnEnterAgain() + { + // Arrange + int enterCount = 0; + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, onEnter: () => enterCount++); + + // Act + state.Enter(new InvalidOperationException("first")); + state.Enter(new ArgumentException("second")); + + // Assert + Assert.Equal(2, enterCount); + var ex = Assert.Throws(() => state.ThrowIfActive()); + Assert.Equal("second", ex.Message); + } + + /// + /// Verifies that does not invoke + /// the onExit callback (only the onEnter callback or the timer should trigger onExit). + /// + [Fact] + public void Enter_DoesNotInvokeOnExitCallback() + { + // Arrange + int exitCount = 0; + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, onExit: () => exitCount++); + + // Act + state.Enter(new Exception()); + + // Assert + Assert.Equal(0, exitCount); + } + + #endregion + + #region Clear + + /// + /// Verifies that resets + /// to false. + /// + [Fact] + public void Clear_AfterEnter_ResetsHasError() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + state.Enter(new Exception()); + + // Act + state.Clear(); + + // Assert + Assert.False(state.HasError); + } + + /// + /// Verifies that after , + /// does not throw because the cached error has been cleared. + /// + [Fact] + public void Clear_AfterEnter_ThrowIfActiveDoesNotThrow() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + state.Enter(new Exception()); + + // Act + state.Clear(); + + // Assert + state.ThrowIfActive(); // Must not throw + } + + /// + /// Verifies that invokes the optional + /// onExit callback after clearing the error state. + /// + [Fact] + public void Clear_AfterEnter_InvokesOnExitCallback() + { + // Arrange + int exitCount = 0; + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, onExit: () => exitCount++); + state.Enter(new Exception()); + + // Act + state.Clear(); + + // Assert + Assert.Equal(1, exitCount); + } + + /// + /// Verifies that on an initial (no-error) state + /// does not invoke the onExit callback because there is nothing to clear. + /// + [Fact] + public void Clear_OnInitialState_DoesNotInvokeOnExitCallback() + { + // Arrange + int exitCount = 0; + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, onExit: () => exitCount++); + + // Act + state.Clear(); + + // Assert + Assert.Equal(0, exitCount); + } + + /// + /// Verifies that is idempotent: + /// calling it a second time does not invoke the onExit callback again. + /// + [Fact] + public void Clear_CalledTwice_OnExitInvokedOnce() + { + // Arrange + int exitCount = 0; + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, onExit: () => exitCount++); + state.Enter(new Exception()); + + // Act + state.Clear(); + state.Clear(); + + // Assert + Assert.Equal(1, exitCount); + } + + /// + /// Verifies that resets the backoff + /// timeout to its initial value, so the next + /// uses the initial wait duration instead of the accumulated backoff. + /// + [Fact] + public void Clear_ResetsBackoffSoNextEnterUsesInitialWait() + { + // Arrange + using var state = new BlockingPeriodErrorState(ownerPoolId: 1); + state.Enter(new Exception("first")); + + // Act + state.Clear(); + state.Enter(new Exception("second")); + + // Assert + Assert.True(state.HasError); + } + + #endregion + + #region Backoff progression + + /// + /// Verifies that the initial enter schedules the timer with the 5-second initial wait. + /// The error state should persist until the timer fires, after which it clears automatically. + /// + [Fact] + public void Enter_FirstEntry_SchedulesInitialWaitTimer() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, timeProvider: fakeTime); + state.Enter(new Exception()); + + // Act: advance just under the initial 5s wait + fakeTime.Advance(TimeSpan.FromSeconds(4)); + + // Assert: timer has not fired yet + Assert.True(state.HasError); + + // Act: advance past the due time + fakeTime.Advance(TimeSpan.FromSeconds(1)); + + // Assert: timer has fired, error cleared + Assert.False(state.HasError); + } + + /// + /// Verifies that successive timer-driven exits double the backoff each time: + /// 5 s → 10 s → 20 s → 40 s → 60 s (capped at MaxWait). + /// Each Enter schedules the timer for the current accumulated wait and the error + /// persists until exactly that duration elapses. + /// + [Fact] + public void Enter_BackoffDoublesOnSuccessiveTimerFiredEntries() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, timeProvider: fakeTime); + + // (expectedWaitSeconds, _) — the wait used by Enter for that iteration + int[] expectedWaits = [5, 10, 20, 40, 60, 60]; + + // Act & Assert + foreach (int wait in expectedWaits) + { + state.Enter(new Exception($"attempt at wait={wait}s")); + + // One second before due time: error still active + fakeTime.Advance(TimeSpan.FromSeconds(wait - 1)); + Assert.True(state.HasError, $"HasError should be true after {wait - 1}s (scheduled wait={wait}s)"); + + // Final second: timer fires, error clears + fakeTime.Advance(TimeSpan.FromSeconds(1)); + Assert.False(state.HasError, $"HasError should be false after {wait}s (scheduled wait={wait}s)"); + } + } + + /// + /// Verifies that a timer-driven exit does NOT reset the backoff. The accumulated + /// backoff is preserved so the next failure uses the doubled wait, reflecting + /// continued instability. Only resets + /// the backoff to the initial value. In this way, we only reset the backoff when + /// a connection is successfully established. + /// + [Fact] + public void Enter_WhenTimerFires_DoesNotResetBackoff() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, timeProvider: fakeTime); + + // First enter: uses 5s wait; _nextWait advances to 10s + state.Enter(new Exception("first")); + fakeTime.Advance(TimeSpan.FromSeconds(5)); // timer fires + Assert.False(state.HasError); + + // Act: enter again — should use 10s, not the initial 5s + state.Enter(new Exception("second")); + + // Assert: not cleared after 9s + fakeTime.Advance(TimeSpan.FromSeconds(9)); + Assert.True(state.HasError); + + // Assert: cleared after the full 10s + fakeTime.Advance(TimeSpan.FromSeconds(1)); + Assert.False(state.HasError); + } + + /// + /// Verifies that resets the backoff + /// to the initial 5-second wait even after the timer has doubled it, so the next + /// enter cycle starts fresh. + /// + [Fact] + public void Clear_AfterTimerFiredEntry_ResetsBackoffToInitialWait() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, timeProvider: fakeTime); + + // First enter/timer-exit: _nextWait advances from 5s to 10s + state.Enter(new Exception("first")); + fakeTime.Advance(TimeSpan.FromSeconds(5)); + Assert.False(state.HasError); + + // Second enter to accumulate more backoff; then Clear resets it + state.Enter(new Exception("second")); // _nextWait advances to 20s + state.Clear(); // _nextWait resets to 5s + + // Act: enter again — should use the initial 5s wait + state.Enter(new Exception("third")); + + // Assert: not cleared after 4s + fakeTime.Advance(TimeSpan.FromSeconds(4)); + Assert.True(state.HasError); + + // Assert: cleared after the initial 5s + fakeTime.Advance(TimeSpan.FromSeconds(1)); + Assert.False(state.HasError); + } + + #endregion + + #region Timer behavior + + /// + /// Verifies that the timer-driven exit invokes the onExit callback, the same + /// callback path used by . + /// + [Fact] + public void Enter_WhenTimerFires_InvokesOnExitCallback() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + int exitCount = 0; + using var state = new BlockingPeriodErrorState( + ownerPoolId: 1, + onExit: () => exitCount++, + timeProvider: fakeTime); + state.Enter(new Exception()); + + // Act + fakeTime.Advance(TimeSpan.FromSeconds(5)); + + // Assert + Assert.Equal(1, exitCount); + Assert.False(state.HasError); + } + + /// + /// Verifies that the timer does not fire before its due time, confirming the + /// scheduled wait is respected and not fired early. + /// + [Fact] + public void Enter_TimerDoesNotFireBeforeDueTime() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + using var state = new BlockingPeriodErrorState(ownerPoolId: 1, timeProvider: fakeTime); + state.Enter(new Exception()); + + // Act: advance to 1ms before the 5s due time + fakeTime.Advance(TimeSpan.FromMilliseconds(4999)); + + // Assert + Assert.True(state.HasError); + } + + #endregion + + #region IDisposable + + /// + /// Verifies that on an initial state + /// does not throw and completes successfully. + /// + [Fact] + public void Dispose_OnInitialState_DoesNotThrow() + { + // Arrange & Act + var state = new BlockingPeriodErrorState(ownerPoolId: 1); + + // Assert + state.Dispose(); // Should not throw + } + + /// + /// Verifies that clears the cached + /// error state so is false after disposal. + /// + [Fact] + public void Dispose_AfterEnter_ClearsHasError() + { + // Arrange + var state = new BlockingPeriodErrorState(ownerPoolId: 1); + state.Enter(new Exception()); + + // Act + state.Dispose(); + + // Assert + Assert.False(state.HasError); + } + + /// + /// Verifies that is idempotent: + /// calling it multiple times does not throw and completes successfully. + /// + [Fact] + public void Dispose_CalledMultipleTimes_DoesNotThrow() + { + // Arrange & Act + var state = new BlockingPeriodErrorState(ownerPoolId: 1); + state.Dispose(); + + // Assert + state.Dispose(); // Must be idempotent and not throw + } + + /// + /// Verifies that does not invoke + /// the onExit callback because disposal is a resource-cleanup path, not a logical + /// "exit blocking period" event. + /// + [Fact] + public void Dispose_DoesNotInvokeOnExitCallback() + { + // Arrange + int exitCount = 0; + var state = new BlockingPeriodErrorState(ownerPoolId: 1, onExit: () => exitCount++); + state.Enter(new Exception()); + + // Act + state.Dispose(); + + // Assert + Assert.Equal(0, exitCount); + } + + /// + /// Verifies that properly releases + /// and cancels the internal exit timer, preventing stale callbacks from firing after disposal. + /// Uses to advance time deterministically past the timer's due + /// time without relying on real-time sleeps. + /// + [Fact] + public void Dispose_ReleasesTimer_NoCallbackAfterDispose() + { + // Arrange + var fakeTime = new FakeTimeProvider(); + int exitCount = 0; + var state = new BlockingPeriodErrorState( + ownerPoolId: 1, + onExit: () => Interlocked.Increment(ref exitCount), + timeProvider: fakeTime); + state.Enter(new Exception()); + + // Act: dispose cancels the pending timer + state.Dispose(); + + // Advance well past the 5s due time — the cancelled timer must not fire + fakeTime.Advance(TimeSpan.FromSeconds(60)); + + // Assert + Assert.Equal(0, exitCount); + } + + /// + /// Verifies that works correctly in a standard + /// using statement, with no exceptions thrown during disposal. + /// + [Fact] + public void Dispose_WithUsingStatement_DoesNotThrow() + { + // Arrange & Act + using (var state = new BlockingPeriodErrorState(ownerPoolId: 1)) + { + state.Enter(new Exception()); + Assert.True(state.HasError); + } + + // Assert + // No exception expected when the using block exits + } + + #endregion + + #region Callback behaviour + + /// + /// Verifies that both onEnter and onExit callbacks are optional (nullable) + /// and the instance works correctly when neither is provided. + /// + [Fact] + public void Callbacks_AreNotRequiredAndDefaultToNull() + { + // Arrange & Act + using var state = new BlockingPeriodErrorState(ownerPoolId: 42); + state.Enter(new Exception()); + + // Assert + state.Clear(); // Should work without callbacks + } + + /// + /// Verifies that the onEnter callback is invoked after the internal lock is released. + /// The callback reads and calls + /// — operations that are safe only + /// when the lock is not held. If the implementation were changed to hold the lock during + /// the callback invocation, any re-entrant call from the callback that tries to acquire the + /// same lock (on a non-re-entrant lock) would deadlock. + /// + [Fact] + public void OnEnter_CalledOutsideLock_CanCallBackIntoState() + { + // Arrange + bool hasErrorObservedInCallback = false; + using var state = new BlockingPeriodErrorState( + ownerPoolId: 1, + onEnter: () => + { + // Observe state from inside the callback. + // HasError must already be true at this point. + hasErrorObservedInCallback = state.HasError; + + // Calling ThrowIfActive from the callback must not deadlock. + Assert.Throws(() => state.ThrowIfActive()); + }); + + // Act + state.Enter(new InvalidOperationException("test")); + + // Assert + Assert.True(hasErrorObservedInCallback); + } + + /// + /// Verifies that the onExit callback is invoked after the internal lock is released. + /// The callback reads — confirming it + /// observes the cleared state — and calls + /// without deadlocking. If the implementation were changed to hold the lock during the + /// callback, any re-entrant call from the callback would deadlock on a non-re-entrant lock. + /// + [Fact] + public void OnExit_CalledOutsideLock_CanCallBackIntoState() + { + // Arrange + bool hasErrorObservedInCallback = true; // initialized to true; must be false after Clear + using var state = new BlockingPeriodErrorState( + ownerPoolId: 1, + onExit: () => + { + // HasError must already be false when onExit is called. + hasErrorObservedInCallback = state.HasError; + + // Calling ThrowIfActive from the callback must not deadlock. + state.ThrowIfActive(); // must not throw + }); + + // Act + state.Enter(new Exception()); + state.Clear(); + + // Assert + Assert.False(hasErrorObservedInCallback); + } + + #endregion + } + + /// + /// Test helper for creating instances. Since has + /// an internal constructor, instances must be created via the factory method. + /// + internal static class SqlExceptionHelper + { + /// + /// Creates a with the specified message using the internal factory method. + /// + /// The error message for the exception. + /// A new with the specified message. + internal static SqlException CreateSqlException(string message) + { + var collection = new SqlErrorCollection(); + collection.Add(new SqlError(0, (byte)0, (byte)0, "TestServer", message, "", 0)); + return SqlException.CreateException(collection, ""); + } + } +} From 7309cf7bb5851e2d1782ac5c8545e889f688b732 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 18 Jun 2026 15:59:36 -0700 Subject: [PATCH 13/23] Cleanup --- .../ConnectionPool/ChannelDbConnectionPool.cs | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 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 852538c405..9f56dcf305 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 @@ -106,9 +106,10 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// /// Encapsulates the blocking-period error state for this pool: cached exception, exponential - /// backoff timer, and synchronization. See . + /// backoff timer, and synchronization. Created only when blocking period is enabled for + /// this pool group. See . /// - private readonly BlockingPeriodErrorState _errorState; + private readonly BlockingPeriodErrorState? _errorState; #endregion /// @@ -131,7 +132,10 @@ internal ChannelDbConnectionPool( _connectionSlots = new(MaxPoolSize); _idleChannel = new(); - _errorState = new BlockingPeriodErrorState(_instanceId); + 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. @@ -159,7 +163,7 @@ public ConcurrentDictionary< public int IdleCount => _idleChannel.Count; /// - public bool ErrorOccurred => _errorState.HasError; + public bool ErrorOccurred => _errorState?.HasError ?? false; /// public int Id => _instanceId; @@ -205,7 +209,7 @@ public void Clear() // Clearing the pool implies the caller wants a clean slate, so abandon any cached // error state. FR-011. - _errorState.Clear(); + _errorState?.Clear(); Interlocked.Increment(ref _clearGeneration); @@ -425,7 +429,7 @@ public bool TryGetConnection( cancellationToken.ThrowIfCancellationRequested(); // Fast-fail in the error state. FR-006. - _errorState.ThrowIfActive(); + _errorState?.ThrowIfActive(); try { @@ -442,7 +446,8 @@ 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). When no limiter is + // 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. using RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; @@ -485,8 +490,12 @@ public bool TryGetConnection( // for the default lease). After releasing, signal a waiter on the // idle channel that they may now retry an open. We only do this if // the pool can still grow; if we're at MaxPoolSize, only a connection - // return can satisfy a waiter. FR-004. - if (lease.IsAcquired && _connectionSlots.ReservationCount < MaxPoolSize) + // return can satisfy a waiter. FR-004. This is best-effort; Disposing + // a lease doesn't guarantee that the rate limiter will immediately + // have an available permit. + if (lease.IsAcquired && + (_connectionCreationRateLimiter?.GetStatistics()?.CurrentAvailablePermits ?? 0) > 0 && + _connectionSlots.ReservationCount < MaxPoolSize) { _idleChannel.TryWrite(null); } @@ -505,12 +514,10 @@ public bool TryGetConnection( // 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 any prior error state and resets backoff. FR-009. - if (connection is not null && _errorState.HasError) - { - _errorState.Clear(); + // A successful creation clears error/backoff state + // FR-009. + _errorState?.Clear(); } return connection; @@ -519,10 +526,7 @@ public bool TryGetConnection( { // Enter the blocking period error state on creation failure if configured. // FR-006, FR-007. - if (PoolGroup.IsBlockingPeriodEnabled()) - { - _errorState.Enter(ex); - } + _errorState?.Enter(ex); throw; } From c660cda57ce9f2142ff9564e52bff1d8e3afe04b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 18 Jun 2026 16:17:21 -0700 Subject: [PATCH 14/23] Clean up tests. add comments. --- .../BlockingPeriodErrorStateTest.cs | 30 +- .../ChannelDbConnectionPoolTest.cs | 466 +++++++++++++++--- 2 files changed, 425 insertions(+), 71 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs index e4a031c2e9..157249f5f9 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs @@ -4,7 +4,7 @@ using System; using System.Threading; -using Microsoft.Extensions.TimeProvider.Testing; +using Microsoft.Extensions.Time.Testing; using Microsoft.Data.SqlClient.ConnectionPool; using Xunit; @@ -608,20 +608,23 @@ public void OnEnter_CalledOutsideLock_CanCallBackIntoState() { // Arrange bool hasErrorObservedInCallback = false; - using var state = new BlockingPeriodErrorState( + BlockingPeriodErrorState? state = null; + using (state = new BlockingPeriodErrorState( ownerPoolId: 1, onEnter: () => { // Observe state from inside the callback. // HasError must already be true at this point. - hasErrorObservedInCallback = state.HasError; + hasErrorObservedInCallback = state!.HasError; // Calling ThrowIfActive from the callback must not deadlock. Assert.Throws(() => state.ThrowIfActive()); - }); + })) + { - // Act - state.Enter(new InvalidOperationException("test")); + // Act + state.Enter(new InvalidOperationException("test")); + } // Assert Assert.True(hasErrorObservedInCallback); @@ -639,20 +642,23 @@ public void OnExit_CalledOutsideLock_CanCallBackIntoState() { // Arrange bool hasErrorObservedInCallback = true; // initialized to true; must be false after Clear - using var state = new BlockingPeriodErrorState( + BlockingPeriodErrorState? state = null; + using (state = new BlockingPeriodErrorState( ownerPoolId: 1, onExit: () => { // HasError must already be false when onExit is called. - hasErrorObservedInCallback = state.HasError; + hasErrorObservedInCallback = state!.HasError; // Calling ThrowIfActive from the callback must not deadlock. state.ThrowIfActive(); // must not throw - }); + })) + { - // Act - state.Enter(new Exception()); - state.Clear(); + // Act + state.Enter(new Exception()); + state.Clear(); + } // Assert Assert.False(hasErrorObservedInCallback); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index a72b1e34f3..43f90e660b 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -18,11 +18,25 @@ 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. + /// A configured instance for testing. private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFactory, DbConnectionPoolIdentity? identity = null, DbConnectionPoolGroup? dbConnectionPoolGroup = null, @@ -50,6 +64,10 @@ private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFac ); } + /// + /// 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)] @@ -74,11 +92,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)] @@ -105,11 +126,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() { @@ -125,6 +149,7 @@ public void GetConnectionMaxPoolSize_ShouldTimeoutAfterPeriod() out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -153,6 +178,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() { @@ -168,6 +197,7 @@ public async Task GetConnectionAsyncMaxPoolSize_ShouldTimeoutAfterPeriod() out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -194,6 +224,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() { @@ -217,16 +251,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)), @@ -241,6 +274,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() { @@ -264,6 +301,7 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -271,7 +309,7 @@ out DbConnectionInternal? internalConnection TaskCompletionSource taskCompletionSource = new(); // Act - var exceeded = pool.TryGetConnection( + pool.TryGetConnection( new SqlConnection(""), taskCompletionSource, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), @@ -284,6 +322,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() @@ -308,6 +350,7 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -353,6 +396,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() @@ -377,6 +424,7 @@ out DbConnectionInternal? firstConnection out DbConnectionInternal? internalConnection ); + // Assert Assert.True(completed); Assert.NotNull(internalConnection); } @@ -385,7 +433,7 @@ out DbConnectionInternal? internalConnection TaskCompletionSource failedCompletionSource = new(); // Act - var exceeded = pool.TryGetConnection( + pool.TryGetConnection( new SqlConnection(""), recycledTaskCompletionSource, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), @@ -395,7 +443,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)), @@ -410,6 +458,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() { @@ -446,6 +498,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() { @@ -468,6 +524,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() { @@ -493,13 +553,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(() => @@ -523,16 +588,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 () => @@ -554,58 +626,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, @@ -615,12 +731,19 @@ public void TestLoadBalanceTimeout() hasTransactionAffinity: true ); 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), @@ -632,12 +755,19 @@ public void TestPoolGroup() loadBalanceTimeout: 500, hasTransactionAffinity: true)); 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, @@ -646,34 +776,61 @@ public void TestPoolGroupOptions() loadBalanceTimeout: 500, hasTransactionAffinity: true); 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, @@ -682,6 +839,8 @@ public void TestUseLoadBalancing() loadBalanceTimeout: 500, hasTransactionAffinity: true); var pool = ConstructPool(SuccessfulConnectionFactory, poolGroupOptions: poolGroupOptions); + + // Act & Assert Assert.Equal(poolGroupOptions.UseLoadBalancing, pool.UseLoadBalancing); } @@ -689,41 +848,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() { @@ -758,6 +947,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() { @@ -782,6 +975,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() { @@ -811,6 +1008,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() { @@ -851,6 +1052,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() { @@ -900,6 +1105,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() { @@ -940,10 +1149,22 @@ out DbConnectionInternal? newConnection #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, @@ -957,8 +1178,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, @@ -971,6 +1200,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 @@ -1004,6 +1237,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() { @@ -1022,7 +1259,7 @@ public void Constructor_WithZeroMaxPoolSize_ThrowsArgumentOutOfRangeException() poolGroupOptions ); - // Act & Assert + // Act var exception = Assert.Throws(() => new ChannelDbConnectionPool( SuccessfulConnectionFactory, @@ -1030,15 +1267,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, @@ -1055,7 +1297,7 @@ public void Constructor_WithLargeMaxPoolSize() try { - // Act & Assert - This should not throw ArgumentOutOfRangeException, but may throw OutOfMemoryException + // Act var pool = new ChannelDbConnectionPool( SuccessfulConnectionFactory, dbConnectionPoolGroup, @@ -1063,6 +1305,7 @@ public void Constructor_WithLargeMaxPoolSize() new DbConnectionPoolProviderInfo() ); + // Assert Assert.NotNull(pool); Assert.Equal(0, pool.Count); } @@ -1074,12 +1317,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, @@ -1094,7 +1339,7 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() poolGroupOptions1 ); - // Act & Assert - Pool size of 1 should work + // Act var pool1 = new ChannelDbConnectionPool( SuccessfulConnectionFactory, dbConnectionPoolGroup1, @@ -1102,10 +1347,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, @@ -1120,6 +1366,7 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() poolGroupOptions2 ); + // Act var pool2 = new ChannelDbConnectionPool( SuccessfulConnectionFactory, dbConnectionPoolGroup2, @@ -1127,81 +1374,121 @@ public void Constructor_WithValidSmallPoolSizes_WorksCorrectly() new DbConnectionPoolProviderInfo() ); + // Assert Assert.NotNull(pool2); Assert.Equal(0, pool2.Count); } #region Rate Limiting And Blocking Period Tests - private ChannelDbConnectionPool ConstructPoolWithOptions( - SqlConnectionFactory connectionFactory, - string connectionString, - int maxPoolSize = 4) - { - var poolGroupOptions = new DbConnectionPoolGroupOptions( - poolByIdentity: false, - minPoolSize: 0, - maxPoolSize: maxPoolSize, - creationTimeout: 15, - loadBalanceTimeout: 0, - hasTransactionAffinity: true); - var dbConnectionPoolGroup = new DbConnectionPoolGroup( - new SqlConnectionOptions(connectionString), - new ConnectionPoolKey("TestDataSource", credential: null, accessToken: null, accessTokenCallback: null, sspiContextProvider: null), - poolGroupOptions); - return new ChannelDbConnectionPool( - connectionFactory, - dbConnectionPoolGroup, - DbConnectionPoolIdentity.NoIdentity, - new DbConnectionPoolProviderInfo()); - } - + /// + /// 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 pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); + 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)); + 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() { - var pool = ConstructPoolWithOptions( - TimeoutConnectionFactory, - "Data Source=localhost;Pool Blocking Period=NeverBlock;"); + // 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)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + // Act Assert.Throws(() => pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); - // FR-007: NeverBlock must not enter the error state. + // 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() { - var pool = ConstructPoolWithOptions( - TimeoutConnectionFactory, - "Data Source=localhost;Pool Blocking Period=AlwaysBlock;"); + // 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)); + 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 instead of waiting for a full connection attempt. + /// [Fact] public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() { - var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); + // 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)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + // Act var first = Assert.Throws(() => pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred); @@ -1213,30 +1500,63 @@ public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); sw.Stop(); + // Assert Assert.Equal(first.Message, second.Message); Assert.True(sw.ElapsedMilliseconds < 1000, $"Expected fast-fail (<1000ms) while in blocking period, took {sw.ElapsedMilliseconds}ms."); } + /// + /// Verifies that clearing the pool while in the blocking-period error state resets the + /// externally visible error indicator. + /// [Fact] public void Clear_InErrorState_ResetsErrorOccurred() { - var pool = ConstructPoolWithOptions(TimeoutConnectionFactory, "Data Source=localhost;"); + // 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)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); Assert.Throws(() => pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); Assert.True(pool.ErrorOccurred); - // FR-011: Clear must reset the error state. + // 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 pool = ConstructPoolWithOptions(factory, "Data Source=localhost;"); + 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)); + var pool = ConstructPool(factory, dbConnectionPoolGroup: dbConnectionPoolGroup); // First call fails and enters the error state. factory.FailNextCreate = true; @@ -1250,23 +1570,40 @@ public void SuccessfulCreate_AfterFailure_ClearsErrorState() 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 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 pool = ConstructPoolWithOptions( - TimeoutConnectionFactory, - "Data Source=localhost;Pool Blocking Period=NeverBlock;", - maxPoolSize: 4); + 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)); + var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + // Act & Assert for (int i = 0; i < 8; i++) { await Assert.ThrowsAsync(async () => @@ -1278,10 +1615,21 @@ await Assert.ThrowsAsync(async () => } } + /// + /// 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, From 4c9f154b75b99c618e794d43960056a39c7c5df8 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Thu, 18 Jun 2026 16:26:33 -0700 Subject: [PATCH 15/23] Test basic limiting behavior. --- .../ConnectionPool/ChannelDbConnectionPool.cs | 6 +- .../ChannelDbConnectionPoolTest.cs | 268 +++++++++++++++++- 2 files changed, 268 insertions(+), 6 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 9f56dcf305..8dd0074b1c 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 @@ -100,9 +100,7 @@ internal sealed class ChannelDbConnectionPool : IDbConnectionPool, IDisposable /// 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. /// -#pragma warning disable CS0649 // Field is never assigned — rate-limiter wiring is in-progress (FR-001) private readonly RateLimiter? _connectionCreationRateLimiter; -#pragma warning restore CS0649 /// /// Encapsulates the blocking-period error state for this pool: cached exception, exponential @@ -119,7 +117,8 @@ internal ChannelDbConnectionPool( SqlConnectionFactory connectionFactory, DbConnectionPoolGroup connectionPoolGroup, DbConnectionPoolIdentity identity, - DbConnectionPoolProviderInfo connectionPoolProviderInfo) + DbConnectionPoolProviderInfo connectionPoolProviderInfo, + RateLimiter? connectionCreationRateLimiter = null) { ConnectionFactory = connectionFactory; PoolGroup = connectionPoolGroup; @@ -129,6 +128,7 @@ internal ChannelDbConnectionPool( AuthenticationContexts = new(); MaxPoolSize = Convert.ToUInt32(PoolGroupOptions.MaxPoolSize); TransactedConnectionPool = new(this); + _connectionCreationRateLimiter = connectionCreationRateLimiter; _connectionSlots = new(MaxPoolSize); _idleChannel = new(); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 43f90e660b..7551e9c330 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -3,10 +3,12 @@ // 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.Diagnostics; using System.Threading; +using System.Threading.RateLimiting; using System.Threading.Tasks; using System.Transactions; using Microsoft.Data.Common; @@ -36,12 +38,14 @@ public class ChannelDbConnectionPoolTest /// 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, @@ -60,7 +64,8 @@ private ChannelDbConnectionPool ConstructPool(SqlConnectionFactory connectionFac connectionFactory, dbConnectionPoolGroup, identity ?? DbConnectionPoolIdentity.NoIdentity, - connectionPoolProviderInfo ?? new DbConnectionPoolProviderInfo() + connectionPoolProviderInfo ?? new DbConnectionPoolProviderInfo(), + connectionCreationRateLimiter ); } @@ -1580,6 +1585,89 @@ public void SuccessfulCreate_AfterFailure_ClearsErrorState() 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); + 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. @@ -1591,6 +1679,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 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), @@ -1601,7 +1690,10 @@ public async Task RateLimiter_LeaseDisposedOnFailure_DoesNotStarvePool() creationTimeout: 15, loadBalanceTimeout: 0, hasTransactionAffinity: true)); - var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + var pool = ConstructPool( + TimeoutConnectionFactory, + dbConnectionPoolGroup: dbConnectionPoolGroup, + connectionCreationRateLimiter: rateLimiter); // Act & Assert for (int i = 0; i < 8; i++) @@ -1613,6 +1705,9 @@ await Assert.ThrowsAsync(async () => await tcs.Task; }); } + + Assert.Equal(8, rateLimiter.AttemptAcquireCount); + Assert.Equal(0, rateLimiter.OutstandingPermitCount); } /// @@ -1647,6 +1742,173 @@ protected override DbConnectionInternal CreateConnection( } } + /// + /// Test connection factory that records how many physical connection creations the pool + /// performs so rate-limiter tests can distinguish reuse from new opens. + /// + 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 From bd02bf4d9bc9ad0ebdac01630b8383b5f0c181f8 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 22 Jun 2026 11:44:28 -0700 Subject: [PATCH 16/23] Use appropriate version based on TFM --- Directory.Packages.props | 3 ++- .../tests/UnitTests/Microsoft.Data.SqlClient.UnitTests.csproj | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index b428a0294e..39b96a1702 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -145,12 +145,12 @@ - + @@ -158,6 +158,7 @@ + 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 e09e49c967..eefa88ac4a 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 @@ -100,6 +100,7 @@ + @@ -116,6 +117,7 @@ + From 878be81aa43c7a4fe4cca1363cc6f7ce9d76105b Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 22 Jun 2026 13:32:07 -0700 Subject: [PATCH 17/23] No 8.0.1 published --- Directory.Packages.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index 39b96a1702..9a27c0d37d 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -158,7 +158,7 @@ - + From 72c080cc200dcd794514a4fd10f5f3a374c388dd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 22 Jun 2026 22:25:08 +0000 Subject: [PATCH 18/23] Add RateLimiting package to ManualTests project Co-authored-by: mdaigle <4722049+mdaigle@users.noreply.github.com> --- .../ManualTests/Microsoft.Data.SqlClient.ManualTests.csproj | 2 ++ 1 file changed, 2 insertions(+) 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 ce159f7821..202db19a64 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 @@ -99,6 +99,7 @@ + @@ -126,6 +127,7 @@ + From 25524915cbe5a91850f5232ff8d7f1d71c953352 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 22 Jun 2026 17:18:35 -0700 Subject: [PATCH 19/23] Fix async flow. Dispose of error state. --- .../ConnectionPool/BlockingPeriodErrorState.cs | 12 +++++++++++- .../ConnectionPool/ChannelDbConnectionPool.cs | 5 +++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs index dad2019fbe..935b3ccc44 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs @@ -4,6 +4,7 @@ using System; using System.Threading; +using Microsoft.Data.Common; using Microsoft.Data.SqlClient.Internal; #nullable enable @@ -94,7 +95,16 @@ internal void Enter(Exception ex) _cachedException = ex; wait = _nextWait; - newTimer = _timeProvider.CreateTimer(ExitCallback, null, Timeout.InfiniteTimeSpan, Timeout.InfiniteTimeSpan); + // Create the exit timer disarmed (infinite due time); it is armed below outside + // the lock. ADP.UnsafeCreateTimer suppresses execution-context flow so the timer + // doesn't capture and pin the current ExecutionContext and its AsyncLocals for its + // lifetime, while still honoring the injected TimeProvider for testability. + newTimer = ADP.UnsafeCreateTimer( + _timeProvider, + ExitCallback, + null, + Timeout.InfiniteTimeSpan, + Timeout.InfiniteTimeSpan); oldTimer = _exitTimer; _exitTimer = newTimer; 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 8dd0074b1c..1d489d9c94 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 @@ -299,6 +299,11 @@ public void Shutdown() { State = ShuttingDown; Pruner?.Dispose(); + + // 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. + _errorState?.Dispose(); } /// From f6358f42ad25ffcfc666895e721b4da914886906 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 22 Jun 2026 17:37:15 -0700 Subject: [PATCH 20/23] Release before poking --- .../ConnectionPool/ChannelDbConnectionPool.cs | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 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 1d489d9c94..ffb8e18b8c 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 @@ -455,10 +455,15 @@ public bool TryGetConnection( // 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. - using RateLimitLease lease = _connectionCreationRateLimiter?.AttemptAcquire(1) ?? NoOpAcquiredLease.Instance; + + //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 (!lease.IsAcquired) + if (!leaseAcquired) { // TODO: When we fail to acquire a lease, surface the lease metadata // (e.g. RateLimitMetadataName.RetryAfter, ReasonPhrase) in the error @@ -491,16 +496,23 @@ public bool TryGetConnection( } finally { - // Disposing the lease releases the permit back to the limiter (no-op - // for the default lease). After releasing, signal a waiter on the - // idle channel that they may now retry an open. We only do this if + // 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; Disposing - // a lease doesn't guarantee that the rate limiter will immediately - // have an available permit. - if (lease.IsAcquired && - (_connectionCreationRateLimiter?.GetStatistics()?.CurrentAvailablePermits ?? 0) > 0 && - _connectionSlots.ReservationCount < MaxPoolSize) + // 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); } From 58f34da0d6c67bc44747455215d280f2fce8f4c5 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Mon, 22 Jun 2026 17:41:26 -0700 Subject: [PATCH 21/23] Fix comment --- .../Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs index b71fbc48e3..c1f2b730cc 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs @@ -95,9 +95,7 @@ internal DbConnectionPoolGroupProviderInfo ProviderInfo /// /// Determines whether the blocking period is enabled for this pool group based on the - /// configured and the target data source. Returns true - /// when no connection options are available so callers fail safe into the blocking - /// behavior. + /// configured and the target data source. /// internal bool IsBlockingPeriodEnabled() { From 048ad2f79962cf3a0960e6a5c6c90dc6bb2c9340 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 23 Jun 2026 09:21:19 -0700 Subject: [PATCH 22/23] Make test more reliable. --- .../ChannelDbConnectionPoolTest.cs | 49 ++++++++++++++----- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs index 7551e9c330..b0ae206780 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using System.Collections.Concurrent; using System.Data.Common; -using System.Diagnostics; using System.Threading; using System.Threading.RateLimiting; using System.Threading.Tasks; @@ -1475,12 +1474,13 @@ public void ErrorOccurred_FailureWithAlwaysBlock_BecomesTrue() /// /// Verifies that once the pool enters the blocking period, subsequent synchronous requests - /// fail fast with the cached exception instead of waiting for a full connection attempt. + /// 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), @@ -1491,24 +1491,24 @@ public void ErrorOccurred_BlockingEnabled_SubsequentRequestFastFails() creationTimeout: 15, loadBalanceTimeout: 0, hasTransactionAffinity: true)); - var pool = ConstructPool(TimeoutConnectionFactory, dbConnectionPoolGroup: dbConnectionPoolGroup); + 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. We assert it returns very quickly compared to a fresh attempt. - var sw = Stopwatch.StartNew(); + // cached exception without attempting another physical open. var second = Assert.Throws(() => pool.TryGetConnection(new SqlConnection(), taskCompletionSource: null, TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)), out _)); - sw.Stop(); - // Assert + // 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(sw.ElapsedMilliseconds < 1000, - $"Expected fast-fail (<1000ms) while in blocking period, took {sw.ElapsedMilliseconds}ms."); + Assert.True(pool.ErrorOccurred); + Assert.Equal(1, factory.CreateCount); } /// @@ -1743,10 +1743,35 @@ protected override DbConnectionInternal CreateConnection( } /// - /// Test connection factory that records how many physical connection creations the pool - /// performs so rate-limiter tests can distinguish reuse from new opens. + /// 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 CountingSuccessfulConnectionFactory : SqlConnectionFactory + 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(); + } + } + + /// { /// /// Gets the number of times the pool asked the factory to create a physical connection. From c61f65f87dfeab49371d0d27476466713ca2dfb2 Mon Sep 17 00:00:00 2001 From: Malcolm Daigle Date: Tue, 23 Jun 2026 10:04:40 -0700 Subject: [PATCH 23/23] Add TimeProvider overload of ADP.UnsafeCreateTimer BlockingPeriodErrorState calls a 5-arg UnsafeCreateTimer(TimeProvider, ...) returning ITimer that was missing from AdapterUtil, breaking the build. Re-add the overload, which suppresses ExecutionContext flow while honoring the injected TimeProvider for testability. --- .../src/Microsoft/Data/Common/AdapterUtil.cs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs index 568592072d..3bb0c09875 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs @@ -175,6 +175,27 @@ internal static Timer UnsafeCreateTimer(TimerCallback callback, object state, Ti } } + internal static ITimer UnsafeCreateTimer( + TimeProvider timeProvider, + TimerCallback callback, + object state, + TimeSpan dueTime, + TimeSpan period) + { + // Don't capture the current ExecutionContext and its AsyncLocals onto + // a timer causing them to live forever. Honor the supplied TimeProvider + // so callers can inject a test double for deterministic scheduling. + if (ExecutionContext.IsFlowSuppressed()) + { + return timeProvider.CreateTimer(callback, state, dueTime, period); + } + + using (ExecutionContext.SuppressFlow()) + { + return timeProvider.CreateTimer(callback, state, dueTime, period); + } + } + #region COM+ exceptions internal static ArgumentException Argument(string error)