Add configurable idle connection timeout (ADO #39970)#4295
Add configurable idle connection timeout (ADO #39970)#4295priyankatiwari08 wants to merge 3 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 66.04% 65.89% -0.16%
==========================================
Files 275 272 -3
Lines 42976 66035 +23059
==========================================
+ Hits 28383 43512 +15129
- Misses 14593 22523 +7930
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 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).
| } | ||
| } | ||
| else if (!obj.IsConnectionAlive()) | ||
| else if (!obj.IsConnectionAlive() || IsIdleExpired(obj)) |
There was a problem hiding this comment.
Good catch — fixed in 93ab7ee. DeactivateObject now calls obj.MarkPooledIdle() (guarded by the idle-timeout-enabled check) immediately before _transactedConnectionPool.PutTransactedObject(transaction, obj), so idle duration measures time parked in the transacted pool rather than time since create-time / last general-pool return.
Added a regression test IdleTimeout_TransactedPool_StampsOnReturn in WaitHandleDbConnectionPoolIdleTimeoutTest.cs that opens a TransactionScope, backdates IdleSinceUtc before return, and asserts the value is refreshed by the return path.
There was a problem hiding this comment.
Copilot is almost right, but the real issue is that transacting connections need to be treated specially. We never proactively close a transacting connection because that will abort the transaction (if it's distributed). We put transacting connections off to the side in their own storage specifically because we don't want them to be cleaned up before the transaction finishes. We should not consider idle timeout at this spot.
- 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;
| // Connection has been sitting idle longer than the configured idle timeout. | ||
| // IdleSinceUtc is initialized to CreateTime so a freshly minted connection never trips this | ||
| // check on first retrieval, and is then stamped by ReturnInternalConnection on every return. | ||
| TimeSpan idleTimeout = PoolGroupOptions.IdleTimeout; | ||
| if (idleTimeout != TimeSpan.Zero && DateTime.UtcNow > connection.IdleSinceUtc + idleTimeout) |
paulmedynski
added
the
Area\Connection Pooling
| internal const string PacketSize = "packetsize"; | ||
| internal const string PersistSecurityInfo = "persistsecurityinfo"; | ||
| internal const string PoolBlockingPeriod = "poolblockingperiod"; | ||
| internal const string PoolIdleTimeout = "pool idle timeout"; |
There was a problem hiding this comment.
we don't need to add a synonym
| } | ||
| } | ||
| else if (!obj.IsConnectionAlive()) | ||
| else if (!obj.IsConnectionAlive() || IsIdleExpired(obj)) |
There was a problem hiding this comment.
Copilot is almost right, but the real issue is that transacting connections need to be treated specially. We never proactively close a transacting connection because that will abort the transaction (if it's distributed). We put transacting connections off to the side in their own storage specifically because we don't want them to be cleaned up before the transaction finishes. We should not consider idle timeout at this spot.
| // pool so the next retrieval measures idle time from when it left the active set, | ||
| // not from create-time or the previous general-pool return. Skip when idle expiry | ||
| // is disabled to avoid an unnecessary DateTime.UtcNow on the hot return path. | ||
| if (PoolGroupOptions.IdleTimeout != TimeSpan.Zero) |
There was a problem hiding this comment.
Same here. Transacting connections don't need to get an idle stamp because they won't be proactively closed. You can add a comment explaining it.
|
|
||
| if (0 != idleTimeout) | ||
| { | ||
| _idleTimeout = new TimeSpan(0, 0, idleTimeout); |
There was a problem hiding this comment.
| _idleTimeout = new TimeSpan(0, 0, idleTimeout); | |
| _idleTimeout = TimeSpan.FromSeconds(idleTimeout); |
| int loadBalanceTimeout, | ||
| bool hasTransactionAffinity | ||
| bool hasTransactionAffinity, | ||
| int idleTimeout = 0 |
There was a problem hiding this comment.
Let's make this required and have the default value handled by DbConnectionStringDefaults.
| internal const bool IntegratedSecurity = false; | ||
| internal const SqlConnectionIPAddressPreference IpAddressPreference = SqlConnectionIPAddressPreference.IPv4First; | ||
| internal const int LoadBalanceTimeout = 0; // default of 0 means don't use | ||
| internal const int IdleTimeout = 0; // default of 0 means don't expire idle connections |
There was a problem hiding this comment.
Let's default this to 300, like npgsql. We'll later use this value to determine how often we prune. Today we prune every 4-8 minutes. 300 seconds will default us to 5 mins which falls in that range.
https://www.npgsql.org/doc/connection-string-parameters.html
There was a problem hiding this comment.
Please also use this value to set the _cleanupWait value in WaitHandleDbConnectionPool
github-project-automation
Bot
moved this from In review
to Waiting for customer
in SqlClient Board
This pull request adds a new
Connection Idle Timeoutconnection-string keyword to Microsoft.Data.SqlClient, allowing users to control how long a pooled connection may remain idle before it is discarded on the next retrieval. This addresses #343, where firewalls or load balancers silently kill idle connections and applications then pull dead connections from the pool. Tracks ADO #39970.The implementation is based on the Idle Connection Timeout spec.
Configuration surface:
Connection Idle Timeout(in seconds), withPool Idle Timeoutas a synonym.IdleTimeoutproperty onSqlConnectionStringBuilder(int, seconds, default0). Negative values throw at parse time. A value of0disables idle expiration.Behavior:
ChannelDbConnectionPoolandWaitHandleDbConnectionPool).MinPoolSize: a caller always receives a working connection.Design decisions taken:
Connection Idle Timeoutis the canonical keyword;Pool Idle Timeoutis registered as a synonym.SqlConnection.IdleTimeoutproperty — the keyword is exposed only onSqlConnectionStringBuilder.0(idle expiration disabled).Out of scope / deferred to a follow-up PR:
MinPoolSizefloor on proactive removal. The spec requires this to share a timer with the pruning feature (#37338), which is not yet landed;ChannelDbConnectionPoolalso has no periodic timer today. Deferred to a follow-up that builds on the pruning work.Tests:
ChannelDbConnectionPoolTest.cscovering zero-disables, expiry on retrieval, recently-returned-not-expired, stamping on return, initial value, and "checked-out connections aren't affected".SqlConnectionStringBuilderTest.cscovering default value, canonical keyword parsing, synonym parsing, round-trip through the builder, and negative-value rejection. Mirrors the existingLoadBalanceTimeout/MaxPoolSize/MinPoolSizetest pattern.ChannelDbConnectionPoolwork itself was shipped.net9.0; clean build.Links: