Skip to content

Reclaim emancipated connections in ChannelDbConnectionPool - #4490

Draft
mdaigle wants to merge 45 commits into
dev/automation/channel-pool-transactionsfrom
dev/automation/channel-pool-v2-parity
Draft

Reclaim emancipated connections in ChannelDbConnectionPool#4490
mdaigle wants to merge 45 commits 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.

The remaining parity fixes I found (metrics, Count semantics, async idle fast path) are stacked on top of this one in a follow-up PR, so this one stays focused on reclamation.

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. This was the most serious of the gaps it turned up.

A SqlConnection that is garbage collected without ever being closed or disposed leaves its internal connection emancipated: still tracked by the pool, but with no owner that could ever return it. WaitHandleDbConnectionPool sweeps for these before waiting for a free connection; ChannelDbConnectionPool did not. So an emancipated connection permanently occupied a pool slot, and at MaxPoolSize every subsequent Open timed out — forever, not just once. Leaking a single connection was enough to eventually wedge the whole pool.

What

GetInternalConnection now performs the same sweep just before parking on the idle channel. It's deliberately confined to the slow path: it's O(MaxPoolSize) and allocates a snapshot, so it must not run on the hot acquire path.

A couple of details worth calling out for review:

  • The sweep takes the connection lock with Monitor.TryEnter rather than Enter. IsEmancipated has to be read under that lock to avoid racing PrePush/PostPop, but a connection that is currently locked is being actively handed out or returned and therefore isn't emancipated anyway — so skipping it costs nothing and keeps the sweep from blocking the caller.
  • Only PrePush happens under the lock. Deactivation can make server round trips, so it's deferred until all locks are released.
  • Deactivating and routing a returned connection is factored out of ReturnInternalConnection into DeactivateAndRouteConnection so reclamation can share it. Reclamation can't just call ReturnInternalConnection, because it has already done the PrePush and there's no owning object left to validate against.

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 ReclaimEmancipatedOnOpenTest and MaxPoolWaitForConnectionTest by pool version. ReclaimEmancipatedOnOpenTest fails against ChannelDbConnectionPool without this fix and passes with it.
  • 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.

Verification

Ran all three suites under both pools on net9.0/managed SNI against SQL Server. With the full stack applied the failure sets are identical between V1 and V2 apart from TestDefaultAppContextSwitchValues, which necessarily fails when the switch is globally on.

For this PR in isolation, the pool, transaction, resiliency and metrics test classes all pass with the switch in its default (off) state, and ReclaimEmancipatedOnOpenTest passes explicitly under both pool versions.

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

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

mdaigle and others added 30 commits July 13, 2026 16:38
Introduce an optional System.Threading.RateLimiting policy that throttles new physical connection opens in the channel pool: when a permit is denied the caller waits for a returned connection instead of forcing a create, and leases are always released (including on failure) to avoid starvation. Adds NoOpAcquiredLease, wires the RateLimiting package into the product and test projects, and includes the 006-pool-rate-limiting spec. Also repairs two pre-existing build breaks in ChannelDbConnectionPoolTest (a dropped CountingSuccessfulConnectionFactory declaration and DbConnectionPoolGroupOptions calls missing the new idleTimeout argument).
The connection pool only needs a concurrency limiter (pooling against
on-prem SQL Server), so change ChannelDbConnectionPool to take a concrete
System.Threading.RateLimiting.ConcurrencyLimiter? instead of the abstract
RateLimiter base. The limiter remains optional (null = no limiting), and
AttemptAcquire(1)/RateLimitLease usage is unchanged (both inherited).

Rework the three rate-limiter unit tests to use real ConcurrencyLimiter
instances and assert via GetStatistics() (CurrentAvailablePermits,
TotalFailedLeases) instead of the now-removed TestRateLimiter double.

Update the spec and diagram to describe a concurrency limiter specifically,
noting other limiter types can be added later if needed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the two "options to consider" TODOs above the AttemptAcquire call and
replace them with a comment explaining why non-blocking fast-fail was chosen
over failing immediately or blocking on the limiter.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Drop the two "options to consider" TODOs above the AttemptAcquire call. The
rationale for choosing non-blocking fast-fail lives in the PR discussion
rather than in code.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the redundant leaseAcquired local; read lease.IsAcquired directly in
the early-return guard and the finally-block poke condition.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
RateLimiter_SuccessfulCreate_ReleasesLeaseForNextCreate exercises a
single-permit ConcurrencyLimiter with two sequential opens against distinct
owners. A leaked lease on the success path would deny the second open, so
asserting both create physical connections (CreateCount == 2) guards the
release-on-success behavior at the behavioral level rather than only via the
permit counter.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Cover the previously untested concurrency behavior where a caller blocked
purely by rate limiting is woken by another caller's lease release (the
finally-block null poke) and then creates its own physical connection.

RateLimiter_LeaseReleaseWakesRateLimitedWaiter_CreatesPhysicalConnection is a
[Theory] over the sync and async idle-channel wait mechanisms. It uses a new
GatedSuccessfulConnectionFactory that blocks the first physical create so the
permit is held in-flight while a second caller is denied and parks on the idle
channel; releasing the gate triggers the release poke that must wake and
satisfy the waiter. Verified the test fails (waiter times out) when the poke
is disabled.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…osal

- Exclude OperationCanceledException from the creation-failure catch so a
  caller's own timeout/cancellation no longer poisons the pool blocking period.
- Gate the finally idle-channel poke to non-faulted completion via a faulted
  flag, avoiding a redundant double wake on exception paths (cleanupCallback
  already writes a wake).
- Document that the pool does not own the injected ConcurrencyLimiter and never
  disposes it (caller owns its lifetime).
- Fix comment typo (rather then -> rather than) and trailing whitespace.
- Reword spec User Story 1 / FR-002 from strict FIFO to best-effort idle-channel
  wait, matching the non-blocking AttemptAcquire implementation.
- Dispose ConcurrencyLimiter instances in tests (using var) and drop the unused
  System.Collections.Generic using.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A blank line inside the <remarks> block was missing its '///' prefix,
causing CS1570 and breaking the build.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…onnection

ReplaceConnection now tries GetIdleConnection() before establishing a new
physical connection. When a live idle connection is available it is checked
out and activated under the old connection's ambient transaction, then the
replaced connection's slot is freed and it is disposed. This avoids an
unnecessary physical connect and keeps the reserved slot count strictly
decreasing, so the pool never exceeds MaxPoolSize.

When no idle connection is available the previous create-and-swap path is
used unchanged. In both paths the old connection is left untouched until the
replacement is activated, so a failure leaves it reusable by the caller's
reconnect retry loop.

Adds ReplaceConnection_PrefersIdleOverNewConnection and
ReplaceConnection_IdleReuse_AtMaxCapacity_FreesOldSlot unit tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Passing the boolean forceNewConnection flag positionally as a bare true/false
obscures intent at the call site. Name the argument at every literal call site
so the open/reconnect paths read clearly.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…eConnection

The idle-reuse branch of ReplaceConnection previously deactivated and removed
the reused connection if activation failed, unconditionally discarding a
connection that was healthy moments earlier. Route the failure through
ReturnInternalConnection instead so a still-healthy connection is re-pooled
and only a genuinely dead one is removed, matching the normal get path.

Since the reuse branch's check-out + activate + return-on-failure is now
identical to PrepareConnection, call PrepareConnection directly to remove the
duplication.

Adds ReplaceConnection_IdleReuse_ActivationFails_ReturnedToPool.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Trim the large explanatory comments in ReplaceConnection so they no longer
dominate the method, keeping the non-obvious rationale (slot accounting,
reuse-on-failure, never over MaxPoolSize) in a couple of lines each.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Consolidate the scattered per-branch rationale in ReplaceConnection into a
single header comment explaining the two invariants that shape the method
(forward progress under pool saturation via atomic reservation handoff, and
oldConnection as the failure anchor) and why the create branch cannot delegate
to PrepareConnection. Slim the inline branch comments to short pointers so the
control flow reads cleanly.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…nel-rate-limiting

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…e/replace-conn-2

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Revert the named-to-positional reversions that crept into the TryOpenInner
call sites in SqlConnectionConcurrentOpenTests and SqlConnectionStateTransitionTests,
restoring the readable forceNewConnection: false/true form to match the rest of
the branch and the call sites on main. Also restore the accidental missing space
after the comma in the TryOpenWithRetry parameter list.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
mdaigle and others added 14 commits July 15, 2026 17:22
- Remove stray blank doc line that split the forceNewConnection <remarks>
  sentence into two paragraphs in generated docs.
- Correct the TestReplaceConnection summary (it no longer asserts
  NotImplementedException) and move it out of the 'Not Implemented Method
  Tests' region into a dedicated 'Replace Connection Tests' region.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Add a class-level XML summary to ChannelDbConnectionPoolReplaceConnectionTest
  describing the behavior under test.
- Replace the try/catch that swallowed the expected InvalidOperationException in
  ReplaceConnection_ActivationFails_NewConnectionReturnedToPool with an explicit
  Assert.Throws, matching the sibling failure-path tests and making the intent
  fail-safe.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…path

The create branch of ReplaceConnection now honors the pool's blocking-period
error state (ThrowIfActive) before opening a new physical connection and clears
the backoff ramp on a successful open, mirroring OpenNewInternalConnection. Idle
reuse stays exempt, and a reconnect failure still does not enter the error state
by design, so a targeted reconnect cannot poison the pool.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…onn-2

Resolve conflicts in the connection-pool area:
- NoOpAcquiredLease.cs: keep main's fuller doc comments (code identical).
- ChannelDbConnectionPool.cs: take main's refined rate-limiting/blocking-period
  and background-warmup code; the branch's ReplaceConnection implementation and
  PrepareConnection transaction parameter live in non-conflicting regions.
- ChannelDbConnectionPoolTest.cs: adopt main's consolidated/deterministic
  blocking-period and rate-limiter tests; drop the now-obsolete TestReplaceConnection
  stub (ReplaceConnection is implemented and covered by
  ChannelDbConnectionPoolReplaceConnectionTest.cs).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Localize the "slot could not be replaced" guard in
  ChannelDbConnectionPool.ReplaceConnection: replace the hard-coded
  InvalidOperationException with ADP.InternalError using a new
  InternalErrorCode.ConnectionSlotReplacementFailed (still an
  InvalidOperationException, so behavior is unchanged).
- Correct the TryOpenInner forceNewConnection XML remarks: the flag is
  also valid when the connection was previously opened and is now
  disconnected (the reconnect path via DbConnectionClosedPreviouslyOpened /
  DbConnectionClosedConnecting), not only when already open. Also removes a
  stray blank doc line by using <para> blocks.
- Fix the activation-failure test so its name, summary, and inline comment
  match the implementation: the new connection is disposed (never slotted)
  and the old connection is left intact, so pool count is unchanged.
- Remove unused usings (Microsoft.Data.Common,
  Microsoft.Data.Common.ConnectionString) from the ReplaceConnection tests.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the opaque ADP.InternalError(ConnectionSlotReplacementFailed) at the
ReplaceConnection !replaced guard with a localized InvalidOperationException
(SQL_ConnectionPoolReplaceConnectionFailed). Removes the now-unused
InternalErrorCode.ConnectionSlotReplacementFailed enum value.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Document that the ReplaceConnection create branch intentionally skips
_connectionCreationRateLimiter: a replacement is a 1-for-1 swap (not pool
growth) and must make forward progress for an already checked-out caller's
reconnect, so the limiter's fast-fail-then-wait-for-idle contract does not apply.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The new ReplaceConnection tests built pools on TimeProvider.System, letting
time-driven background maintenance (idle-timeout pruning, warmup/replenishment,
blocking-period expiry) advance in real time and potentially race the
assertions. Thread a frozen FakeTimeProvider through the replacement test
helper (default) and the TestReplaceConnection case so the pool clock only
moves when a test drives it.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Brings in the completed connection-pool pruning work (Story 2/3/4, #4463),
which reworks PoolPruner to be driven by Connection Idle Timeout and only
constructs a Pruner when IdleTimeout != 0. The single overlapping file,
ChannelDbConnectionPool.cs, auto-merged cleanly: main's constructor pruner
block coexists with this branch's ReplaceConnection additions. Also pulls in
#4460 (unobserved-exception repro), #4347 (vector test refactor), and #4459
(pool benchmark coverage).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Mirror WaitHandleDbConnectionPool: when the physical open of a
replacement connection fails, enter the blocking-period error state so
subsequent opens fast-fail until it expires. Activation failures are
excluded (the server proved reachable), matching the WaitHandle pool
where PrepareConnection runs outside CreateObject's error-state catch.

Adds two tests and updates the creation-failure retry test to reflect
that the failed open now enters the blocking period.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- ConnectionPoolSlotsTest: move null-forgiveness to the Add() assignment
  instead of every use site; confirm the untouched occupant survives a
  failed TryReplace; add a self-replace test (benign no-op).
- SqlConnection: name all args in the Open overrides ternary; drop a
  stray blank line.
- ReplaceConnection tests: assert the replacement is not the old
  connection; assert the blocking-period throw is the same cached
  exception instance (with the factory flipped back to succeeding to
  prove the create path never ran).
- Collapse the three test factories into one TunableSqlConnectionFactory
  (FailOnCreate/FailOnActivate) and fold ActivationFailDbConnectionInternal
  into StubDbConnectionInternal, which now reads the factory's flag live
  so idle-reuse tests can toggle it after creation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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>
@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
A SqlConnection that is garbage collected without ever being closed or disposed
leaves its internal connection "emancipated": still tracked by the pool, but
with no owner that could ever return it. WaitHandleDbConnectionPool sweeps for
these before waiting for a free connection; ChannelDbConnectionPool did not, so
an emancipated connection permanently occupied a pool slot. At MaxPoolSize that
meant every subsequent Open timed out -- forever, not just once.

GetInternalConnection now performs the same sweep just before parking on the
idle channel. This is deliberately confined to the slow path: it is
O(MaxPoolSize) and allocates a snapshot, so it must not run on the hot acquire
path.

The sweep takes the connection lock with Monitor.TryEnter rather than Enter.
IsEmancipated has to be read under that lock to avoid racing PrePush/PostPop,
but a connection that is currently locked is being actively handed out or
returned and therefore is not emancipated anyway, so skipping it costs nothing
and keeps the sweep from blocking the caller. Only PrePush happens under the
lock; deactivation, which can make server round trips, is deferred until all
locks are released.

Deactivating and routing a returned connection is now factored out of
ReturnInternalConnection into DeactivateAndRouteConnection so reclamation can
share it. Reclamation must not go through ReturnInternalConnection itself
because it has already performed the PrePush and there is no owning object left
to validate against.

Tests:

- 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 and MaxPoolWaitForConnectionTest by
  pool version. ReclaimEmancipatedOnOpenTest fails against ChannelDbConnectionPool
  without this fix.
- 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 what they meant anyway.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@mdaigle
mdaigle force-pushed the dev/automation/channel-pool-v2-parity branch from 04b8001 to ca8079b Compare August 4, 2026 22:17
Copilot AI review requested due to automatic review settings August 4, 2026 22:17
@mdaigle mdaigle changed the title Close remaining ChannelDbConnectionPool parity gaps with WaitHandleDbConnectionPool Reclaim emancipated connections in ChannelDbConnectionPool Aug 4, 2026

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

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Generated file
Suppressed comments (1)

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

  • ReclaimEmancipatedConnections logs reclaimed connections but does not emit the corresponding metrics counter. WaitHandleDbConnectionPool.ReclaimEmancipatedObjects calls SqlClientDiagnostics.Metrics.ReclaimedConnectionRequest() for each reclaimed connection; missing this in ChannelDbConnectionPool means reclaimed-connection metrics remain incorrect under V2.
                SqlClientEventSource.Log.TryPoolerTraceEvent(
                    "<prov.DbConnectionPool.ReclaimEmancipatedObjects|RES|CPOOL> {0}, Connection {1}, Reclaiming.",
                    Id,
                    connection.ObjectID);

                connection.DetachCurrentTransactionIfEnded();
                DeactivateAndRouteConnection(connection);

// Use 3-phase synchronization so task1 gets AND returns before task2 requests.
// This ensures the connection is back in the transacted pool for task2 to reuse.
using var task1Returned = new ManualResetEventSlim(false);
using var task2Done = new ManualResetEventSlim(false);
Comment on lines +1 to +6
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace Microsoft.Data.SqlClient.Tests.Common;

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