Enable ConnectionPoolV2 by default - #4537
Conversation
There was a problem hiding this comment.
Pull request overview
This PR flips the default of the internal Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2 AppContext switch to true, making the Channel-based pool (ChannelDbConnectionPool) the default connection pooling implementation while keeping the legacy pool (WaitHandleDbConnectionPool) available via explicit opt-out.
Changes:
- Change
LocalAppContextSwitches.UseConnectionPoolV2default fromfalse→true, and update its XML doc accordingly. - Update the unit test that asserts the default switch values.
- Update the internal feature documentation table for the switch default.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs | Flips UseConnectionPoolV2 default to true and updates XML documentation. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs | Updates default-value assertion to expect UseConnectionPoolV2 == true. |
| .github/instructions/features.instructions.md | Updates the documented default for UseConnectionPoolV2 in the AppContext switches table. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | `Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour` | `false` | Uses legacy async behavior for compatibility | | ||
| | `Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni` | `false` | Uses legacy SNI processing path | |
|
CI investigation: the two failing legs ( This is not a pre-existing flake: the same test passes on #4504's CI (same base branch, Root cause: Per this PR's stated purpose, I'm not fixing pool internals here — documented the gap in the PR body so the pool-v2 workstream can add reclaim-on-open support to |
Broader CI update: failures across nearly the full manual-test matrixAs more legs of 1.
|
4e920b5 to
6c1ea47
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/instructions/features.instructions.md:257
- The AppContext switch default-value table is now inconsistent with the actual defaults in
LocalAppContextSwitches.cs:UseCompatibilityAsyncBehaviourandUseCompatibilityProcessSniboth default totrue(compatibility mode), but the table still listsfalse. Since this PR already edits this section, please update these rows so the table reflects real defaults and explains that setting them tofalseenables the newer behaviors.
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour` | `false` | Uses legacy async behavior for compatibility |
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni` | `false` | Uses legacy SNI processing path |
| `Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2` | `true` | Enables the new `ChannelDbConnectionPool` implementation; set to `false` to restore the legacy `WaitHandleDbConnectionPool` |
513c4d3 to
6cf4e79
Compare
6c1ea47 to
ee208e9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/instructions/features.instructions.md:257
- The switch-default table still lists
UseCompatibilityAsyncBehaviourandUseCompatibilityProcessSnias defaulting tofalse, but both are asserted astruedefaults inLocalAppContextSwitchesTestand documented/implemented asdefaultValue: trueinLocalAppContextSwitches.cs. This table should be updated to avoid misleading contributors about the actual defaults.
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour` | `false` | Uses legacy async behavior for compatibility |
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni` | `false` | Uses legacy SNI processing path |
| `Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2` | `true` | Enables the new `ChannelDbConnectionPool` implementation; set to `false` to restore the legacy `WaitHandleDbConnectionPool` |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/instructions/features.instructions.md:257
- The switch default-value table is inconsistent with the actual defaults in
LocalAppContextSwitches.cs: bothUseCompatibilityAsyncBehaviourandUseCompatibilityProcessSnidefault totrue(seeLocalAppContextSwitches.cs:539-575), but this table still lists them asfalse. This makes the switch reference misleading, especially now thatUseConnectionPoolV2is being updated here as well.
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour` | `false` | Uses legacy async behavior for compatibility |
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni` | `false` | Uses legacy SNI processing path |
| `Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2` | `true` | Enables the new `ChannelDbConnectionPool` implementation; set to `false` to restore the legacy `WaitHandleDbConnectionPool` |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs:588
- The PR description calls out a missing “reclaim emancipated connections on open” path in PoolV2 as the primary root cause of CI failures, but
ChannelDbConnectionPool.GetInternalConnectionalready performs a reclaim sweep on the slow path before it parks on the idle channel (seeChannelDbConnectionPool.cs:1516-1526,ReclaimEmancipatedConnections()). Either the investigation summary is out of date, or the problem is that reclamation is not triggering / not freeing a usable connection; please update the PR description (or add a note) so the documented root cause matches the current code.
/// </summary>
public static bool UseConnectionPoolV2 =>
AcquireAndReturn(
UseConnectionPoolV2String,
defaultValue: true,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/instructions/features.instructions.md:257
- The switch-default table now claims
UseCompatibilityAsyncBehaviourandUseCompatibilityProcessSnidefault tofalse, butLocalAppContextSwitchesdefaults both totrue(see LocalAppContextSwitches.cs:557 and :575). Since this PR updates this table, it should keep these defaults accurate to avoid misleading guidance.
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityAsyncBehaviour` | `false` | Uses legacy async behavior for compatibility |
| `Switch.Microsoft.Data.SqlClient.UseCompatibilityProcessSni` | `false` | Uses legacy SNI processing path |
| `Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2` | `true` | Enables the new `ChannelDbConnectionPool` implementation; set to `false` to restore the legacy `WaitHandleDbConnectionPool` |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs:72
Task.Factory.StartNew(..., LongRunning).Unwrap()is unnecessary here and adds thread-creation overhead;RunPacketNumberWraparoundis already async and can be started directly and raced against the timeout task.
// Task.Factory.StartNew with an async delegate returns a Task<Task>, so it must be
// unwrapped before use in Task.WhenAny below. Without Unwrap(), WhenAny would observe
// only the outer task (which completes as soon as the async lambda hits its first
// await) instead of the actual completion of RunPacketNumberWraparound.
Task actionTask = Task.Factory.StartNew(
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs:86
- If the timeout wins, any fault from
actionTaskis never observed (and the task may keep running briefly after test failure), which can hide the real failure cause and leak background work into later tests. Consider best-effort observing faults after cancellation on the timeout path.
// Propagate any unexpected failure from the action task (e.g. a connection open
// failure) instead of letting it surface only as a low enumerator count below.
if (completedTask == actionTask)
{
await actionTask;
d80ca68 to
ead2ea3
Compare
| Task completedTask = await Task.WhenAny(actionTask, timeoutTask); | ||
|
|
||
| stopwatch.Stop(); | ||
| cancellationTokenSource.Cancel(); | ||
|
|
||
| // Propagate any unexpected failure from the action task (e.g. a connection open | ||
| // failure) instead of letting it surface only as a low enumerator count below. | ||
| if (completedTask == actionTask) | ||
| { | ||
| await actionTask; | ||
| } |
Use unique pool-group keys for ephemeral simulated servers and isolate login-token handling from fatal connection-break behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs:2241
- The XML doc comment for CountingTimeoutConnectionFactory.CreateConnection says it always throws the pooled-open timeout, but the implementation now throws a supplied marker exception when provided. Update the comment to match the new behavior.
TimeoutTimer timeout)
{
CreateCount++;
throw _exception ?? ADP.PooledOpenTimeout();
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ParameterTest/TvpTest.cs:74
- Task.Factory.StartNew without specifying TaskScheduler.Default uses TaskScheduler.Current, which can be a non-default scheduler under test frameworks and lead to unexpected scheduling for a LongRunning task. Other tests in this repo pass TaskScheduler.Default explicitly (e.g., ChannelDbConnectionPoolWarmupTest.cs:190-204).
Task actionTask = Task.Factory.StartNew(
() => RunPacketNumberWraparound(enumerator, cancellationTokenSource.Token),
TaskCreationOptions.DenyChildAttach | TaskCreationOptions.LongRunning).Unwrap();
Observe asynchronous stress workers so connection failures fail the test instead of terminating the test host, and quarantine the sync variant of the known transient retry timing flake. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| workers[ConcurrentConnections - 1] = CreateWorkerTask( | ||
| connectionString, command, barrier, doomConnections: true, async, doomAction); | ||
| } | ||
|
|
||
| // Start all threads | ||
| foreach (Thread thread in threads.Where(t => t != null)) | ||
| { | ||
| thread.Start(); | ||
| } | ||
|
|
||
| // Wait for completion | ||
| countdown.Wait(); | ||
| Task.WhenAll(workers).GetAwaiter().GetResult(); |
Count completed failover logins so abandoned pre-login transport attempts do not obscure the fresh physical connection created after the pool is cleared. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolStressTest.cs:199
workersis allocated withConcurrentConnectionselements but whenConcurrentConnections == 1no worker is ever assigned (theforloop does not run and theif (ConcurrentConnections > 1)block is skipped). This leaves a null element in the array andTask.WhenAll(workers)will throw (ArgumentException) before the stress test actually runs.
if (ConcurrentConnections > 1)
{
workers[ConcurrentConnections - 1] = CreateWorkerTask(
connectionString, command, barrier, doomConnections: true, async, doomAction);
}
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolStressTest.cs:184
- RunStressTest allocates a Task[] sized to ConcurrentConnections but does not populate any entry when ConcurrentConnections == 1 (the for-loop runs 0 iterations and the dooming worker is only created when > 1). This leaves a null in the array and causes Task.WhenAll(workers) to throw immediately, potentially leaving started workers unobserved.
var workers = new Task[ConcurrentConnections];
using Barrier barrier = new(ConcurrentConnections);
var command = string.IsNullOrWhiteSpace(WaitForDelay)
? "SELECT GETDATE()"
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolStressTest.cs:184
workersis sized toConcurrentConnections, but whenConcurrentConnections == 1neither theforloop nor the dooming-worker branch assignsworkers[0], soTask.WhenAll(workers)will throw due to a null task. Either validateConcurrentConnections >= 2up front or populate all slots and conditionally mark one worker as the dooming worker.
var workers = new Task[ConcurrentConnections];
using Barrier barrier = new(ConcurrentConnections);
var command = string.IsNullOrWhiteSpace(WaitForDelay)
? "SELECT GETDATE()"
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectionPoolTest/ConnectionPoolStressTest.cs:191
RunStressTestleavesworkers[0]as null whenConcurrentConnections == 1(the loop runs 0 iterations and the dooming worker is only created when> 1).Task.WhenAll(workers)will then throwArgumentException/NullReferenceExceptioninstead of running the stress test. Create all worker tasks in a single loop and only enable the dooming behavior on the last worker whenConcurrentConnections > 1.
// Create regular threads (don't doom connections)
for (int i = 0; i < ConcurrentConnections - 1; i++)
{
workers[i] = CreateWorkerTask(
connectionString, command, barrier, doomConnections: false, async);
CI results summary
This PR flips
UseConnectionPoolV2totrueso the full CI matrix runs againstChannelDbConnectionPoolby default. CI exposed these V1/V2 differences and latent test issues:ReclaimEmancipatedOnOpenTestexposed missing reclaim-on-open behavior in V2. The fix merged through Channel Pool: Reclaim leaked connections #4529 and is now onmain.TvpTest.TestPacketNumberWraparoundexposed a latentTask<Task>test bug. Commit6941f0c63unwraps the task and propagates early failures correctly.AmbientTransactionFailureTestexposed a V2 timeout race on Windows. Commit00e17ff4fpreserves the physical connection error instead of replacing it with a pool-exhaustion timeout.d0e0ae2c2isolates pool-group metadata for ephemeral ports and makes login-token assertions independent of fatal connection-break timing.async voidworkers that terminated the test host when SQL Server reset connections. Commit680e297fdobserves all worker tasks and reports their exceptions through the test. It also quarantines the synchronous variant of the existing simulated transient-retry timing flake after the same failure appeared on Windows x86.87ccff741verifies completed failover logins, preserving the fresh-connection contract without treating abandoned transport attempts as extra physical connections.6c3ee9b7ewidens the sync and async retry-disabled test budgets to 10 seconds. These tests verify retry behavior, not timeout precision.07bb497aequarantines the retry-enabled sibling.45b28ed95acquires through the normal pool path when the old connection is already detached and adds deterministic max-pool-size coverage.The clean matrix on
07bb497aeproduced:ConnectionResiliencySPIDTestexposed the detached-old-connection V2 regression corrected by45b28ed95.The clean matrix on
45b28ed95completed all 366 checks:Identity not foundfor its managed identity. All other package checks passed.Summary
Changes the default connection pool from
WaitHandleDbConnectionPooltoChannelDbConnectionPool. Applications can restore the legacy pool explicitly:Changes
UseConnectionPoolV2's default totrue.TvpTest.TestPacketNumberWraparoundsoTask.Factory.StartNew's nested task is unwrapped and observed.Performance comparison
An interleaved best-of-three comparison covered 162 benchmarks with a 10% threshold:
Confirmed regressions
The strongest remaining performance signals are fixed per-call overhead in pooled
OpenAsync, allocation and synchronization cost in rapid open/close loops, and contention whenMaxPoolSizeis small relative to concurrency. Higher-concurrency workloads with adequately sized pools generally improved.Validation
ChannelDbConnectionPoolTest: 64/64 passed on net8.0, net9.0, and net10.0 before the rebase; 64/64 passed on net9.0 after rebasing ontomain.ChannelDbConnectionPoolReplaceConnectionTest: 16/16 passed on net8.0 and net9.0 after adding detached-old-connection coverage.AmbientTransactionFailureTest: 2/2 passed locally on net8.0 and net9.0 before the rebase; 2/2 passed on net8.0 after rebasing.TvpTest.TestPacketNumberWraparoundpassed after the test fix.NetworkError_TriggersFailover_ClearsPoolpasses locally on net9.0.NetworkDelay_RetryDisabledsync and async theories passed 40/40 executions across net8.0 and net9.0.Suggested release note
Checklist