-
Notifications
You must be signed in to change notification settings - Fork 5k
Add more System.Net.Http counters #38686
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
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 |
---|---|---|
|
@@ -31,6 +31,10 @@ namespace System.Net.Http | |
/// <summary>Provides a set of connection pools, each for its own endpoint.</summary> | ||
internal sealed class HttpConnectionPoolManager : IDisposable | ||
{ | ||
// Used by HttpTelemetry to calculate statistics over all connection pools | ||
private static readonly ConcurrentDictionary<WeakReference<HttpConnectionPoolManager>, object?> s_allConnectionPools = | ||
new ConcurrentDictionary<WeakReference<HttpConnectionPoolManager>, object?>(); | ||
|
||
/// <summary>How frequently an operation should be initiated to clean out old pools and connections in those pools.</summary> | ||
private readonly TimeSpan _cleanPoolTimeout; | ||
/// <summary>The pools, indexed by endpoint.</summary> | ||
|
@@ -44,6 +48,8 @@ internal sealed class HttpConnectionPoolManager : IDisposable | |
private readonly IWebProxy? _proxy; | ||
private readonly ICredentials? _proxyCredentials; | ||
|
||
private readonly WeakReference<HttpConnectionPoolManager>? _weakThisRef; | ||
|
||
private NetworkChangeCleanup? _networkChangeCleanup; | ||
|
||
/// <summary> | ||
|
@@ -103,14 +109,19 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings) | |
// Create the timer. Ensure the Timer has a weak reference to this manager; otherwise, it | ||
// can introduce a cycle that keeps the HttpConnectionPoolManager rooted by the Timer | ||
// implementation until the handler is Disposed (or indefinitely if it's not). | ||
_weakThisRef = new WeakReference<HttpConnectionPoolManager>(this); | ||
|
||
_cleaningTimer = new Timer(s => | ||
{ | ||
var wr = (WeakReference<HttpConnectionPoolManager>)s!; | ||
if (wr.TryGetTarget(out HttpConnectionPoolManager? thisRef)) | ||
{ | ||
thisRef.RemoveStalePools(); | ||
} | ||
}, new WeakReference<HttpConnectionPoolManager>(this), Timeout.Infinite, Timeout.Infinite); | ||
}, _weakThisRef, Timeout.Infinite, Timeout.Infinite); | ||
|
||
bool success = s_allConnectionPools.TryAdd(_weakThisRef, null); | ||
Debug.Assert(success); | ||
} | ||
finally | ||
{ | ||
|
@@ -121,6 +132,10 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings) | |
} | ||
} | ||
} | ||
else | ||
{ | ||
GC.SuppressFinalize(this); | ||
} | ||
|
||
// Figure out proxy stuff. | ||
if (settings._useProxy) | ||
|
@@ -133,6 +148,14 @@ public HttpConnectionPoolManager(HttpConnectionSettings settings) | |
} | ||
} | ||
|
||
~HttpConnectionPoolManager() | ||
{ | ||
if (_weakThisRef != null) | ||
{ | ||
s_allConnectionPools.TryRemove(_weakThisRef, out _); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Starts monitoring for network changes. Upon a change, <see cref="HttpConnectionPool.OnNetworkChanged"/> will be | ||
/// called for every <see cref="HttpConnectionPool"/> in the <see cref="HttpConnectionPoolManager"/>. | ||
|
@@ -455,6 +478,12 @@ public void Dispose() | |
{ | ||
_cleaningTimer?.Dispose(); | ||
|
||
if (_weakThisRef != null) | ||
{ | ||
s_allConnectionPools.TryRemove(_weakThisRef, out _); | ||
GC.SuppressFinalize(this); | ||
} | ||
|
||
foreach (KeyValuePair<HttpConnectionKey, HttpConnectionPool> pool in _pools) | ||
{ | ||
pool.Value.Dispose(); | ||
|
@@ -527,6 +556,38 @@ private static string GetIdentityIfDefaultCredentialsUsed(bool defaultCredential | |
return defaultCredentialsUsed ? CurrentUserIdentityProvider.GetIdentity() : string.Empty; | ||
} | ||
|
||
public static int GetMaxHttp11ConnectionsPerPool() | ||
{ | ||
int max = 0; | ||
foreach ((WeakReference<HttpConnectionPoolManager> weakConnectionPoolManagerRef, _) in s_allConnectionPools) | ||
{ | ||
if (weakConnectionPoolManagerRef.TryGetTarget(out HttpConnectionPoolManager? connectionPoolManager)) | ||
{ | ||
foreach ((_, HttpConnectionPool connectionPool) in connectionPoolManager._pools) | ||
{ | ||
max = Math.Max(max, connectionPool.Http11ConnectionCount); | ||
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. I'd suggest to consider using an associative MaxHeap (i.e. MaxHeap combined with Dictionary to access elements for O(1)) where each node stores open connection count for a single HttpConnectionPool. On connection open or close, the pool will increase or decrease the respective counter. Since each pool will change only their own counter, it can be easily done thread safely. When the polling counter There is one more issue though, that is to ensure Heapify can run concurrently with heap nodes changes. To solve that, I'd suggest to guard Heapify call with 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. Thank you for the idea! While having a better O() when collecting the maximum on polling intervals, I am doubtful it would be a measurable perf improvement at the end, especially considering the added complexity. The current approach tries to minimize the overhead when telemetry is disabled. The proposed one would incur an overhead on every connection even when telemetry is disabled. While the O(N) for this collection might seem scary, it only runs once per polling interval, and the N is effectively capped at the port limit, so I doubt it would show up as a significant overhead. 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.
That's not exactly so. If telemetry is disabled increase/decrease counter methods will be no-op and Heapify will never be called since timer will be disabled as well. However, I do agree it adds more complexity. |
||
} | ||
} | ||
} | ||
return max; | ||
} | ||
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. Instead of maintaining a collection and looping, can you modify 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. I still question the value of these max counters. Wouldn't a current count be more useful? You could approximate a max on top of it, but you can't do the inverse. 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. I agree. A current count seems a lot more useful. 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.
The maximum reported here can fluctuate up and down, from what I understand that approach would only allow growing the max over time. I also don't see the value of this information. @davidsh if you have the time, could you share what use case you had in mind? By "current count", I assume you mean a sum of all opened http11 connections / all active Http2 streams? (in which case this global list mess isn't needed since we can just increment/decrement a global counter) 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.
Yup 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.
Does it really make sense to count total number of streams on all connections? I think it would be better to count HTTP/2 connections only. HTTP/2 stream is a logical entity in the nutshell whereas HTTP/2 connection is a physical one consuming limited resources (in example SNAT ports). |
||
|
||
public static int GetMaxHttp20StreamsPerConnection() | ||
{ | ||
int max = 0; | ||
foreach ((WeakReference<HttpConnectionPoolManager> weakConnectionPoolManagerRef, _) in s_allConnectionPools) | ||
{ | ||
if (weakConnectionPoolManagerRef.TryGetTarget(out HttpConnectionPoolManager? connectionPoolManager)) | ||
{ | ||
foreach ((_, HttpConnectionPool connectionPool) in connectionPoolManager._pools) | ||
{ | ||
max = Math.Max(max, connectionPool.Http20StreamCount); | ||
} | ||
} | ||
} | ||
return max; | ||
} | ||
|
||
internal readonly struct HttpConnectionKey : IEquatable<HttpConnectionKey> | ||
{ | ||
public readonly HttpConnectionKind Kind; | ||
|
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.
Could we some how make it more friendly to upcoming multiple HTTP/2 connection API?