Background pool warmup and replenishment for ChannelDbConnectionPool#4452
Draft
mdaigle wants to merge 16 commits into
Draft
Background pool warmup and replenishment for ChannelDbConnectionPool#4452mdaigle wants to merge 16 commits into
mdaigle wants to merge 16 commits into
Conversation
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>
…nel-rate-limiting
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>
…nel-rate-limiting Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- NoOpAcquiredLease: document that the shared Instance is stateless and its Dispose is an idempotent no-op, safe to dispose repeatedly/concurrently. - ChannelDbConnectionPool: add a class-level pointer to the feature spec so the FR-0xx requirement tags in comments are resolvable; note at lease.Dispose() that the no-limiter case disposes the idempotent singleton; document the faulted flag and how the finally uses it to avoid a redundant idle-channel poke. - Tests: merge the three ErrorOccurred_* blocking-period facts into a single Theory parameterized by the connection string's Pool Blocking Period; comment that a rising TotalFailedLeases is how the wake test detects requestB was denied a permit and parked on the idle channel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replaces the hollow SuccessfulCreate_AfterFailure_ClearsErrorState test (which called pool.Clear() and made its final assertion vacuous) with Failure_ThenBlockingPeriodExpiry_AllowsSuccessfulCreate. The new test drives the blocking-period exit timer deterministically via an injected FakeTimeProvider, verifying the real recovery path: failure enters the blocking period, in-window requests fast-fail without touching the factory, and once the backoff elapses a create succeeds and clears the error state. Adds an optional TimeProvider parameter to the ChannelDbConnectionPool constructor (forwarded to BlockingPeriodErrorState) and to the ConstructPool test helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The ConcurrencyLimiter field doc previously suggested a limiter could be shared across pools. Because the permit-release wake is signaled on this pool's own idle channel, a permit released by another pool would not wake waiters parked here. Reword the doc to state the limiter is expected to be scoped to a single pool and that cross-pool sharing is unsupported, while keeping the caller-owns-lifetime note. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The permit-release poke condition read lease.IsAcquired after lease.Dispose() had run. Even though the current ConcurrencyLimiter lease keeps IsAcquired stable post-dispose, using an object after disposal is fragile if the lease implementation changes. Capture the value into a local before disposing and use the captured bool in the poke condition. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ctionPool Adds serial, rate-limited background warmup that pre-creates connections up to Min Pool Size on Startup and replenishes whenever the pool drops below the minimum (destroy-on-return, idle-timeout eviction, pruning). Warmup failures are absorbed without triggering the pool error state, warmup is coalesced to a single loop, cancels promptly on shutdown, and stops on generation change. Implements specs/003-pool-warmup/spec.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements background warmup and automatic replenishment for ChannelDbConnectionPool so pools proactively create connections up to MinPoolSize and restore back to the minimum after below-minimum events, without blocking callers.
Changes:
- Add a coalesced, serial warmup/replenishment loop driven by
Startup()andRemoveConnection()triggers, using the shared connection-creation rate limiter. - Extend
OpenNewInternalConnectionwith anisWarmupmode that avoids blocking-period error-state interaction and absorbs warmup failures. - Add a new unit test suite (
ChannelDbConnectionPoolWarmupTest) and a feature spec document (specs/003-pool-warmup/spec.md).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | Adds warmup/replenishment loop plumbing, shutdown cancellation, and warmup-specific connection creation behavior. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolWarmupTest.cs | New unit tests covering warmup, shared rate limiter behavior, failure resilience, shutdown cancellation, and replenishment triggers. |
| specs/003-pool-warmup/spec.md | New spec documenting scenarios and acceptance criteria for the warmup feature. |
Comment on lines
+1067
to
+1072
| // No-op when there is nothing to pre-create (MinPoolSize == 0), the pool is not running, | ||
| // or shutdown has cancelled background activity. | ||
| if (MinPoolSize == 0 || State != Running || _warmupCts.IsCancellationRequested) | ||
| { | ||
| return; | ||
| } |
Comment on lines
+1196
to
+1201
| if (connection is null) | ||
| { | ||
| // A slot is guaranteed available here (Count < MinPoolSize <= MaxPoolSize), so a | ||
| // null return means the shared rate limiter is currently saturated. Wait our | ||
| // turn rather than bypassing it (Story 2), then retry. Capacity frees up as user | ||
| // requests release their permits. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements background pool warmup and replenishment for
ChannelDbConnectionPool, per the newspecs/003-pool-warmup/spec.md. Stacks on the pool rate-limiting work (base:dev/mdaigle/pool-channel-rate-limiting).On
Startup(), the pool asynchronously pre-creates connections up to Min Pool Size, serially (one at a time) through the shared rate limiter used by user requests. Whenever the pool drops below Min Pool Size for any reason (destroy-on-return, idle-timeout eviction, pruning), it automatically replenishes through the same path.Implementation
RequestWarmup()— single coalesced entry point; no-op whenMinPoolSize == 0, not running, or shutting down.TryStartWarmupLoop()/RunWarmupLoopAsync()— CAS-guarded single background loop (coalesces concurrent requests); absorbs all exceptions so nothing escapes onto the thread pool.WarmupPassAsync()— creates connections serially up toMinPoolSizeviaOpenNewInternalConnection(isWarmup: true); a null return means the rate limiter is saturated, so it waits its turn rather than bypassing it; publishes fresh connections to the idle channel; stops on generation change (Clear) and on cancellation.OpenNewInternalConnectiongains anisWarmupflag that suppresses all blocking-period error-state interaction and converts creation failures into a tracednull(warmup failures never poison the pool).Startup()→ warmup;RemoveConnection()→ replenish (the single choke point for all below-minimum events);Shutdown()cancels + disposes the warmup CTS.Testing
ChannelDbConnectionPoolWarmupTest.cs, 13 tests covering Stories 1–5: Min Pool Size 0/1/N, serial creation, shared rate limiter, failure resilience + no error state, shutdown cancellation, and all replenishment triggers with no overshoot at the minimum.Full ConnectionPool unit suite: 262/262 pass. Warmup tests stable across repeated runs.
Implements
specs/003-pool-warmup/spec.md.