-
Notifications
You must be signed in to change notification settings - Fork 330
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 all commits
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 |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ internal static class DbConnectionStringSynonyms | |
| internal const string PacketSize = "packetsize"; | ||
| internal const string PersistSecurityInfo = "persistsecurityinfo"; | ||
| internal const string PoolBlockingPeriod = "poolblockingperiod"; | ||
| internal const string PoolIdleTimeout = "pool idle timeout"; | ||
|
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. we don't need to add a synonym |
||
| internal const string Pwd = "pwd"; | ||
| internal const string Server = "server"; | ||
| internal const string ServerCertificate = "servercertificate"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -13,6 +13,7 @@ internal sealed class DbConnectionPoolGroupOptions | |||||
| private readonly int _maxPoolSize; | ||||||
| private readonly int _creationTimeout; | ||||||
| private readonly TimeSpan _loadBalanceTimeout; | ||||||
| private readonly TimeSpan _idleTimeout; | ||||||
| private readonly bool _hasTransactionAffinity; | ||||||
| private readonly bool _useLoadBalancing; | ||||||
|
|
||||||
|
|
@@ -22,7 +23,8 @@ public DbConnectionPoolGroupOptions( | |||||
| int maxPoolSize, | ||||||
| int creationTimeout, | ||||||
| int loadBalanceTimeout, | ||||||
| bool hasTransactionAffinity | ||||||
| bool hasTransactionAffinity, | ||||||
| int idleTimeout = 0 | ||||||
|
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 make this required and have the default value handled by DbConnectionStringDefaults. |
||||||
| ) | ||||||
| { | ||||||
| _poolByIdentity = poolByIdentity; | ||||||
|
|
@@ -36,6 +38,11 @@ bool hasTransactionAffinity | |||||
| _useLoadBalancing = true; | ||||||
| } | ||||||
|
|
||||||
| if (0 != idleTimeout) | ||||||
| { | ||||||
| _idleTimeout = new TimeSpan(0, 0, idleTimeout); | ||||||
|
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.
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| _hasTransactionAffinity = hasTransactionAffinity; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -54,6 +61,14 @@ public TimeSpan LoadBalanceTimeout | |||||
| { | ||||||
| get { return _loadBalanceTimeout; } | ||||||
| } | ||||||
| /// <summary> | ||||||
| /// The maximum time a pooled connection can sit unused (idle) in the pool before it is discarded | ||||||
| /// on the next retrieval attempt. <see cref="TimeSpan.Zero"/> disables idle expiration. | ||||||
| /// </summary> | ||||||
| public TimeSpan IdleTimeout | ||||||
| { | ||||||
| get { return _idleTimeout; } | ||||||
| } | ||||||
| public int MaxPoolSize | ||||||
| { | ||||||
| get { return _maxPoolSize; } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -670,6 +670,14 @@ 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."); | ||
| // Stamp the idle-since timestamp before parking the connection in the transacted | ||
| // 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) | ||
|
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. 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. |
||
| { | ||
| obj.MarkPooledIdle(); | ||
| } | ||
| _transactedConnectionPool.PutTransactedObject(transaction, obj); | ||
| rootTxn = true; | ||
| } | ||
|
|
@@ -1028,7 +1036,7 @@ private bool TryGetConnection(DbConnection owningObject, uint waitForMultipleObj | |
| Interlocked.Decrement(ref _waitCount); | ||
| obj = GetFromGeneralPool(); | ||
|
|
||
| if ((obj != null) && (!obj.IsConnectionAlive())) | ||
| 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); | ||
|
|
@@ -1207,7 +1215,7 @@ private DbConnectionInternal GetFromTransactedPool(out Transaction transaction) | |
| throw; | ||
| } | ||
| } | ||
| else if (!obj.IsConnectionAlive()) | ||
| else if (!obj.IsConnectionAlive() || IsIdleExpired(obj)) | ||
|
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. Good catch — fixed in 93ab7ee. Added a regression test
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. 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. |
||
| { | ||
| SqlClientEventSource.Log.TryPoolerTraceEvent("<prov.DbConnectionPool.GetFromTransactedPool|RES|CPOOL> {0}, Connection {1}, found dead and removed.", Id, obj.ObjectID); | ||
| DestroyObject(obj); | ||
|
|
@@ -1329,13 +1337,32 @@ 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 idle-since timestamp immediately before placing the connection on the idle stack | ||
| // so that idle-expiry checks on later retrieval can decide whether it has sat unused too long. | ||
| // Skip the stamp when idle expiry is disabled (the default) to avoid the per-return | ||
| // DateTime.UtcNow on the hot return path. | ||
| if (PoolGroupOptions.IdleTimeout != TimeSpan.Zero) | ||
| { | ||
| obj.MarkPooledIdle(); | ||
| } | ||
| _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) | ||
| { | ||
| TimeSpan idleTimeout = PoolGroupOptions.IdleTimeout; | ||
| return idleTimeout != TimeSpan.Zero && DateTime.UtcNow > obj.IdleSinceUtc + idleTimeout; | ||
| } | ||
|
|
||
| public void ReturnInternalConnection(DbConnectionInternal obj, DbConnection owningObject) | ||
| { | ||
| Debug.Assert(obj != null, "null obj?"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also use this value to set the
_cleanupWaitvalue in WaitHandleDbConnectionPool