-
Notifications
You must be signed in to change notification settings - Fork 329
Add configurable idle connection timeout (ADO #39970) #4295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 15 commits
126f16a
2a94926
93ab7ee
dd30ce7
b3a7b12
a6a69c5
a37358d
f32d6cd
0f0d018
911b83a
228fe6f
80da656
7611570
abfa301
5b1bbe9
0cf4d4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -979,6 +979,28 @@ The following example converts an existing connection string from using SQL Serv | |
| </para> | ||
| </remarks> | ||
| </LoadBalanceTimeout> | ||
| <IdleTimeout> | ||
| <summary> | ||
| Gets or sets the maximum time, in seconds, that a connection can sit unused (idle) in the connection pool before it is discarded. The default is 300 (5 minutes). | ||
| </summary> | ||
| <value> | ||
| The idle timeout for pooled connections, in seconds. | ||
| </value> | ||
| <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
+989
to
+1004
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rightly pointed. Added in document |
||
| </remarks> | ||
| </IdleTimeout> | ||
| <MaxPoolSize> | ||
| <summary> | ||
| Gets or sets the maximum number of connections allowed in the connection pool for this specific connection string. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,6 +255,16 @@ public void ReturnInternalConnection(DbConnectionInternal connection, DbConnecti | |
| } | ||
| else | ||
| { | ||
| // Stamp the return time so IsLiveConnection can later evict the connection if it sits | ||
| // idle past the configured limit. Skip the stamp when idle expiry is disabled or the | ||
| // legacy idle-timeout behavior is in effect to avoid the per-return DateTime.UtcNow on | ||
| // the hot return path; IsLiveConnection short-circuits on the same conditions so the | ||
| // value would be unread in those cases. | ||
| if (!LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior && | ||
|
priyankatiwari08 marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new connection pool has never had "legacy idle timeout behaviour", so should we even be checking the switch in this class? You can see this by the lack of an else block in |
||
| PoolGroupOptions.IdleTimeout != TimeSpan.Zero) | ||
| { | ||
| connection.ReturnedToPool(); | ||
| } | ||
| var written = _idleChannel.TryWrite(connection); | ||
| Debug.Assert(written, "Failed to write returning connection to the idle channel."); | ||
| } | ||
|
|
@@ -429,6 +439,22 @@ public bool TryGetConnection( | |
| /// <returns>Returns true if the connection is live and unexpired, otherwise returns false.</returns> | ||
| private bool IsLiveConnection(DbConnectionInternal connection) | ||
| { | ||
| // Connection has been sitting idle longer than the configured idle timeout. | ||
| // Checked before the (potentially expensive) liveness probe so an idle-expired | ||
| // connection is discarded without an SNI round-trip. | ||
| // ReturnedTime 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. | ||
| // Use subtraction rather than addition so the comparison cannot throw if ReturnedTime is | ||
| // ever close to DateTime.MaxValue. A clock skew that leaves ReturnedTime in the future | ||
| // produces a negative TimeSpan, which falls through as not-expired (fail safe). | ||
| TimeSpan idleTimeout = PoolGroupOptions.IdleTimeout; | ||
|
mdaigle marked this conversation as resolved.
|
||
| if (!LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior && | ||
| idleTimeout != TimeSpan.Zero && | ||
| DateTime.UtcNow - connection.ReturnedTime > idleTimeout) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Broken physical connection | ||
| if (!connection.IsConnectionAlive()) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,8 +223,26 @@ internal WaitHandleDbConnectionPool( | |
|
|
||
| lock (s_random) | ||
| { | ||
| // Random.Next is not thread-safe | ||
| _cleanupWait = s_random.Next(12, 24) * 10 * 1000; // 2-4 minutes in 10 sec intervals, WebData 103603 | ||
| TimeSpan idleTimeout = connectionPoolGroup.PoolGroupOptions.IdleTimeout; | ||
| if (LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior) | ||
| { | ||
| // Legacy: preserve the historical 2-4 minute random cleanup window. | ||
| _cleanupWait = s_random.Next(12, 24) * 10 * 1000; // 2-4 minutes in 10 sec intervals, WebData 103603 | ||
| } | ||
| else if (idleTimeout != TimeSpan.Zero) | ||
| { | ||
| // New + idle-expiry enabled: the WaitHandle pool takes two pruning cycles to evict | ||
| // an idle connection (new->old generation, then old->closed), so halve the configured | ||
| // timeout to approximate the requested idle lifetime. | ||
| long cleanupWaitMilliseconds = (long)idleTimeout.TotalMilliseconds / 2; | ||
| _cleanupWait = cleanupWaitMilliseconds >= int.MaxValue ? int.MaxValue : (int)cleanupWaitMilliseconds; | ||
|
Comment on lines
+237
to
+238
|
||
| } | ||
| else | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's avoid duplicating the calculation and move this condition up to the first block: Note that your changes here eliminate the "None" state I discussed earlier - this old pool will always perform idle connection cleanup, despite the connection string option claiming that a value of 0 means idle cleanup is disabled. |
||
| { | ||
| // New pool, idle-expiry disabled (IdleTimeout=0). The cleanup timer still runs for | ||
| // non-idle maintenance, so reuse the historical 2-4 minute window. | ||
| _cleanupWait = s_random.Next(12, 24) * 10 * 1000; | ||
| } | ||
| } | ||
|
|
||
| _connectionFactory = connectionFactory; | ||
|
|
@@ -667,6 +685,10 @@ private void DeactivateObject(DbConnectionInternal obj) | |
| // DelegatedTransactionEnded event will clean up the | ||
| // connection appropriately regardless of the pool state. | ||
| Debug.Assert(_transactedConnectionPool != null, "Transacted connection pool was not expected to be null."); | ||
| // Transacting connections are held in their own store and are never | ||
| // proactively closed (doing so would abort the transaction, which can be | ||
| // distributed). Idle-timeout enforcement does not apply here, so we do | ||
| // not call ReturnedToPool when parking the connection in the transacted pool. | ||
| _transactedConnectionPool.PutTransactedObject(transaction, obj); | ||
| rootTxn = true; | ||
| } | ||
|
|
@@ -1061,7 +1083,7 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj | |
| Interlocked.Decrement(ref _waitCount); | ||
| obj = GetFromGeneralPool(); | ||
|
|
||
| if ((obj != null) && (!obj.IsConnectionAlive())) | ||
| if ((obj != null) && (IsIdleExpired(obj) || !obj.IsConnectionAlive())) | ||
| { | ||
| SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetConnection|RES|CPOOL> {0}, Connection {1}, found dead and removed.", Id, obj.ObjectID); | ||
| DestroyObject(obj); | ||
|
|
@@ -1243,6 +1265,8 @@ private DbConnectionInternal GetFromTransactedPool(out Transaction transaction) | |
| } | ||
| else if (!obj.IsConnectionAlive()) | ||
| { | ||
| // Transacting connections are exempt from idle-timeout eviction (closing them | ||
| // would abort the transaction, possibly distributed). Only liveness is checked here. | ||
| SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetFromTransactedPool|RES|CPOOL> {0}, Connection {1}, found dead and removed.", Id, obj.ObjectID); | ||
| DestroyObject(obj); | ||
| obj = null; | ||
|
|
@@ -1363,13 +1387,39 @@ private void PutNewObject(DbConnectionInternal obj) | |
|
|
||
| SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.PutNewObject|RES|CPOOL> {0}, Connection {1}, Pushing to general pool.", Id, obj.ObjectID); | ||
|
|
||
| // Stamp the return time so IsIdleExpired can later decide whether the connection has sat | ||
| // unused too long. Skip the stamp when idle expiry is disabled or the legacy idle-timeout | ||
| // behavior is in effect to avoid the per-return DateTime.UtcNow on the hot return path; | ||
| // IsIdleExpired short-circuits on the same conditions so the value would be unread in | ||
| // those cases. | ||
| if (!LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior && | ||
| PoolGroupOptions.IdleTimeout != TimeSpan.Zero) | ||
| { | ||
| obj.ReturnedToPool(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... so now we're in a situation where the pool is 100% returning this connection to the idle pool, but we're not calling ReturnedToPool(), which seems wrong based on the context here and method name. Maybe a less-general method name like |
||
| } | ||
| _stackNew.Push(obj); | ||
| _waitHandles.PoolSemaphore.Release(1); | ||
|
|
||
| SqlClientDiagnostics.Metrics.EnterFreeConnection(); | ||
|
|
||
| } | ||
|
|
||
| /// <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) | ||
| { | ||
| // Use subtraction rather than addition so the comparison cannot throw if ReturnedTime is | ||
| // ever close to DateTime.MaxValue. A clock skew that leaves ReturnedTime in the future | ||
| // produces a negative TimeSpan, which falls through as not-expired (fail safe). | ||
| TimeSpan idleTimeout = PoolGroupOptions.IdleTimeout; | ||
|
mdaigle marked this conversation as resolved.
|
||
| return !LocalAppContextSwitches.UseLegacyIdleTimeoutBehavior && | ||
| idleTimeout != TimeSpan.Zero && | ||
| DateTime.UtcNow - obj.ReturnedTime > idleTimeout; | ||
| } | ||
|
|
||
| public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject) | ||
| { | ||
| Debug.Assert(obj != null, "null obj?"); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.