Skip to content

Emit pool metrics, fix Count semantics, and add an async idle fast path - #4504

Draft
mdaigle wants to merge 1 commit into
dev/automation/channel-pool-v2-parityfrom
dev/automation/channel-pool-v2-followups
Draft

Emit pool metrics, fix Count semantics, and add an async idle fast path#4504
mdaigle wants to merge 1 commit into
dev/automation/channel-pool-v2-parityfrom
dev/automation/channel-pool-v2-followups

Conversation

@mdaigle

@mdaigle mdaigle commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Stacked on #4490, which is itself stacked on #4487. Review only the top commit.

These are the remaining ChannelDbConnectionPool parity gaps I found while differential-testing the two pool implementations. None of them had test coverage, so nothing was catching them.

1. 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 WaitHandleDbConnectionPool uses.

IdleConnectionChannel turns out to be a convenient single choke point for the free-connection counters, since every idle enqueue and dequeue passes through it — no need to scatter the calls across the pool.

2. Count reported reservations rather than connections

Reservations include connections that are still being opened, whereas the wait handle pool's Count is its total object count. This broke the SQL Express user instance path in SqlConnectionFactory.CreateConnection, which branches on pool.Count <= 0: it took the wrong branch and threw a NullReferenceException out of SqlConnectionOptions.ValidateValueLength, because providerInfo.InstanceName was never populated.

Added ConnectionPoolSlots.ConnectionCount, which tracks slot occupancy rather than reservations, and pointed Count at it. ReservationCount stays as-is for the callers that genuinely want capacity accounting.

3. 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. That's an observable behavioural difference, not just a perf one.

Added the same fast path. It deliberately does not try to create a connection — that can block on the wire and must stay off the caller's thread.

Transactional requests are excluded from the fast path. They have to consult the transacted store first for a connection already enlisted in the same transaction, which only GetInternalConnection does; taking a plain idle connection would both miss that affinity and skip enlistment. I have a harness scenario that opens inside a TransactionScope against a pre-warmed pool specifically to catch this.

Tests

  • Parameterized ConnectionResiliencySPIDTest and MetricsTest.PooledConnectionsCounters_Functional by pool version, using the ConnectionPoolVersionScope helper from Reclaim emancipated connections in ChannelDbConnectionPool #4490.
  • ChannelDbConnectionPoolTest.StressTestAsync awaited its TaskCompletionSource unconditionally, which hangs now that TryGetConnection can complete synchronously.
  • 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. 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, Windows-only CNG/CSP and named pipe tests).

All TransactionEnlistmentTest.* cases and MetricsTest.TransactedConnectionPool_VerifyActiveConnectionCounters pass under V2 with this stack applied.

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, 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

Three remaining behavioural gaps between ChannelDbConnectionPool and
WaitHandleDbConnectionPool, none of which had test coverage.

1. 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, since every idle enqueue and dequeue passes through it.

2. Count reported reservations rather than connections. Reservations include
   connections that are still being opened, whereas the wait handle pool's Count
   is its total object count. This broke the SQL Express user instance path in
   SqlConnectionFactory.CreateConnection, which branches on `pool.Count <= 0`: it
   took the wrong branch and threw a NullReferenceException out of
   SqlConnectionOptions.ValidateValueLength because providerInfo.InstanceName was
   never populated. Added ConnectionPoolSlots.ConnectionCount, which tracks slot
   occupancy rather than reservations, and pointed Count at it.

3. 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. It deliberately does not try to
   *create* a connection, which can block on the wire and must stay off the
   caller's thread.

   Transactional requests are excluded from the fast path. They have to consult
   the transacted store first for a connection already enlisted in the same
   transaction, which only GetInternalConnection does; taking a plain idle
   connection would both miss that affinity and skip enlistment.

Tests:

- Parameterized ConnectionResiliencySPIDTest and
  MetricsTest.PooledConnectionsCounters_Functional by pool version.
- ChannelDbConnectionPoolTest.StressTestAsync awaited its TaskCompletionSource
  unconditionally, which hangs now that TryGetConnection can complete
  synchronously.
- 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.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mdaigle
mdaigle requested a review from a team as a code owner August 4, 2026 22:19
Copilot AI review requested due to automatic review settings August 4, 2026 22:19
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Aug 4, 2026
@mdaigle
mdaigle changed the base branch from dev/automation/channel-pool-transactions to dev/automation/channel-pool-v2-parity August 4, 2026 22:19

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

This PR closes parity gaps between ChannelDbConnectionPool (V2) and WaitHandleDbConnectionPool (V1) discovered via differential testing, focusing on correct pooling semantics and consistent diagnostics/metrics behavior across implementations.

Changes:

  • Emit pool metrics in the channel-based pool to match the wait-handle pool (pooled/free/active connection counters and connect/disconnect-related counters).
  • Fix ChannelDbConnectionPool.Count semantics to report tracked connections (slot occupancy) rather than in-flight reservations.
  • Add an async idle-connection fast path so OpenAsync can complete synchronously on a warm pool (excluding transactional requests), and update/parameterize tests accordingly.

Reviewed changes

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

Show a summary per file
File Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs Implements async idle fast path, fixes Count to use tracked connections, and wires metrics at key lifecycle points.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ConnectionPoolSlots.cs Adds ConnectionCount to distinguish tracked connections from reservations.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IdleConnectionChannel.cs Emits free-connection metrics on idle enqueue/dequeue to centralize counter correctness.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs Updates stress test to avoid hanging when async acquisition can complete synchronously.
src/Microsoft.Data.SqlClient/tests/ManualTests/TracingTests/MetricsTest.cs Parameterizes pooled connection metrics test across pool versions via ConnectionPoolVersionScope.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs Parameterizes resiliency SPID test across pool versions via ConnectionPoolVersionScope.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs Fixes Task.Factory.StartNew(async …) by unwrapping the nested task so failures/timeouts are observed correctly.

Comment on lines +659 to 662
pool.ReturnInternalConnection(internalConnection!, owningObject);

Assert.NotNull(internalConnection);
});
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