ChannelDbConnectionPool transaction support by mdaigle · Pull Request #4487 · dotnet/SqlClient

mdaigle · 2026-07-29T17:44:38Z

Stacked on #4429 (dev/mdaigle/replace-conn-2) — please review that one first; this PR only adds the transaction work on top.

Summary

Implements transaction support in ChannelDbConnectionPool, using WaitHandleDbConnectionPool as the reference for correct behavior. Before this change the channel pool constructed a TransactedConnectionPool but never used it, and three IDbConnectionPool members threw NotImplementedException.

Changes

PutObjectFromTransactedPool (was NotImplementedException) — returns a connection to general circulation once its transaction has ended, or destroys it if the pool is no longer running or the connection can't be pooled.
TransactionEnded (was NotImplementedException) — delegates to TransactedConnectionPool.TransactionEnded, which calls back into PutObjectFromTransactedPool.
ReturnInternalConnection — rewritten to mirror WaitHandleDbConnectionPool.DeactivateObject. It now deactivates first (deactivation is what detaches a completed transaction, so reading EnlistedTransaction beforehand could park a connection under an already-ended transaction), then decides under the connection lock between the transacted pool, stasis, the idle channel, and destruction. The idle-channel path moved into a new PutConnectionInIdleChannel helper.
GetFromTransactedPool (new) — vends a connection already enlisted in the ambient transaction. Transacted connections are exempt from idle-timeout and clear-generation eviction, since closing them would abort a possibly-distributed transaction, so only liveness is checked. A dead transaction root rethrows rather than silently retrying, because its delegated transaction cannot be recovered on another connection.
GetInternalConnection / PrepareConnection — consult the transacted pool when the pool group has transaction affinity, and pass the ambient transaction through to ActivateConnection.
Async acquisition path — enables ADP.SetCurrentTransaction(taskCompletionSource.Task.AsyncState as Transaction); SqlConnection already captures the ambient transaction in the TCS AsyncState, so the pooled open running on a thread-pool thread now sees it.
RemoveConnection — no longer disposes a transaction root that is still waiting for its delegated transaction to end (parity with DestroyObject). It comes back through PutObjectFromTransactedPool when the transaction completes.
ReplaceConnection — no functional change; the two TODO: Full transaction enlistment support (Story 2) markers from ChannelDbConnectionPool replace connection #4429 are removed now that enlistment is wired through.

Tests

New ChannelDbConnectionPoolTransactionTest (~35 tests) mirroring WaitHandleDbConnectionPoolTransactionTest: transacted routing, commit/rollback, nested Required/RequiresNew, mixed transacted/non-transacted workloads, shared transactions, pool saturation, two-thread contention, shutdown, and sequential-scope isolation. Adds channel-specific coverage for idle-channel return on completion, completion after shutdown, unknown-connection TransactionEnded, and enlistment carry-over through ReplaceConnection.
Removed the stale tests asserting NotImplementedException from the three now-implemented members.

Validation

Unit tests: 842 passed / 0 failed on net9.0 (324 in ConnectionPool).

Manual/integration tests were run against a local SQL Server with Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2 enabled, across TransactionEnlistmentTest, TransactionPoolTest, SQL.TransactionTest, ParallelTransactionsTest, ConnectionPoolTest, PoolBlockPeriodTest and DistributedTransactionTest (48 tests):

Run	Passed	Failed
V1 (WaitHandle) baseline	39	6
V2 without this change	36	9
V2 with this change	37	8

This change fixes three previously failing tests: TestAutoEnlistment_TxScopeNonComplete, TestManualEnlistment_Enlist and TestManualEnlistment_Enlist_TxScopeComplete.

The 6 failures shared with the V1 baseline are all PlatformNotSupportedException: This platform does not support distributed transactions — MSDTC isn't available on the test machine. The 2 remaining failures (ConnectionPoolTest.ReclaimEmancipatedOnOpenTest) are a pre-existing V2 gap: ReclaimEmancipatedObjects has never been implemented in ChannelDbConnectionPool. Both were confirmed by reverting this change and reproducing.

A broader V2 sweep (SqlCommand, AsyncTest, MARSTest, DataReaderTest, ConnectivityTests, WeakRefTest, AdapterTest, ExceptionTest, RetryLogic) gave 213 passed / 9 failed, where all 9 are named-pipe tests that fail identically on V1.

Ambient transaction flow on the async path

Transaction.Current does not flow into a Task.Run unless the TransactionScope was created with TransactionScopeAsyncFlowOption.Enabled (the default is Suppress, which keeps the ambient transaction in thread-static storage). The async open path therefore cannot simply read Transaction.Current — it has to take the transaction from the TaskCompletionSource's AsyncState, which is where SqlConnection.InternalOpenAsync captures it. That is the only site in the repo that constructs a TaskCompletionSource<DbConnectionInternal>, and it always passes the ambient transaction, so the mechanism is reliable (including across OpenAsyncRetry.Retry, which reuses the same TCS).

The original approach was to restore it by assigning ADP.SetCurrentTransaction(...) inside the Task.Run. That is unsafe here: assigning Transaction.Current writes to thread-static storage that ExecutionContext does not unwind, so the transaction outlives the open and is observable by unrelated work later scheduled onto the same thread pool thread — most notably the login-time auto-enlistment that non-pooled connections perform against Transaction.Current. A try/finally restore doesn't fix it either, because the async continuation may resume on a different thread than the one that was polluted. (WaitHandleDbConnectionPool does the same assignment safely because WaitForPendingOpen asserts it is not on a thread pool thread.)

The fix is to never mutate Transaction.Current in the pool: the transaction is captured on the caller's thread and threaded explicitly through GetInternalConnection into GetFromTransactedPool and PrepareConnection. The sync path passes ADP.GetCurrentTransaction() directly, since it runs on the caller's thread.

Three regression tests cover this: an async open with a transaction that is never ambient anywhere (proving the AsyncState path works without Transaction.Current), a check that repeated async opens leave no ambient transaction behind on any thread pool thread, and a sync open that must still pick up Transaction.Current from the caller's thread. The leak test was verified to fail when the SetCurrentTransaction call is reinstated.

Checklist

Tests added or updated
Public API changes documented — n/a, no public API change
Verified against customer repro (if applicable) — n/a
Ensure no breaking changes introduced — behavior is behind the existing UseConnectionPoolV2 switch, which defaults to off

Notes

ReclaimEmancipatedObjects remains unimplemented in the channel pool; it is orthogonal to transactions and left for a follow-up.

Ports the transacted-pool state machine from WaitHandleDbConnectionPool so the channel pool honors ambient System.Transactions enlistment: - Implement PutObjectFromTransactedPool and TransactionEnded (previously NotImplementedException). - Rewrite ReturnInternalConnection to mirror DeactivateObject: deactivate first, then route the connection to the transacted pool, stasis, the idle channel, or destruction under the connection lock. - Vend connections already enlisted in the ambient transaction via a new GetFromTransactedPool helper, and pass the transaction through to PrepareConnection/ActivateConnection. - Set the ambient transaction on the async acquisition path from the TaskCompletionSource's AsyncState. - Guard RemoveConnection against disposing a transaction root that is still waiting for its delegated transaction to end. Adds ChannelDbConnectionPoolTransactionTest mirroring the WaitHandle pool's transaction test suite, and drops the stale NotImplementedException tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Adds full transaction enlistment/routing support to the V2 ChannelDbConnectionPool, aligning behavior with the legacy WaitHandleDbConnectionPool so pooled connections correctly participate in ambient System.Transactions flows (including async acquisition).

Changes:

Implemented transaction lifecycle plumbing in ChannelDbConnectionPool (PutObjectFromTransactedPool, TransactionEnded, transacted acquisition path, and updated return/deactivation logic).
Enabled async acquisition to restore the captured ambient transaction on the worker thread (ADP.SetCurrentTransaction(...)).
Added a comprehensive unit test suite for channel-pool transaction behavior and removed the now-stale NotImplementedException assertions.

Reviewed changes

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

File	Description
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs	New transaction-focused unit tests for the channel-based pool, mirroring WaitHandle pool coverage and adding channel-specific scenarios.
src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs	Removes tests that asserted transaction methods were unimplemented (now implemented).
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Implements transaction support: transacted pool vending/parking, correct return paths for enlisted connections, and async ambient transaction propagation.

mdaigle · 2026-07-29T19:16:36Z

+        using var task1Returned = new ManualResetEventSlim(false);
+        using var task2Done = new ManualResetEventSlim(false);
+        DbConnectionInternal? connFromTask1 = null;
+        DbConnectionInternal? connFromTask2 = null;


Removed — that whole test is gone. It was the flaky two-thread test, which I dropped along with the other stress-style cases.

The async open path ran GetInternalConnection inside a Task.Run and restored the ambient transaction by assigning Transaction.Current on that thread pool thread. That assignment writes to thread-static storage which ExecutionContext does not unwind, so the transaction outlived the open and was observable by unrelated work later scheduled onto the same thread -- including the login-time auto-enlistment that non-pooled connections perform against Transaction.Current. A try/finally restore is not sufficient either, because the continuation may resume on a different thread than the one that was polluted. Instead, capture the ambient transaction on the caller's thread (from the TaskCompletionSource's AsyncState, which is where SqlConnection.OpenAsync puts it) and thread it explicitly through GetInternalConnection into GetFromTransactedPool and PrepareConnection. The sync path passes ADP.GetCurrentTransaction() directly since it runs on the caller's thread. Also gate the transaction on HasTransactionAffinity in one place so a pool without automatic enlistment neither reads from nor writes to the transacted store. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTransactionTest.cs:649

task2Done is declared but never used, which adds noise and makes the synchronization intent harder to follow. Remove it (or use it if it was meant to assert task2 completion).

        using var task2Done = new ManualResetEventSlim(false);

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:1206

GetInternalConnection creates a CancellationTokenSource even when a connection was successfully retrieved from the transacted pool, which adds avoidable allocations on that hot path. Consider returning early after GetFromTransactedPool succeeds so the CTS/loop is skipped entirely.

            // Derive a CancellationTokenSource from the TimeoutTimer so pool-internal wait operations
            // (channel reads, semaphore waits) are cancelled when the overall budget expires.
            using CancellationTokenSource cancellationTokenSource = timeout.CreateCancellationTokenSource();
            CancellationToken cancellationToken = cancellationTokenSource.Token;

mdaigle

Overall, the tests need a lot of cleanup. Verify pool count, idle, general stats and metrics at each step. Remove tests that are a strict subset of other tests. Add comments, think deeply about which tests are really required and provide good coverage. Use code coverage metrics to guide your decisions.

mdaigle · 2026-07-29T18:11:51Z

        {
-            throw new NotImplementedException();
+            Debug.Assert(connection is not null, "null connection?");
+            Debug.Assert(connection.EnlistedTransaction is null, "connection is still enlisted?");


improve these messages.

Done. Both now state the invariant and why it matters, e.g. the enlistment one explains that the transaction must have ended and been detached before the connection re-enters general circulation, otherwise it could be vended to a caller in a different transaction.

mdaigle · 2026-07-29T18:17:01Z

+            // If automatic transaction enlistment is enabled, we first try to get a connection that
+            // is already enlisted in the ambient transaction. If enlistment is not enabled we cannot
+            // vend connections from the transacted pool.
+            if (HasTransactionAffinity)


why not move this inside the loop?

Good call — moved inside the loop. It also fixes a real (if narrow) issue: if we loop for any other reason (e.g. we pulled a dead connection out of the idle channel and discarded it), the retry would previously skip the transacted store and open a fresh connection even though one enlisted in our transaction had become available in the meantime. Reusing the enlisted one is what avoids promoting the transaction to a distributed one.

It breaks out of the loop rather than falling through, because the liveness/idle/generation gate at the bottom must not apply to a transacted connection — GetFromTransactedPool has already probed liveness, and closing a transacted connection would abort a possibly distributed transaction.

mdaigle · 2026-07-29T18:48:33Z

+            // System.Transactions transaction object keeps it owned (not lost) and is certain to
+            // put it back into the pool. The decision is made under the connection lock so that a
+            // transaction completing asynchronously on another thread cannot race with us.
+            lock (connection)


this logic is all hopelessly complicated. simplify it

Agreed — it was a transcription of WaitHandleDbConnectionPool.DeactivateObject, nesting and all. Rewritten.

The three bool flags and the postcondition assert are gone, replaced by a ReturnDisposition enum (Reuse / Destroy / HeldByTransaction) returned from a single DecideReturnDisposition helper. The nesting collapses because the "shutting down", "transaction root with no pool", and "no longer poolable" branches all lead to the same two-line conclusion:

bool isReusable = State is Running && connection.CanBePooled && !(connection.IsTransactionRoot && connection.Pool is null); if (!isReusable) { if (connection.IsTransactionRoot) { connection.SetInStasis(); return HeldByTransaction; } return Destroy; } Transaction? transaction = connection.EnlistedTransaction; if (transaction is not null) { TransactedConnectionPool.PutTransactedObject(transaction, connection); return HeldByTransaction; } return Reuse;

Five nested branches down to three flat ones, with the same decision table. ReturnInternalConnection is now just deactivate, doom check, decide under the lock, then switch on the result outside it — which also makes it obvious that the I/O happens outside the connection lock.

mdaigle · 2026-07-29T18:50:19Z

            if (!LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior &&
                PoolGroupOptions.IdleTimeout != TimeSpan.Zero)
            {
                connection.SetReturnedTime(_timeProvider.GetUtcNow().UtcDateTime);


This means that transacted connections won't be subject to idle timeout. is this correct?

Correct, and intentional — it matches WaitHandleDbConnectionPool. A parked transacted connection is never proactively closed, because closing it would abort a transaction that may be distributed, so idle-timeout enforcement cannot apply while its transaction is live.

It is not permanently exempt though: when the transaction ends, TransactionEnded -> PutObjectFromTransactedPool routes it through PutConnectionInIdleChannel, which stamps SetReturnedTime then. So the idle clock starts from the moment the connection actually becomes available to other callers, which is the right semantics anyway — same reason we stamp on return rather than on checkout, so a long checkout does not count against the idle budget.

I added a comment on PutConnectionInIdleChannel spelling this out.

mdaigle · 2026-07-29T18:54:28Z

+    #region Transaction Routing Tests
+
+    [Fact]
+    public void GetConnection_UnderTransaction_RoutesToTransactedPool()


These test return behavior, not get. Also, add doc comments

Right — the pool decides routing at return time, not at get time. Renamed and regrouped: the routing tests are now ReturnConnection_WhileEnlisted_ParksInTransactedStoreNotIdleChannel, ReturnConnection_WithoutTransaction_ReturnsToIdleChannel, ReturnConnection_AfterTransactionCompleted_ReturnsToIdleChannel, ReturnConnection_WithTransactionAffinityDisabled_ReturnsToIdleChannel, and ReturnConnection_ToShutDownPool_DestroysConnection, all under a Connection Return Routing region. The tests that genuinely are about Get (vending an already-enlisted connection) are in a separate region and kept the GetConnection_ prefix.

Every test now has a doc comment stating the invariant it protects rather than restating the code.

mdaigle · 2026-07-29T18:59:32Z

+    }
+
+    [Fact]
+    public void DeeplyNestedTransactions_RequiresNew_AllTrackedSeparately()


what does this cover that others don't?

Nothing — it was NestedTransaction_RequiresNew_CreatesSeparateTransactedEntry with an extra level of nesting, so it exercised System.Transactions scope semantics rather than any additional pool behavior. Deleted.

mdaigle · 2026-07-29T18:59:44Z

+    }
+
+    [Fact]
+    public void DeeplyNestedTransactions_Required_AllShareOneEntry()


same question

Same answer — the Required variant just re-verified that TransactionScope returns the same ambient transaction, which is not our behavior to test. The pool-side invariant (same transaction gets the same connection) is covered by the round-trip test. Deleted.

mdaigle · 2026-07-29T19:00:18Z

+    #region Mixed Transacted and Non-Transacted Tests
+
+    [Fact]
+    public void MixedWorkload_AlternatingTransactedAndNonTransacted()


these are really stress tests. unit tests should target a specific behavior.

Agreed — deleted. MixedWorkload_AlternatingTransactedAndNonTransacted, AlternatingCommitAndRollback_MaintainsConsistentState, PoolSaturation_BlocksUntilConnectionAvailable and both TwoThreads_* tests are all gone. They were long-running, timing-sensitive, and their only assertion was that nothing had obviously fallen over. The suite is now 16 tests, each targeting one behavior, and runs in under 100ms.

mdaigle · 2026-07-29T19:01:16Z

+    public void PoolSaturation_BlocksUntilConnectionAvailable()
+    {
+        // Arrange - small pool
+        _pool.Shutdown();


what are you doing here?

Tearing down the fixture pool and building a smaller one, because the constructor already created a 50-connection pool. It read badly.

That particular test (pool saturation) is deleted — it was a timing test rather than a transaction test, and saturation is already covered in ChannelDbConnectionPoolTest. For the two tests that genuinely need different pool options there is now an explicitly named ReplaceFixturePool(bool hasTransactionAffinity) helper with a doc comment, so the intent is stated rather than inferred.

mdaigle · 2026-07-29T19:01:38Z

+        using var allAcquired = new ManualResetEventSlim(false);
+        using var releaseFirst = new ManualResetEventSlim(false);
+
+        var saturatingTask = Task.Run(() =>


make sure that you use fake time to prevent background processes from interfering.

Done — CreatePool now injects a frozen FakeTimeProvider, matching ChannelDbConnectionPoolReplaceConnectionTest. That stops idle-timeout pruning, warmup/replenishment and blocking-period expiry from advancing at all, so background maintenance cannot race the pool-state assertions.

The specific test this was on is deleted anyway (see above), but the fix applies suite-wide, which is the better place for it.

Source: - Flesh out the PutObjectFromTransactedPool asserts to say what invariant is being violated and why it matters. - Replace the five-way nested branch and three bool flags in ReturnInternalConnection with a ReturnDisposition enum and a single DecideReturnDisposition helper. The "shutting down", "transaction root with no pool" and "no longer poolable" cases all collapse into one reusability test followed by "stasis if it's a transaction root, otherwise destroy", which removes the need for the postcondition assert entirely. - Move the transacted-store lookup inside the acquisition loop so a connection returned to the store while we were looping is preferred over opening a fresh one. It breaks out of the loop to keep skipping the idle/generation gate, which must not apply to a transacted connection. - Document why a parked transacted connection is exempt from idle timeout and when its idle clock actually starts. Tests: - Rewrite the suite around specific behaviors: 16 focused tests replacing 33. Dropped the loop-driven stress tests (alternating commit/rollback, mixed workloads, deeply nested scopes, pool saturation, the flaky two-thread test) and the granular cases that were covered by a round trip. - Name the tests after the operation whose behavior they pin down, and document what invariant each one protects. - Assert full pool accounting (Count / IdleCount / transacted connections) at every step via AssertPoolState rather than spot-checking one collection. - Inject a frozen FakeTimeProvider so background maintenance cannot race the assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

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

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:260

The new XML doc for HasTransactionAffinity says connections may be "vended from (and parked in)" the TransactedConnectionPool when enabled. The code still parks connections based on connection.EnlistedTransaction in DecideReturnDisposition even when transaction affinity is disabled (e.g., manually enlisted connections), so the doc is misleading about the parking behavior. Consider rewording to clarify that this flag controls automatic transaction affinity (consulting the transacted pool / auto-enlisting on activation), not whether parking can occur at all.

        /// <summary>
        /// Indicates whether connections may be vended from (and parked in) the
        /// <see cref="TransactedConnectionPool"/>. This mirrors automatic transaction enlistment:
        /// when enlistment is disabled a connection is never bound to an ambient transaction, so
        /// the transacted store must not be consulted.
        /// </summary>
        private bool HasTransactionAffinity => PoolGroupOptions.HasTransactionAffinity;

Copilot AI review requested due to automatic review settings July 29, 2026 17:44

mdaigle requested a review from a team as a code owner July 29, 2026 17:44

github-project-automation Bot added this to SqlClient Board Jul 29, 2026

github-project-automation Bot moved this to To triage in SqlClient Board Jul 29, 2026

Copilot started reviewing on behalf of mdaigle July 29, 2026 17:45 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

mdaigle added this to the 7.1.0-preview3 milestone Jul 29, 2026

github-actions Bot assigned apoorvdeshmukh and cheenamalhotra Jul 29, 2026

mdaigle marked this pull request as draft July 29, 2026 18:08

Copilot AI review requested due to automatic review settings July 29, 2026 18:57

Copilot started reviewing on behalf of mdaigle July 29, 2026 18:57 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

mdaigle commented Jul 29, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 29, 2026 19:16

Copilot started reviewing on behalf of mdaigle July 29, 2026 19:16 View session

Copilot AI reviewed Jul 29, 2026

View reviewed changes

mdaigle mentioned this pull request Jul 29, 2026

Close remaining ChannelDbConnectionPool parity gaps with WaitHandleDbConnectionPool #4490

Draft

4 tasks

Uh oh!

Conversation

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

Uh oh!

Summary

Changes

Tests

Validation

Ambient transaction flow on the async path

Checklist

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

mdaigle 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!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdaigle commented Jul 29, 2026 •

edited

Loading