Skip to content

Close remaining ChannelDbConnectionPool parity gaps with WaitHandleDbConnectionPool - #4490

Draft
mdaigle wants to merge 1 commit into
dev/automation/channel-pool-transactionsfrom
dev/automation/channel-pool-v2-parity
Draft

Close remaining ChannelDbConnectionPool parity gaps with WaitHandleDbConnectionPool#4490
mdaigle wants to merge 1 commit into
dev/automation/channel-pool-transactionsfrom
dev/automation/channel-pool-v2-parity

Conversation

@mdaigle

@mdaigle mdaigle commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Stacked on #4487 (dev/automation/channel-pool-transactions). Review only the top commit; the rest is #4487.

Why

I ran the whole test suite against both pool implementations and diffed the results, plus built a standalone differential harness that exercises pool semantics the suite doesn't cover. That turned up four behavioural gaps in ChannelDbConnectionPool that had no test coverage, so nothing was catching them.

What

1. Emancipated connection reclamation. A SqlConnection that was garbage collected without ever being closed or disposed left its internal connection permanently occupying a pool slot. At MaxPoolSize that meant every subsequent Open timed out — forever, not just once. GetInternalConnection now sweeps for emancipated connections before parking on the idle channel, the same way WaitHandleDbConnectionPool does. The sweep is deliberately confined to the slow path: it's O(MaxPoolSize) and allocates a snapshot, so it must not run on the hot acquire path.

2. Pool metrics were never emitted. PooledConnections, FreeConnections, ActiveConnections and the soft/hard connect and disconnect counters all read zero under this pool. Wired up the same call sites the wait handle pool uses. IdleConnectionChannel is a convenient single choke point for the free-connection counters.

3. Count reported reservations rather than connections. Reservations include connections that are still being opened, whereas V1's Count is _totalObjects. This broke the SQL Express user instance path in SqlConnectionFactory.CreateConnection, which branches on pool.Count <= 0 — it took the wrong branch and threw an NRE out of SqlConnectionOptions.ValidateValueLength because providerInfo.InstanceName was never populated. Added ConnectionPoolSlots.ConnectionCount and pointed Count at it.

4. Async opens always completed asynchronously. WaitHandleDbConnectionPool makes a non-blocking, non-creating attempt at an idle connection before enqueuing a pending open; this pool did not, so OpenAsync against a warm pool always took a thread pool hop. Added the same fast path. Transactional requests are excluded from it — they have to go through the transacted store first to find a connection already enlisted in the same transaction, and taking a plain idle connection would both miss that affinity and skip enlistment.

Tests

  • New ConnectionPoolVersionScope helper. It flips the switch and clears all pools on entry and exit — the clearing matters because a pool binds to its implementation at creation time, so without it V2 pools leak into later tests.
  • Parameterized by pool version: ReclaimEmancipatedOnOpenTest, MaxPoolWaitForConnectionTest, ConnectionResiliencySPIDTest, MetricsTest.PooledConnectionsCounters_Functional. Each was verified to fail before its corresponding fix and pass after.
  • ChannelDbConnectionPoolTest.StressTestAsync awaited its TaskCompletionSource unconditionally, which hangs now that TryGetConnection can complete synchronously.
  • Three pool-exhaustion unit tests let their owning SqlConnections go out of scope, so reclamation could legitimately hand the "should time out" waiter a connection. They now keep the owners alive, which is also what they actually meant.
  • TvpTest.TestPacketNumberWraparound passed an async lambda to Task.Factory.StartNew and so awaited a Task<Task>, never observing the inner task or its failures. Added the missing Unwrap.

Verification

Ran all three suites under both pools on net9.0/managed SNI against SQL Server, and the failure sets are identical apart from the one expected difference.

Suite V1 V2
FunctionalTests 1267 passed / 72 failed 1267 passed / 72 failed — identical set
UnitTests 985 passed / 4 failed 984 passed / 5 failed — delta is TestDefaultAppContextSwitchValues, which necessarily fails when the switch is globally on
ManualTests 1227 passed / 37 failed 1227 passed / 37 failed — identical set

The pre-existing failures in both columns are environmental for my box (no MSDTC, no SQL CLR/UDT support, Windows-only CNG/CSP and named pipe tests).

Notably all TransactionEnlistmentTest.* cases and MetricsTest.TransactedConnectionPool_VerifyActiveConnectionCounters now pass under V2 — those were the outstanding failures before #4487 landed.

I also wrote a transaction-focused differential harness covering scope commit/rollback, transaction affinity across two connections in one scope, an enlisted connection not being handed to a non-transactional caller, return-to-pool after the transaction ends, explicit SqlTransaction, manual EnlistTransaction, async open inside a scope, scoped open against a pre-warmed pool (specifically to catch the sync fast path stealing an unenlisted connection), and 15 s of concurrent transaction churn across 16 tasks verifying the committed row count exactly. 10/10 on both pools; V2 sustained ~12% more committed transactions per second.

Checklist

  • Tests added or updated
  • Public API changes documented — none, all changes are internal
  • Verified against customer repro — N/A
  • Ensure no breaking changes introduced

…ConnectionPool

Differential testing of the two pool implementations (running the full
Functional/Unit/Manual suites under both, plus a bespoke harness) turned up
four behavioural gaps in ChannelDbConnectionPool that had no test coverage.

1. Emancipated connection reclamation. A SqlConnection that was garbage
   collected without ever being closed or disposed left its internal connection
   permanently occupying a pool slot. At MaxPoolSize this meant every
   subsequent Open timed out forever. GetInternalConnection now sweeps for
   emancipated connections before parking on the idle channel, mirroring
   WaitHandleDbConnectionPool. The sweep is confined to the slow path since it
   is O(MaxPoolSize) and allocates a snapshot.

2. Pool metrics were never emitted. PooledConnections, FreeConnections,
   ActiveConnections and the soft/hard connect and disconnect counters all read
   zero under this pool. Wired up the same call sites the wait handle pool uses.

3. Count reported reservations rather than connections. Reservations include
   connections that are still being opened, which broke the SQL Express user
   instance path in SqlConnectionFactory.CreateConnection (a `pool.Count <= 0`
   check) with a NullReferenceException. Added
   ConnectionPoolSlots.ConnectionCount and pointed Count at it.

4. Async opens always completed asynchronously. The wait handle pool makes a
   non-blocking, non-creating attempt at an idle connection before enqueuing a
   pending open; this pool did not, so OpenAsync against a warm pool always
   took a thread pool hop. Added the same fast path, excluding transactional
   requests so they still go through the transacted store and enlist properly.

Test changes:

- Added ConnectionPoolVersionScope, which flips the pool version switch and
  clears all pools on both entry and exit. Clearing is required because a pool
  binds to its implementation at creation time, so without it pools leak across
  tests.
- Parameterized ReclaimEmancipatedOnOpenTest, MaxPoolWaitForConnectionTest,
  ConnectionResiliencySPIDTest and MetricsTest.PooledConnectionsCounters_Functional
  by pool version. Each was verified to fail before the corresponding fix.
- ChannelDbConnectionPoolTest.StressTestAsync awaited its TaskCompletionSource
  unconditionally, which hangs now that TryGetConnection can complete
  synchronously.
- Three pool-exhaustion unit tests let their owning SqlConnections go out of
  scope, so reclamation could legitimately hand the "should time out" waiter a
  connection. They now keep the owners alive.
- TvpTest.TestPacketNumberWraparound passed an async lambda to
  Task.Factory.StartNew and so awaited a Task<Task>, never observing the inner
  task. Added the missing Unwrap.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mdaigle
mdaigle requested a review from a team as a code owner July 29, 2026 21:35
Copilot AI review requested due to automatic review settings July 29, 2026 21:35
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Jul 29, 2026
@mdaigle
mdaigle changed the base branch from dev/mdaigle/replace-conn-2 to dev/automation/channel-pool-transactions July 29, 2026 21:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Closes the remaining behavioral and observability parity gaps between ChannelDbConnectionPool (V2) and WaitHandleDbConnectionPool (V1), primarily around reclaiming emancipated connections, emitting pool metrics, reporting Count consistently, and allowing async opens to complete synchronously on a warm pool when safe.

Changes:

  • Add an emancipated-connection reclamation sweep on the V2 slow acquisition path to prevent permanent pool-slot leaks at MaxPoolSize.
  • Wire up V2 pool metrics (pooled/free/active connections and connect/disconnect counters) and align Count semantics with V1 via a new tracked connection count.
  • Improve V2 async acquisition parity by attempting a non-blocking idle-channel hit before enqueuing work (excluding transactional requests), and update tests to run under both pool versions with proper isolation.

Reviewed changes

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

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs Fix test reliability under new synchronous-completion behavior and prevent GC-induced “emancipated” reclamation from invalidating pool-exhaustion tests.
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs Parameterize pooled-connection metrics validation across V1/V2 via a pool-version scope helper.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs Fix Task.Factory.StartNew async-lambda misuse by Unwrap() so failures/timeout behavior are observed correctly.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs Parameterize resiliency test across pool versions with isolation via the new scope helper.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolTest.cs Run emancipated-reclaim and max-pool-wait tests under both pool versions using a shared provider and pool-version scope.
src/Microsoft.Data.SqlClient/tests/Common/ConnectionPoolVersionScope.cs New RAII helper to toggle UseConnectionPoolV2 and clear pools on entry/exit to prevent cross-test implementation leakage.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs Emit free-connection metric transitions at the idle-channel choke point.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs Track actual connection count (distinct from reservations) and add a best-effort snapshot API for infrequent bookkeeping.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Align Count with V1, add idle-channel async fast path, add emancipated reclamation sweep, and wire up pooled/soft/hard metric emission.

@mdaigle
mdaigle marked this pull request as draft July 29, 2026 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

2 participants