Add configurable idle connection timeout (ADO #39970)#4295
Add configurable idle connection timeout (ADO #39970)#4295priyankatiwari08 wants to merge 22 commits into
Conversation
Implements spec User Stories 1, 2, 4 + FR-009 of US3 from specs/003-pool-idle-timeout/spec.md. Adds 'Connection Idle Timeout' keyword (synonym: 'Pool Idle Timeout') exposed via SqlConnectionStringBuilder.IdleTimeout. When > 0, connections that have sat idle in the pool longer than the configured number of seconds are discarded on retrieval and a fresh connection is returned. Default 0 (disabled) matches the existing convention used by LoadBalanceTimeout and ConnectionLifetime. Covers both pool designs (ChannelDbConnectionPool, WaitHandleDbConnectionPool). Deferred to follow-up: proactive timer sweep (FR-008, FR-010) which the spec assumes is built on top of the pruning feature (#37338).
There was a problem hiding this comment.
Pull request overview
Adds a new pooling-related connection-string keyword, Connection Idle Timeout (synonym: Pool Idle Timeout), enabling lazy eviction of pooled connections that have sat idle longer than the configured number of seconds. This targets stale/half-open pooled connections (e.g., behind firewalls/load balancers) by discarding expired idle connections on the retrieval path in both pool implementations.
Changes:
- Introduces
Connection Idle Timeoutparsing +SqlConnectionStringBuilder.IdleTimeoutproperty (default0disables). - Tracks per-connection idle timestamp (
DbConnectionInternal.IdleSinceUtc) and stamps it on return-to-pool; evicts idle-expired connections on retrieval in both pool designs. - Adds unit/functional tests for the new builder keyword/property and Channel pool idle-expiry behavior.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/snippets/Microsoft.Data.SqlClient/SqlConnectionStringBuilder.xml | Adds XML doc snippet for SqlConnectionStringBuilder.IdleTimeout. |
| src/Microsoft.Data.SqlClient/ref/Microsoft.Data.SqlClient.cs | Updates reference assembly surface with IdleTimeout property. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringKeywords.cs | Adds canonical keyword string Connection Idle Timeout. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringDefaults.cs | Adds default IdleTimeout = 0. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/ConnectionString/DbConnectionStringSynonyms.cs | Adds synonym pool idle timeout. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs | Adds IdleSinceUtc tracking + MarkPooledIdle() stamping helper. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs | Stamps idle time on return and adds idle-expiry check in IsLiveConnection. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolOptions.cs | Adds IdleTimeout option (as TimeSpan) to pool group options. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs | Stamps idle time on return and adds IsIdleExpired check at retrieval sites. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs | Wires parsed idle-timeout option into pool group options creation. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionOptions.cs | Parses/validates idle-timeout integer from connection string. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnectionStringBuilder.cs | Adds IdleTimeout keyword/property support + synonym mapping. |
| src/Microsoft.Data.SqlClient/src/Resources/Strings.resx | Adds localized description string for the new keyword/property. |
| src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs | Regenerates resource accessor for DbConnectionString_IdleTimeout. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/ConnectionPool/ChannelDbConnectionPoolTest.cs | Adds unit tests for Channel pool idle-timeout behavior and stamping. |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionStringBuilderTest.cs | Adds functional tests for keyword parsing, round-trip, default, and invalid values. |
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs:445
- The comment says that when
IdleSinceUtcis the default (DateTime.MinValue) the idle-timeout check is a no-op and indicates the connection has never been pooled. In this PRIdleSinceUtcis initialized toCreateTimeinDbConnectionInternal, so it will not beDateTime.MinValue, and even if it were, the current comparison would not be a no-op. Please update/remove this comment to match the actual semantics (e.g., describe the create-time initialization and that the check applies to any connection read from the idle channel).
// Connection has been sitting idle longer than the configured idle timeout.
// IdleSinceUtc is stamped by ReturnInternalConnection on each return; if it is the default
// (DateTime.MinValue), the connection has never been pooled yet and the check is a no-op.
TimeSpan idleTimeout = PoolGroupOptions.IdleTimeout;
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs:1351
- Idle-timeout enforcement was added to the legacy
WaitHandleDbConnectionPool(newIsIdleExpiredcheck +MarkPooledIdlestamping), but there doesn’t appear to be any unit test coverage exercising this path (existing idle-timeout unit tests are only forChannelDbConnectionPool). Please add at least one focused test validating that an idle-expired connection is discarded and replaced inWaitHandleDbConnectionPool, and that IdleTimeout==0 leaves behavior unchanged.
/// <summary>
/// Returns true when the supplied connection has been sitting idle in the pool longer than the
/// configured <see cref="DbConnectionPoolGroupOptions.IdleTimeout"/>. Returns false when idle timeout
/// is disabled (zero).
/// </summary>
private bool IsIdleExpired(DbConnectionInternal obj)
{
TimeSpan idleTimeout = PoolGroupOptions.IdleTimeout;
return idleTimeout != TimeSpan.Zero && DateTime.UtcNow > obj.IdleSinceUtc + idleTimeout;
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4295 +/- ##
==========================================
- Coverage 65.59% 63.53% -2.07%
==========================================
Files 285 280 -5
Lines 43311 66270 +22959
==========================================
+ Hits 28412 42106 +13694
- Misses 14899 24164 +9265
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Fix stale comment in ChannelDbConnectionPool.IsLiveConnection: IdleSinceUtc is initialized to CreateTime, not DateTime.MinValue. - Add WaitHandleDbConnectionPoolIdleTimeoutTest mirroring the existing channel-pool idle-timeout coverage (stamp on return, zero disables expiry, expired connection is replaced, fresh connection is reused).
- Skip MarkPooledIdle on return when IdleTimeout == TimeSpan.Zero so the default config has no per-return DateTime.UtcNow on the hot path. Applies to ChannelDbConnectionPool.ReturnInternalConnection and WaitHandleDbConnectionPool.PutNewObject. - Stamp IdleSinceUtc when returning a connection into the transacted pool (WaitHandleDbConnectionPool.DeactivateObject before TransactedConnectionPool.PutTransactedObject) so idle expiry on the next retrieval measures time spent parked in the transacted pool, not time since create-time / last general-pool return. - Add 2 WaitHandle pool tests covering the new behavior: IdleTimeout_TransactedPool_StampsOnReturn and IdleTimeout_Zero_DoesNotStampOnReturn.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs:1043
- The idle-timeout eviction check is evaluated after obj.IsConnectionAlive(). For long-idle pooled connections, IsConnectionAlive() may perform an expensive SNI-level liveness probe; if the connection is already idle-expired, that work is unnecessary. Consider checking IsIdleExpired(obj) first (or reordering the condition) so expired connections are discarded without doing a liveness check.
This issue also appears on line 1218 of the same file.
if ((obj != null) && (!obj.IsConnectionAlive() || IsIdleExpired(obj)))
{
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Connection {1}, found dead and removed.", Id, obj.ObjectID);
DestroyObject(obj);
obj = null; // Setting to null in case creating a new object fails
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs:1222
- Similar to the general-pool path: in the transacted-pool retrieval, the code calls obj.IsConnectionAlive() before IsIdleExpired(obj). Since IsConnectionAlive() can trigger an SNI liveness probe for idle connections, consider checking IsIdleExpired(obj) first to avoid extra work when the idle-timeout policy is what causes the connection to be recycled.
else if (!obj.IsConnectionAlive() || IsIdleExpired(obj))
{
SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetFromTransactedPool|RES|CPOOL> {0}, Connection {1}, found dead and removed.", Id, obj.ObjectID);
DestroyObject(obj);
obj = null;
…sacted idle handling - Default Connection Idle Timeout 0 -> 300 (5 min, matches Npgsql); 0 disables. - Remove 'Pool Idle Timeout' synonym; the canonical keyword is the only accepted form. - Make idleTimeout a required parameter on DbConnectionPoolGroupOptions; defaults now live in DbConnectionStringDefaults. - Use TimeSpan.FromSeconds for the ctor body conversion. - WaitHandle pool: drop MarkPooledIdle() on transacted-pool return and remove the idle-expiry check on transacted-pool retrieval (transacting connections must never be proactively closed). - WaitHandle pool: reorder general-pool retrieval to check idle expiry before the liveness probe; derive _cleanupWait from IdleTimeout when set. - Channel pool: reorder IsLiveConnection so the idle check runs before IsConnectionAlive(). - Tests, doc snippet, and release notes updated accordingly.
|
All review feedback has been addressed:
Ready for re-review. Please confirm if any further changes are needed. |
mdaigle
left a comment
There was a problem hiding this comment.
Thanks for making those changes. I have a few more updates that I'd like to see. Otherwise it's very close.
…ryGetConnection in shutdown tests PR dotnet#4295 (merged to main) added a required TimeoutTimer parameter to ChannelDbConnectionPool.TryGetConnection. The shutdown tests added in this PR still call the pre-dotnet#4295 3-arg overload, which compiles locally against PoolShutdown's older base but fails in the CI pipeline because the pipeline merges this PR's head into the post-dotnet#4295 main. Pass TimeoutTimer.StartNew(TimeSpan.FromSeconds(15)) at all 5 call sites to match the new signature.
… and SetReturnedTime rename - WaitHandleDbConnectionPool ctor: collapse the three _cleanupWait branches into two (legacy-switch OR IdleTimeout==Zero -> historical 2-4 min window; else use configured timeout/2) per Paul's suggestion, and floor the new-behavior period at 1000 ms so small IdleTimeout values don't schedule a sub-second timer. - WaitHandleDbConnectionPool.CleanupCallback: gate both the destroy and the new->old age sweeps on a single `idleEvictionDisabled'' check (`!UseLegacyIdleTimeoutBehavior && IdleTimeout == Zero'') so the `OFF + 0 = disabled entirely'' case is honored. The MinPoolSize-floor re-queue below the gates still runs. - Rename DbConnectionInternal.ReturnedToPool -> SetReturnedTime and update both call sites (WaitHandle, Channel) and the related comment. The name now describes the mechanism (the connection stamps its returned time) and reads cleanly when the pool decides to skip the call because nothing will read the stamp. - Add WaitHandleDbConnectionPoolIdleTimeoutTest IdleTimeout_LegacyOff_Zero_CleanupCallbackDoesNotEvict covering the new CleanupCallback gate. Reaches the private method via reflection.
… idleTimeout: 0 to DbConnectionPoolGroupOptions ctor
…ectionPoolGroupOptions ctor (required after PR 4295)
…on IsIdleExpired With _cleanupWait = IdleTimeout/2, the legacy two-stack generational sweep in CleanupCallback could destroy a connection from _stackOld after as little as ~IdleTimeout/2 of real idle time, violating the new contract that connections are only destroyed once their idle age exceeds IdleTimeout. Add a per-connection IsIdleExpired check inside the destroy loop: when the new behaviour is enabled and the popped connection has not yet exceeded IdleTimeout, push it back onto _stackOld, release the semaphore count, and break out of the tick. The next cleanup tick reconsiders. Legacy behaviour (switch on or IdleTimeout = 0) is untouched and continues to destroy unconditionally as before.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| // When the new idle-timeout behaviour is enabled and IdleTimeout is 0, idle eviction is | ||
| // disabled entirely: skip the generational destroy/age-into-old-stack sweep and only run | ||
| // the MinPoolSize floor maintenance below. | ||
| bool idleEvictionDisabled = |
There was a problem hiding this comment.
You only ever check the negative of this bool, so let's flip its meaning to idleEvictionEnabled. Then the computation becomes easier to understand as well.
There was a problem hiding this comment.
Flipped in 1e37541. The local is now idleEvictionEnabled, computed positively as UseLegacyIdleTimeoutBehavior || PoolGroupOptions.IdleTimeout != TimeSpan.Zero, and both call sites in CleanupCallback read it without negation. Reads naturally now: legacy mode OR a configured timeout → eviction is on.
…te, clarify IdleTimeout doc, rename idleEvictionDisabled -> idleEvictionEnabled - WaitHandleDbConnectionPool.CleanupCallback: revert the per-connection IsIdleExpired short-circuit added in e999310 (mdaigle). _stackOld is LIFO and not age-ordered after a _stackNew promotion pass, so breaking on the first non-expired top could indefinitely hide older connections underneath under sustained traffic. Pruning is back to unconditional, matching the legacy 1-to-2-tick upper bound on idle lifetime. - DbConnectionPoolOptions.IdleTimeout XML doc: state that eviction is best-effort, runs on retrieval and during periodic maintenance, and may occasionally fire earlier than the configured value (copilot). - WaitHandleDbConnectionPool: flip the local bool from idleEvictionDisabled (only ever read negated) to idleEvictionEnabled for readability (paulmedynski).
paulmedynski
left a comment
There was a problem hiding this comment.
Looks good - just one suggestion for doc clarification, but not gating.
| /// The maximum time a pooled connection can sit unused (idle) in the pool before it becomes | ||
| /// eligible for eviction. Eviction is best-effort: a connection that has been idle longer | ||
| /// than this value is discarded either on the next retrieval attempt or during a periodic | ||
| /// pool maintenance pass, whichever happens first. Because maintenance runs on a fixed |
There was a problem hiding this comment.
Is it worth giving an example of an early eviction scenario? It's not obvious how it could occur, and I suspect it depends heavily on the particular pool's implementation.
Adds a configurable idle connection timeout for pooled
SqlConnectioninstances.Compatibility and behavior:
IdleTimeoutdefaults to300seconds.Switch.Microsoft.Data.SqlClient.UseLegacyIdleTimeoutBehavior, which defaults totrueso current behavior is preserved unless the switch is disabled.IdleTimeoutcontrols when pooled connections are considered idle and eligible for expiration.Additional changes: