From 6264afd642b79e4220c8f56db0b8deb164ea8589 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Fri, 12 Nov 2021 21:46:49 +0100 Subject: [PATCH 1/3] Guard pool scavenging callback from parallel execution when it takes longer than the timer interval gets triggered --- .../HttpConnectionPoolManager.cs | 48 +++++++++++++------ 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs index e257cb4c3e692..e581ec2b0908e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs @@ -51,6 +51,12 @@ internal sealed class HttpConnectionPoolManager : IDisposable /// call. /// private bool _timerIsRunning; + + /// + /// Prevents parallel execution of RemoveStalePools in case the timer triggers faster than the method itself finishes. + /// + private int _removeStalePoolsIsRunning; + /// Object used to synchronize access to state in the pool. private object SyncObj => _pools; @@ -479,27 +485,41 @@ private void RemoveStalePools() { Debug.Assert(_cleaningTimer != null); - // Iterate through each pool in the set of pools. For each, ask it to clear out - // any unusable connections (e.g. those which have expired, those which have been closed, etc.) - // The pool may detect that it's empty and long unused, in which case it'll dispose of itself, - // such that any connections returned to the pool to be cached will be disposed of. In such - // a case, we also remove the pool from the set of pools to avoid a leak. - foreach (KeyValuePair entry in _pools) + // Check whether the method is not already running and prevent parallel execution. + if (Interlocked.CompareExchange(ref _removeStalePoolsIsRunning, 1, 0) != 0) { - if (entry.Value.CleanCacheAndDisposeIfUnused()) - { - _pools.TryRemove(entry.Key, out HttpConnectionPool _); - } + return; } - // Stop running the timer if we don't have any pools to clean up. - lock (SyncObj) + try { - if (_pools.IsEmpty) + // Iterate through each pool in the set of pools. For each, ask it to clear out + // any unusable connections (e.g. those which have expired, those which have been closed, etc.) + // The pool may detect that it's empty and long unused, in which case it'll dispose of itself, + // such that any connections returned to the pool to be cached will be disposed of. In such + // a case, we also remove the pool from the set of pools to avoid a leak. + foreach (KeyValuePair entry in _pools) { - SetCleaningTimer(Timeout.InfiniteTimeSpan); + if (entry.Value.CleanCacheAndDisposeIfUnused()) + { + _pools.TryRemove(entry.Key, out HttpConnectionPool _); + } + } + + // Stop running the timer if we don't have any pools to clean up. + lock (SyncObj) + { + if (_pools.IsEmpty) + { + SetCleaningTimer(Timeout.InfiniteTimeSpan); + } } } + finally + { + // Make sure the guard value gets always reset back to 0 and that it's visible to other threads. + Volatile.Write(ref _removeStalePoolsIsRunning, 0); + } // NOTE: There is a possible race condition with regards to a pool getting cleaned up at the same // time it's about to be used for another request. The timer cleanup could start running, see that From 009576a0b960fecea1db78475860d4245c91ae36 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Fri, 12 Nov 2021 21:59:37 +0100 Subject: [PATCH 2/3] Dispose connection from the pool in a serate task to not to block the caller (scavenge timer callback) --- .../Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs index 2879d37cb405c..b365c3cf8074c 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs @@ -1979,7 +1979,11 @@ public bool CleanCacheAndDisposeIfUnused() } // Dispose the stale connections outside the pool lock, to avoid holding the lock too long. - toDispose?.ForEach(c => c.Dispose()); + // Dispose them asynchronously to not to block the caller on closing the SslStream or NetworkStream. + if (toDispose is not null) + { + Task.Run(() => toDispose.ForEach(c => c.Dispose())); + } // Pool is active. Should not be removed. return false; From 50137280b475c6f542c5c2cbb7ad1ed97cd977cf Mon Sep 17 00:00:00 2001 From: ManickaP Date: Mon, 15 Nov 2021 19:07:42 +0100 Subject: [PATCH 3/3] feedback --- .../SocketsHttpHandler/HttpConnectionPool.cs | 3 +- .../HttpConnectionPoolManager.cs | 49 +++++-------------- 2 files changed, 15 insertions(+), 37 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs index b365c3cf8074c..75b83fbe8bae6 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs @@ -1982,7 +1982,8 @@ public bool CleanCacheAndDisposeIfUnused() // Dispose them asynchronously to not to block the caller on closing the SslStream or NetworkStream. if (toDispose is not null) { - Task.Run(() => toDispose.ForEach(c => c.Dispose())); + Task.Factory.StartNew(static s => ((List)s!).ForEach(c => c.Dispose()), toDispose, + CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); } // Pool is active. Should not be removed. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs index e581ec2b0908e..f118d151c692a 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs @@ -51,12 +51,6 @@ internal sealed class HttpConnectionPoolManager : IDisposable /// call. /// private bool _timerIsRunning; - - /// - /// Prevents parallel execution of RemoveStalePools in case the timer triggers faster than the method itself finishes. - /// - private int _removeStalePoolsIsRunning; - /// Object used to synchronize access to state in the pool. private object SyncObj => _pools; @@ -468,7 +462,7 @@ private void SetCleaningTimer(TimeSpan timeout) { try { - _cleaningTimer!.Change(timeout, timeout); + _cleaningTimer!.Change(timeout, Timeout.InfiniteTimeSpan); _timerIsRunning = timeout != Timeout.InfiniteTimeSpan; } catch (ObjectDisposedException) @@ -485,40 +479,23 @@ private void RemoveStalePools() { Debug.Assert(_cleaningTimer != null); - // Check whether the method is not already running and prevent parallel execution. - if (Interlocked.CompareExchange(ref _removeStalePoolsIsRunning, 1, 0) != 0) - { - return; - } - - try + // Iterate through each pool in the set of pools. For each, ask it to clear out + // any unusable connections (e.g. those which have expired, those which have been closed, etc.) + // The pool may detect that it's empty and long unused, in which case it'll dispose of itself, + // such that any connections returned to the pool to be cached will be disposed of. In such + // a case, we also remove the pool from the set of pools to avoid a leak. + foreach (KeyValuePair entry in _pools) { - // Iterate through each pool in the set of pools. For each, ask it to clear out - // any unusable connections (e.g. those which have expired, those which have been closed, etc.) - // The pool may detect that it's empty and long unused, in which case it'll dispose of itself, - // such that any connections returned to the pool to be cached will be disposed of. In such - // a case, we also remove the pool from the set of pools to avoid a leak. - foreach (KeyValuePair entry in _pools) + if (entry.Value.CleanCacheAndDisposeIfUnused()) { - if (entry.Value.CleanCacheAndDisposeIfUnused()) - { - _pools.TryRemove(entry.Key, out HttpConnectionPool _); - } - } - - // Stop running the timer if we don't have any pools to clean up. - lock (SyncObj) - { - if (_pools.IsEmpty) - { - SetCleaningTimer(Timeout.InfiniteTimeSpan); - } + _pools.TryRemove(entry.Key, out HttpConnectionPool _); } } - finally + + // Restart the timer if we have any pools to clean up. + lock (SyncObj) { - // Make sure the guard value gets always reset back to 0 and that it's visible to other threads. - Volatile.Write(ref _removeStalePoolsIsRunning, 0); + SetCleaningTimer(!_pools.IsEmpty ? _cleanPoolTimeout : Timeout.InfiniteTimeSpan); } // NOTE: There is a possible race condition with regards to a pool getting cleaned up at the same