Add configurable idle connection timeout (ADO #39970)#4295
Open
priyankatiwari08 wants to merge 15 commits into
Open
Add configurable idle connection timeout (ADO #39970)#4295priyankatiwari08 wants to merge 15 commits into
priyankatiwari08 wants to merge 15 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).
Contributor
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 66.69% 64.58% -2.11%
==========================================
Files 284 279 -5
Lines 43238 66146 +22908
==========================================
+ Hits 28836 42721 +13885
- Misses 14402 23425 +9023
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.
Contributor
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;
paulmedynski
added
the
Area\Connection Pooling
mdaigle
requested changes
May 22, 2026
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
…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.
Contributor
Author
|
All review feedback has been addressed:
Ready for re-review. Please confirm if any further changes are needed. |
mdaigle
requested changes
May 26, 2026
Contributor
mdaigle
left a comment
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.
paulmedynski
added
the
Author attention needed
- SqlConnectionStringBuilder.xml IdleTimeout doc: simplify <value>,
move default into <summary>, "close" -> "discard",
"recycled" -> "discarded".
- Rename DbConnectionInternal.IdleSinceUtc -> ReturnedTime
(drop Utc suffix per Paul; align with sibling CreateTime which
is also UTC but unsuffixed).
- Rename DbConnectionInternal.MarkPooledIdle() -> ReturnedToPool()
and reword its XML doc to record a fact ("this connection was
returned to the pool") rather than assert pool policy ("idle"),
per Paul's policy/mechanism separation.
- Update call sites in ChannelDbConnectionPool and
WaitHandleDbConnectionPool to the new names; update comments
to match.
- Rename test helper BackdateIdleSince -> BackdateReturnedTime in
both pool test files for consistency.
…-state ctor - ChannelDbConnectionPool.ReturnInternalConnection / WaitHandleDbConnectionPool.PutNewObject: drop the (!UseLegacyIdleTimeoutBehavior && IdleTimeout != Zero) guard around ReturnedToPool(); always stamp. The read path short-circuits when expiry is disabled, so the stamp is harmless and the unconditional call removes the duplicated guard and its "must always be checked together" maintenance trap. - ChannelDbConnectionPool.IsLiveConnection / WaitHandleDbConnectionPool.IsIdleExpired: rewrite expiry check from "UtcNow > ReturnedTime + idleTimeout" to "UtcNow - ReturnedTime > idleTimeout" so the comparison cannot throw when ReturnedTime is close to DateTime.MaxValue; negative TimeSpan on clock skew falls through as not-expired (fail safe). - DbConnectionPoolOptions: swap Yoda-form "if (0 != idleTimeout)" to natural "if (idleTimeout != 0)". Pre-existing loadBalanceTimeout line left untouched. - WaitHandleDbConnectionPool ctor: split the cleanup-wait initialization into three explicit branches (Legacy / New+enabled / New+disabled) so the state model is documented in the code structure. Same observable behavior as before. - Tests: remove IdleTimeout_Zero_DoesNotStampOnReturn which pinned the now- removed optimization; the user-visible "IdleTimeout=0 -> never expires" invariant is still covered by IdleTimeout_Zero_DoesNotExpire.
Skip the per-return DateTime.UtcNow when UseLegacyIdleTimeoutBehavior is on or IdleTimeout == 0, matching the short-circuit in IsLiveConnection / IsIdleExpired. Addresses Copilot review feedback.
priyankatiwari08
moved this from Waiting for customer
to Investigating
in SqlClient Board
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs:138
- The XML doc comment for UseOverallConnectTimeoutForPoolWaitString is malformed (it closes without an opening
). With TreatWarningsAsErrors enabled, this can fail the build with an XML-doc warning (CS1570).
/// The name of the app context switch that controls whether pool operations
/// should count against the caller's overall ConnectTimeout budget.
/// </summary>
private const string UseOverallConnectTimeoutForPoolWaitString =
"Switch.Microsoft.Data.SqlClient.UseOverallConnectTimeoutForPoolWait";
Comment on lines
191
to
195
| SetSwitchValue( | ||
| "s_useLegacyIdleTimeoutBehavior", | ||
| _useLegacyIdleTimeoutBehaviorOriginal); | ||
| "s_useOverallConnectTimeoutForPoolWait", | ||
| _useOverallConnectTimeoutForPoolWaitOriginal); |
Comment on lines
+344
to
348
| public bool? UseLegacyIdleTimeoutBehavior | ||
| { | ||
| get => GetSwitchValue("s_useLegacyIdleTimeoutBehavior"); | ||
| set => SetSwitchValue("s_useLegacyIdleTimeoutBehavior", value); | ||
| /// Get or set the UseOverallConnectTimeoutForPoolWait switch value. |
The 'accept both' merge resolution dropped a SetSwitchValue( token in Dispose() and a closing brace plus /// <summary> between UseLegacyIdleTimeoutBehavior and UseOverallConnectTimeoutForPoolWait, causing CS1002/CS1513 compile errors.
The 'accept both' merge dropped the opening /// <summary> line on three UseOverallConnectTimeoutForPoolWait XML doc comments (const string, cached SwitchValue field, and public property), leaving them malformed.
Comment on lines
+989
to
+1001
| <remarks> | ||
| <para> | ||
| This property corresponds to the "Connection Idle Timeout" key within the connection string. | ||
| </para> | ||
| <para> | ||
| The driver makes a best effort to discard connections that have remained idle in the pool for longer than this value. The exact point in the connection lifecycle at which the check occurs is an implementation detail and may change over time. This protects callers from receiving connections that may have been silently closed by firewalls, load balancers, or server-side inactivity thresholds. | ||
| </para> | ||
| <para> | ||
| A value of zero (0) disables idle expiration; connections are kept in the pool indefinitely (subject to other expiry rules such as <see cref="P:Microsoft.Data.SqlClient.SqlConnectionStringBuilder.LoadBalanceTimeout" />). | ||
| </para> | ||
| <para> | ||
| Idle timeout operates independently of <see cref="P:Microsoft.Data.SqlClient.SqlConnectionStringBuilder.LoadBalanceTimeout" />. Whichever threshold is exceeded first causes the connection to be discarded. | ||
| </para> |
Comment on lines
+979
to
+993
| [Fact] | ||
| public void IdleTimeout_DefaultIsZero_DisablesExpiry() | ||
| { | ||
| // Explicitly passing zero keeps idle expiry off. | ||
| var poolGroupOptions = new DbConnectionPoolGroupOptions( | ||
| poolByIdentity: false, | ||
| minPoolSize: 0, | ||
| maxPoolSize: 50, | ||
| creationTimeout: 15, | ||
| loadBalanceTimeout: 0, | ||
| hasTransactionAffinity: true, | ||
| idleTimeout: 0); | ||
|
|
||
| Assert.Equal(TimeSpan.Zero, poolGroupOptions.IdleTimeout); | ||
| } |
WaitHandleDbConnectionPoolBudgetTest was added in main while this branch was in flight; updating its DbConnectionPoolGroupOptions construction to pass idleTimeout: 0 fixes the CS7036 net462 build error.
…nPoolGroupOptions in new idle-timeout tests Main added a TimeoutTimer parameter to ChannelDbConnectionPool.TryGetConnection and WaitHandleDbConnectionPool.TryGetConnection while this branch was in flight. The new idle-timeout tests still used the old 3-arg form, causing CS7036 on net9.0. Also missed one DbConnectionPoolGroupOptions ctor call in ConcurrentCallers test.
Comment on lines
+989
to
+1001
| <remarks> | ||
| <para> | ||
| This property corresponds to the "Connection Idle Timeout" key within the connection string. | ||
| </para> | ||
| <para> | ||
| The driver makes a best effort to discard connections that have remained idle in the pool for longer than this value. The exact point in the connection lifecycle at which the check occurs is an implementation detail and may change over time. This protects callers from receiving connections that may have been silently closed by firewalls, load balancers, or server-side inactivity thresholds. | ||
| </para> | ||
| <para> | ||
| A value of zero (0) disables idle expiration; connections are kept in the pool indefinitely (subject to other expiry rules such as <see cref="P:Microsoft.Data.SqlClient.SqlConnectionStringBuilder.LoadBalanceTimeout" />). | ||
| </para> | ||
| <para> | ||
| Idle timeout operates independently of <see cref="P:Microsoft.Data.SqlClient.SqlConnectionStringBuilder.LoadBalanceTimeout" />. Whichever threshold is exceeded first causes the connection to be discarded. | ||
| </para> |
Comment on lines
+963
to
+965
| <data name="DbConnectionString_IdleTimeout" xml:space="preserve"> | ||
| <value>The maximum amount of time (in seconds) a connection can sit unused (idle) in the pool before it is discarded. A value of 0 disables idle expiration.</value> | ||
| </data> |
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.
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: