Extract pool blocking-period error state into BlockingPeriodErrorState by mdaigle · Pull Request #4395 · dotnet/SqlClient

mdaigle · 2026-06-23T17:24:37Z

Summary

This is Part 1 of 3 splitting #4376 ("Dev/mdaigle/pool rate limit") into stacked, reviewable PRs.

This PR extracts the connection pool's blocking-period (error backoff) logic out of WaitHandleDbConnectionPool into a reusable, testable BlockingPeriodErrorState class. This keeps the pool's connection-acquisition path focused on capacity/queue concerns. Part 2 will use the new class to implement blocking period support in the ChannelDbConnectionPool.

The pool blocking period follows this state machine:

The pool calls Enter() any time blocking is enabled and an error occurs while opening a connection. This puts the pool in the blocked state and blocks future requests from attempting an open until the period ends. Subsequent errors double the blocking period (up to a 60s max). Successful connections reset the error state and blocking period.

To show that nothing changed for the existing pool, I added new unit tests and applied them selectively in a separate branch. If these pass, then the refactors have not impacted behavior: #4411

Changes

New BlockingPeriodErrorState — encapsulates cached-exception fast-fail, exponential backoff (5s → 60s cap), exit timer, and synchronization. Takes an injectable TimeProvider so timer scheduling is deterministic in tests.
WaitHandleDbConnectionPool — refactored to use BlockingPeriodErrorState instead of inline error-state fields/logic.
DbConnectionPoolGroup.IsBlockingPeriodEnabled() — new helper centralizing the PoolBlockingPeriod decision (Auto → not an Azure SQL endpoint, AlwaysBlock → true, NeverBlock → false). Consumed by Part 2.
ADP.UnsafeCreateTimer(TimeProvider, ...) — new overload returning ITimer that suppresses ExecutionContext flow while honoring the injected TimeProvider; required by BlockingPeriodErrorState.
BlockingPeriodErrorStateTest — unit tests using FakeTimeProvider (29 tests).

Stacking

Part 1 (this PR) → main
Part 2 (channel-pool rate limiting) → this branch
Part 3 (GitHub instructions) → Part 2

Checklist

Tests added (29 passing via FakeTimeProvider)
No public API changes
No breaking changes
Verified against customer repro (n/a — internal refactor)

Move the connection pool's blocking-period (error backoff) logic out of WaitHandleDbConnectionPool into a reusable, testable BlockingPeriodErrorState class with exponential backoff (5s..60s), cached-exception fast-fail, and an injectable TimeProvider for deterministic tests. Add DbConnectionPoolGroup.IsBlockingPeriodEnabled() and the ADP.UnsafeCreateTimer(TimeProvider,...) overload it depends on. Includes BlockingPeriodErrorStateTest.

Copilot

Pull request overview

Refactors the legacy wait-handle connection pool’s blocking-period (error backoff) logic into a dedicated BlockingPeriodErrorState component, adds a pool-group helper for determining when blocking is enabled, and introduces a TimeProvider-based timer factory to enable deterministic unit testing.

Changes:

Added BlockingPeriodErrorState (cached exception + exponential backoff + exit timer) and corresponding unit tests using FakeTimeProvider.
Refactored WaitHandleDbConnectionPool to delegate blocking-period logic to the new state object and moved the blocking-period enablement decision into DbConnectionPoolGroup.
Added an ADP.UnsafeCreateTimer(TimeProvider, ...) overload returning ITimer to support TimeProvider-driven timers.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/BlockingPeriodErrorStateTest.cs	Adds comprehensive unit tests for the new blocking-period state logic using `FakeTimeProvider`.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs	Replaces inlined error/backoff fields and timer logic with `BlockingPeriodErrorState`.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs	Centralizes the `PoolBlockingPeriod` decision into `IsBlockingPeriodEnabled()`.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs	Introduces the new reusable blocking-period error state implementation.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs	Adds a `TimeProvider`-aware `UnsafeCreateTimer` overload returning `ITimer`.

codecov · 2026-06-23T20:14:37Z

Codecov Report

❌ Patch coverage is 95.27559% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.93%. Comparing base (44e1b72) to head (7d20704).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs	75.00%	2 Missing ⚠️
...lient/ConnectionPool/WaitHandleDbConnectionPool.cs	90.47%	2 Missing ⚠️
...lClient/ConnectionPool/BlockingPeriodErrorState.cs	98.87%	1 Missing ⚠️
.../SqlClient/ConnectionPool/DbConnectionPoolGroup.cs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4395      +/-   ##
==========================================
- Coverage   65.32%   63.93%   -1.40%     
==========================================
  Files         285      282       -3     
  Lines       43373    66630   +23257     
==========================================
+ Hits        28335    42597   +14262     
- Misses      15038    24033    +8995

Flag	Coverage Δ
CI-SqlClient	`?`
PR-SqlClient-Project	`63.93% <95.27%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mdaigle · 2026-06-23T17:43:51Z

+            // so callers can inject a test double for deterministic scheduling.
+            if (ExecutionContext.IsFlowSuppressed())
+            {
+                return timeProvider.CreateTimer(callback, state, dueTime, period);


Creating a timer via a TimeProvider allows for faking time in unit tests. I chose not to change existing overloads to feed into this one because there are many spots already use them and it would be a more extensive refactor.

mdaigle · 2026-06-23T18:01:58Z

            // instead obtained creation mutex

            DbConnectionInternal obj = null;
-            if (ErrorOccurred)


There was a check/act race condition here. The new class uses a local variable to check and throw.

mdaigle · 2026-06-23T18:03:26Z

                                SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Errors are set.", Id);
                                Interlocked.Decrement(ref _waitCount);
-                                throw TryCloneCachedException();
+                                _errorState.ThrowIfActive();


There is a chance this may not throw, in which case, loop back around to check available wait handles.

mdaigle · 2026-06-23T18:07:22Z


-                // Reset the error wait:
-                _errorWait = ERROR_WAIT_DEFAULT;
+                // A successful creation clears any prior error state and resets backoff.


This behavior was missing before. If we somehow successfully create a connection while in the blocking period (via some race condition with checking the error state), then we should reset the whole error state including the ManualResetEvent and the cached exception, not just the wait period. There's no need to wait until the timer fires to allow more creates if we know we're succeeding now.

mdaigle · 2026-06-23T18:26:37Z

                newObj = null; // set to null, so we do not return bad new object

-                // Failed to create instance
-                _resError = e;


Lots of race conditions possible here. I decided to move this all under a lock so that all of the relevant values are updated atomically.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

paulmedynski · 2026-06-24T17:53:17Z

+                // (onEnter/onExit) can never diverge from the internal state transitions under
+                // concurrent Enter/Clear/exit-timer activity. The callbacks are expected to be
+                // cheap, non-reentrant operations.
+                _onEnter?.Invoke();


Guard against misbehaving callbacks and Dispose() with try/catch that swallows. Same for Clear(), ExitCallback(), and our Dispose().

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

paulmedynski

Latest changes look good. Waiting for further offline discussion on my earlier comments.

paulmedynski · 2026-06-26T12:36:57Z

        /// <param name="period">The interval between invocations, or
        /// <see cref="Timeout.InfiniteTimeSpan"/> to disable periodic signaling.</param>
        /// <returns>An <see cref="ITimer"/> created by <paramref name="timeProvider"/>.</returns>
-        // TODO: Route the other UnsafeCreateTimer overloads through this method (passing


We can likely route all other UnsafeCreateTimer methods through this one (and probably remove some of the overloads due to type inference). But let's save that for another PR to avoid cluttering this one up.

It's a good change, but it'll be a bit of a larger change because this returns ITimer whereas the others return Timer.

paulmedynski · 2026-06-26T12:41:01Z

 dotnet_diagnostic.xUnit1030.severity=none
+
+# Disables warning for unnamed enum case values
+# e.g. providing an invalid int value for an enum that wraps int


Is this something we only want suppressed in tests? When would legit driver code want to fake an enum value? I suppose we may have some existing debt in this area.

Agred, this setting is riskier. I think I'll remove it to require a default case.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

priyankatiwari08 · 2026-07-02T08:16:51Z

+// 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;


Can we also add tests covering the _onEnter / _onExit callback contract?
The current tests exercise the Enter / Clear / ExitCallback state transitions well, but I don't see coverage for what happens when the pool-supplied onEnter / onExit callbacks themselves misbehave. Since these callbacks are invoked under the internal lock and are the primary integration point for the wait-handle pool, pinning their contract explicitly would help prevent regressions.

I think the onEnter/onExit abstractions was a bit too generic. There's no other consumer of that contract, and we don't plan to ever add one. I'm going to replace them with a reference to the wait handle pool's error ManualResetEvent so that we can set/reset directly.

paulmedynski · 2026-07-02T11:20:57Z

                PoolBlockingPeriod.AlwaysBlock => true,
-                PoolBlockingPeriod.NeverBlock => false
+                PoolBlockingPeriod.NeverBlock => false,
+                _ => throw ADP.InvalidEnumerationValue(typeof(PoolBlockingPeriod), (int)_connectionOptions.PoolBlockingPeriod)


Doesn't this downgrade a compiler-detected programming error into a runtime error?

I'm going to walk back all these enum changes. We can take these as a standalone piece of work.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

…m:dotnet/SqlClient into dev/mdaigle/pool-blocking-period-refactor

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Copilot AI review requested due to automatic review settings June 23, 2026 17:24

github-project-automation Bot added this to SqlClient Board Jun 23, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Jun 23, 2026

Copilot started reviewing on behalf of mdaigle June 23, 2026 17:25 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

This was referenced Jun 23, 2026

Add connection-creation rate limiting to ChannelDbConnectionPool #4396

Draft

Document pool rate limiting in Copilot instructions #4397

Draft

mdaigle added 2 commits June 23, 2026 10:53

Add comments

2a4a1f4

Improve locking

3517c19

mdaigle commented Jun 23, 2026

View reviewed changes

mdaigle marked this pull request as ready for review June 23, 2026 20:45

Copilot AI review requested due to automatic review settings June 23, 2026 20:45

mdaigle requested a review from a team as a code owner June 23, 2026 20:45

mdaigle added this to the 7.1.0-preview2 milestone Jun 23, 2026

github-actions Bot assigned paulmedynski and priyankatiwari08 Jun 23, 2026

Copilot started reviewing on behalf of mdaigle June 23, 2026 20:46 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

paulmedynski requested changes Jun 24, 2026

View reviewed changes

github-project-automation Bot moved this from To triage to Waiting for customer in SqlClient Board Jun 24, 2026

mdaigle added 2 commits June 24, 2026 16:53

First round of review changes.

b3de8e7

Require exhaustive switch statements.

5a4ae39

Copilot AI review requested due to automatic review settings June 25, 2026 23:24

Copilot started reviewing on behalf of mdaigle June 25, 2026 23:24 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

paulmedynski reviewed Jun 26, 2026

View reviewed changes

Add wait handle pool unit tests for blocking period.

6ba4b82

mdaigle mentioned this pull request Jun 26, 2026

Dev/automation/pool blocking tests on main #4411

Draft

mdaigle added 5 commits July 1, 2026 10:17

Remove rule.

a985fcb

Improve tests. Add TODOs.

43afe78

Update test names and xml.

94eb2d1

Add documentation about callback idempotency.

27d8e44

Fix xml docs

6b2b167

Copilot AI review requested due to automatic review settings July 1, 2026 18:22

Copilot started reviewing on behalf of mdaigle July 1, 2026 18:22 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Add default switch case.

7d20704

mdaigle requested review from paulmedynski and priyankatiwari08 July 1, 2026 18:34

priyankatiwari08 reviewed Jul 2, 2026

View reviewed changes

paulmedynski requested changes Jul 2, 2026

View reviewed changes

priyankatiwari08 modified the milestones: 7.1.0-preview2, 7.1.0-preview3 Jul 2, 2026

Merge branch 'main' into dev/mdaigle/pool-blocking-period-refactor

f93c887

Copilot AI review requested due to automatic review settings July 2, 2026 19:49

Copilot started reviewing on behalf of mdaigle July 2, 2026 19:49 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

mdaigle added 6 commits July 7, 2026 17:17

Swallow errors. Fix disposal.

a124dba

Merge branch 'dev/mdaigle/pool-blocking-period-refactor' of github.co…

61af086

…m:dotnet/SqlClient into dev/mdaigle/pool-blocking-period-refactor

Fix test conflicts

a34c328

Simplify enter/exit API.

0a06f17

Restore original behavior.

c468a29

Revert editorconfig changes.

926ed13

Copilot AI review requested due to automatic review settings July 8, 2026 01:12

Copilot started reviewing on behalf of mdaigle July 8, 2026 01:13 View session

Copilot AI reviewed Jul 8, 2026

View reviewed changes

Comment thread ...osoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/BlockingPeriodErrorState.cs

mdaigle requested review from paulmedynski and priyankatiwari08 July 8, 2026 14:41

Uh oh!

Conversation

mdaigle commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Stacking

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle commented Jun 23, 2026 •

edited

Loading

codecov Bot commented Jun 23, 2026 •

edited

Loading