From fe9215b190d8dc493c96e8f5fc1d3f3a332b5537 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 15 Dec 2021 12:04:26 -0800 Subject: [PATCH 01/15] add test --- .../SocketsHttpHandlerTest.Cancellation.cs | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs index e232ee9441989..ce56e06a1f6ed 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.IO; +using System.Net.Sockets; using System.Net.Test.Common; using System.Threading; using System.Threading.Tasks; @@ -104,6 +105,70 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => options: new GenericLoopbackOptions() { UseSsl = false }); } + [OuterLoop] + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ConnectionFailure_AfterInitialRequestCancelled_SecondRequestSucceedsOnNewConnection(bool useSsl) + { + if (UseVersion == HttpVersion.Version30) + { + // HTTP3 does not support ConnectCallback + return; + } + + await LoopbackServerFactory.CreateClientAndServerAsync(async uri => + { + int connectCount = 0; + + TaskCompletionSource tcsFirstRequestCanceled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + using (var handler = CreateHttpClientHandler()) + using (var client = new HttpClient(handler)) + { + var socketsHandler = GetUnderlyingSocketsHttpHandler(handler); + socketsHandler.ConnectCallback = async (context, token) => + { + // Wait until first request is cancelled and has completed + await tcsFirstRequestCanceled.Task; + + if (Interlocked.Increment(ref connectCount) == 1) + { + // Fail the first connection attempt + throw new Exception("Connect failed"); + } + else + { + // Succeed the second connection attempt + Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true }; + await socket.ConnectAsync(context.DnsEndPoint, token); + return new NetworkStream(socket, ownsSocket: true); + } + }; + + using CancellationTokenSource cts = new CancellationTokenSource(); + Task t1 = client.SendAsync(TestAsync, new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, cts.Token); + Task t2 = client.SendAsync(TestAsync, new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, default); + + // Cancel the first message and wait for it to complete + cts.Cancel(); + await Assert.ThrowsAnyAsync(() => t1); + + // Signal connections to proceed + tcsFirstRequestCanceled.SetResult(); + + // Second request should succeed, even though the first connection failed + HttpResponseMessage resp2 = await t2; + Assert.Equal(HttpStatusCode.OK, resp2.StatusCode); + Assert.Equal("Hello world", await resp2.Content.ReadAsStringAsync()); + } + }, async server => + { + await server.AcceptConnectionSendResponseAndCloseAsync(content: "Hello world"); + }, + options: new GenericLoopbackOptions() { UseSsl = useSsl }); + } + [OuterLoop("Incurs significant delay")] [Fact] public async Task Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent() From a55053f239d3f15e32c3ccc3e6be71449b44af55 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 15 Dec 2021 13:13:02 -0800 Subject: [PATCH 02/15] implement for HTTP/1.1 --- .../SocketsHttpHandler/HttpConnectionPool.cs | 42 +++++++++++++++---- .../SocketsHttpHandlerTest.Cancellation.cs | 23 ++++++++-- 2 files changed, 54 insertions(+), 11 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 75b83fbe8bae6..d7e40f50e5b0a 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 @@ -455,12 +455,12 @@ private async Task AddHttp11ConnectionAsync(HttpRequestMessage request) } catch (OperationCanceledException oce) when (oce.CancellationToken == cts.Token) { - HandleHttp11ConnectionFailure(CreateConnectTimeoutException(oce)); + HandleHttp11ConnectionFailure(request, CreateConnectTimeoutException(oce)); return; } catch (Exception e) { - HandleHttp11ConnectionFailure(e); + HandleHttp11ConnectionFailure(request, e); return; } } @@ -483,6 +483,8 @@ private void CheckForHttp11ConnectionInjection() _http11RequestQueue.Count > _pendingHttp11ConnectionCount && // More requests queued than pending connections _associatedHttp11ConnectionCount < _maxHttp11Connections) // Under the connection limit { +// Console.WriteLine($"Adding new HTTP/1.1 connection. _http11RequestQueue.Count={_http11RequestQueue.Count}, _pendingHttp11ConnectionCount={_pendingHttp11ConnectionCount}"); + _associatedHttp11ConnectionCount++; _pendingHttp11ConnectionCount++; @@ -601,12 +603,12 @@ private async Task HandleHttp11Downgrade(HttpRequestMessage request, Socket? soc } catch (OperationCanceledException oce) when (oce.CancellationToken == cancellationToken) { - HandleHttp11ConnectionFailure(CreateConnectTimeoutException(oce)); + HandleHttp11ConnectionFailure(request, CreateConnectTimeoutException(oce)); return; } catch (Exception e) { - HandleHttp11ConnectionFailure(e); + HandleHttp11ConnectionFailure(request, e); return; } @@ -1573,10 +1575,12 @@ private async ValueTask EstablishProxyTunnelAsync(bool async, HttpReques return (socket, stream); } - private void HandleHttp11ConnectionFailure(Exception e) + private void HandleHttp11ConnectionFailure(HttpRequestMessage request, Exception e) { if (NetEventSource.Log.IsEnabled()) Trace("HTTP/1.1 connection failed"); + bool failRequest; + TaskCompletionSourceWithCancellation? waiter; lock (SyncObj) { Debug.Assert(_associatedHttp11ConnectionCount > 0); @@ -1585,11 +1589,20 @@ private void HandleHttp11ConnectionFailure(Exception e) _associatedHttp11ConnectionCount--; _pendingHttp11ConnectionCount--; - // Fail the next queued request (if any) with this error. - _http11RequestQueue.TryFailNextRequest(e); + // If the request that caused this connection attempt is still pending, fail it. + // Otherwise, the request must have been canceled or satisfied by another connection already. + failRequest = _http11RequestQueue.TryDequeueSpecificRequest(request, out waiter); +// Console.WriteLine($"HandleHttp11ConnectionFailure: failRequest={failRequest}, e={e}"); +// Console.WriteLine("CheckForHttp11ConnectionInjection() from InvalidateHttp11Connection"); CheckForHttp11ConnectionInjection(); } + + if (failRequest) + { + // This may fail if the request was already canceled, but we don't care. + waiter!.TrySetException(e); + } } private void HandleHttp2ConnectionFailure(Exception e) @@ -1624,6 +1637,7 @@ public void InvalidateHttp11Connection(HttpConnection connection, bool disposing _associatedHttp11ConnectionCount--; +// Console.WriteLine("CheckForHttp11ConnectionInjection() from InvalidateHttp11Connection"); CheckForHttp11ConnectionInjection(); } } @@ -2142,6 +2156,20 @@ public TaskCompletionSourceWithCancellation EnqueueRequest(HttpRequestMessage return waiter; } + public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen(false)] out TaskCompletionSourceWithCancellation waiter) + { + if (_queue is not null && _queue.TryPeek(out QueueItem item) && item.Request == request) + { + _queue.Dequeue(); + waiter = item.Waiter; + return true; + } + + waiter = null; + return false; + } + + // TODO: Remove public bool TryFailNextRequest(Exception e) { Debug.Assert(e is HttpRequestException or OperationCanceledException, "Unexpected exception type for connection failure"); diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs index ce56e06a1f6ed..cdfc33a58c637 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -117,6 +117,12 @@ public async Task ConnectionFailure_AfterInitialRequestCancelled_SecondRequestSu return; } + if (!TestAsync) + { + // Test relies on ordering of async operations, so we can't test the sync case + return; + } + await LoopbackServerFactory.CreateClientAndServerAsync(async uri => { int connectCount = 0; @@ -124,8 +130,9 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => TaskCompletionSource tcsFirstRequestCanceled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); using (var handler = CreateHttpClientHandler()) - using (var client = new HttpClient(handler)) + using (var client = CreateHttpClient(handler)) { + handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates; var socketsHandler = GetUnderlyingSocketsHttpHandler(handler); socketsHandler.ConnectCallback = async (context, token) => { @@ -134,26 +141,34 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => if (Interlocked.Increment(ref connectCount) == 1) { + Console.WriteLine("First connection failed"); + // Fail the first connection attempt - throw new Exception("Connect failed"); + throw new Exception("Failing first connection"); } else { // Succeed the second connection attempt Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true }; await socket.ConnectAsync(context.DnsEndPoint, token); + + + Console.WriteLine("Second connection succeeded"); + return new NetworkStream(socket, ownsSocket: true); } }; using CancellationTokenSource cts = new CancellationTokenSource(); - Task t1 = client.SendAsync(TestAsync, new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, cts.Token); - Task t2 = client.SendAsync(TestAsync, new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, default); + Task t1 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, cts.Token); + Task t2 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, default); // Cancel the first message and wait for it to complete cts.Cancel(); await Assert.ThrowsAnyAsync(() => t1); + Console.WriteLine("First request canceled"); + // Signal connections to proceed tcsFirstRequestCanceled.SetResult(); From 9b580780e85ab7e74f4ae457547a9f03384f4c77 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Tue, 14 Dec 2021 09:49:14 -0800 Subject: [PATCH 03/15] add TryDequeueRequest --- .../SocketsHttpHandler/HttpConnectionPool.cs | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 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 d7e40f50e5b0a..9ebbb66b8621c 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 @@ -574,8 +574,11 @@ private async Task HandleHttp11Downgrade(HttpRequestMessage request, Socket? soc _pendingHttp2Connection = false; // Signal to any queued HTTP2 requests that they must downgrade. - while (_http2RequestQueue.TryDequeueNextRequest(null)) - ; + while (_http2RequestQueue.TryDequeueRequest(out RequestQueue.QueueItem item)) + { + // We don't care if this fails; that means the request was previously canceled. + item.Waiter.TrySetResult(null); + } if (_associatedHttp11ConnectionCount < _maxHttp11Connections) { @@ -2136,7 +2139,7 @@ private void Trace(string? message, [CallerMemberName] string? memberName = null private struct RequestQueue { - private struct QueueItem + public struct QueueItem { public HttpRequestMessage Request; public TaskCompletionSourceWithCancellation Waiter; @@ -2156,6 +2159,7 @@ public TaskCompletionSourceWithCancellation EnqueueRequest(HttpRequestMessage return waiter; } +<<<<<<< HEAD public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen(false)] out TaskCompletionSourceWithCancellation waiter) { if (_queue is not null && _queue.TryPeek(out QueueItem item) && item.Request == request) @@ -2170,6 +2174,14 @@ public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen } // TODO: Remove +======= + // TODO: TryDequeueNextRequest below was changed to not handle request cancellation. + // TryFailNextRequest should probably be changed similarly -- or rather, callers should just use the new + // TryDequeueRequest call below and then handle canceled requests themselves. + // However, we should fix the issue re cancellation failure first because that will affect this code too. + // TODO: Link cancellation failure issue here. + +>>>>>>> b178c595973... add TryDequeueRequest public bool TryFailNextRequest(Exception e) { Debug.Assert(e is HttpRequestException or OperationCanceledException, "Unexpected exception type for connection failure"); @@ -2193,6 +2205,8 @@ public bool TryFailNextRequest(Exception e) return false; } + // TODO: Remove this in favor of TryDequeueNextRequest below + public bool TryDequeueNextRequest(T connection) { if (_queue is not null) @@ -2213,6 +2227,17 @@ public bool TryDequeueNextRequest(T connection) return false; } + public bool TryDequeueRequest(out QueueItem item) + { + if (_queue is not null) + { + return _queue.TryDequeue(out item); + } + + item = default; + return false; + } + public bool TryPeekNextRequest([NotNullWhen(true)] out HttpRequestMessage? request) { if (_queue is not null) From f3a7a9b1f2ff32d84974080a8e9829aa2020a781 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Tue, 14 Dec 2021 10:15:26 -0800 Subject: [PATCH 04/15] rework ReturnHttp11Connection --- .../SocketsHttpHandler/HttpConnectionPool.cs | 75 ++++++++++++------- 1 file changed, 50 insertions(+), 25 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 9ebbb66b8621c..2e1f6f5039d9a 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 @@ -1701,44 +1701,69 @@ public void ReturnHttp11Connection(HttpConnection connection, bool isNewConnecti return; } - lock (SyncObj) + // Loop in case we get a cancelled request. + while (true) { - Debug.Assert(!_availableHttp11Connections.Contains(connection)); - - if (isNewConnection) + TaskCompletionSourceWithCancellation? waiter = null; + bool added = false; + lock (SyncObj) { - Debug.Assert(_pendingHttp11ConnectionCount > 0); - _pendingHttp11ConnectionCount--; - } + Debug.Assert(!_availableHttp11Connections.Contains(connection), $"Connection already in available list"); + Debug.Assert(_associatedHttp11ConnectionCount > 0, $"Expected {_associatedHttp11ConnectionCount} > 0"); + Debug.Assert(_associatedHttp11ConnectionCount <= _maxHttp11Connections, $"Expected {_associatedHttp11ConnectionCount} <= {_maxHttp11Connections}"); - if (_http11RequestQueue.TryDequeueNextRequest(connection)) - { - Debug.Assert(_availableHttp11Connections.Count == 0, $"With {_availableHttp11Connections.Count} available HTTP/1.1 connections, we shouldn't have a waiter."); + if (isNewConnection) + { + Debug.Assert(_pendingHttp11ConnectionCount > 0); + _pendingHttp11ConnectionCount--; + isNewConnection = false; + } - if (NetEventSource.Log.IsEnabled()) connection.Trace("Dequeued waiting HTTP/1.1 request."); - return; + if (_http11RequestQueue.TryDequeueRequest(out RequestQueue.QueueItem item)) + { + Debug.Assert(_availableHttp11Connections.Count == 0, $"With {_availableHttp11Connections.Count} available HTTP/1.1 connections, we shouldn't have a waiter."); + waiter = item.Waiter; + } + else if (!_disposed) + { + // Add connection to the pool. + added = true; + _availableHttp11Connections.Add(connection); + Debug.Assert(_availableHttp11Connections.Count <= _associatedHttp11ConnectionCount, $"Expected {_availableHttp11Connections.Count} <= {_associatedHttp11ConnectionCount}"); + } + + // If the pool has been disposed of, we will dispose the connection below outside the lock. + // We do this after processing the queue above so that any queued requests will be handled properly. } - if (_disposed) + if (waiter is not null) { - // If the pool has been disposed of, dispose the connection being returned, - // as the pool is being deactivated. We do this after the above in order to - // use pooled connections to satisfy any requests that pended before the - // the pool was disposed of. - if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing connection returned to pool. Pool was disposed."); + Debug.Assert(!added); + if (waiter.TrySetResult(connection)) + { + if (NetEventSource.Log.IsEnabled()) connection.Trace("Dequeued waiting HTTP/1.1 request."); + return; + } + else + { + Debug.Assert(waiter.Task.IsCanceled); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Discarding canceled request from queue."); + // Loop and process the queue again + } } - else + else if (added) { - // Add connection to the pool. - _availableHttp11Connections.Add(connection); - Debug.Assert(_availableHttp11Connections.Count <= _maxHttp11Connections, $"Expected {_availableHttp11Connections.Count} <= {_maxHttp11Connections}"); if (NetEventSource.Log.IsEnabled()) connection.Trace("Put connection in pool."); return; } + else + { + Debug.Assert(_disposed); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing connection returned to pool. Pool was disposed."); + connection.Dispose(); + return; + } } - - // We determined that the connection is no longer usable. - connection.Dispose(); } public void ReturnHttp2Connection(Http2Connection connection, bool isNewConnection) From 0bca9789c1f7f59cd1a6c4f87b665ce072eb4e53 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 10:44:54 -0800 Subject: [PATCH 05/15] fix merge issue --- .../System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 4 +--- 1 file changed, 1 insertion(+), 3 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 2e1f6f5039d9a..8dea1c7771d0c 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 @@ -2184,7 +2184,6 @@ public TaskCompletionSourceWithCancellation EnqueueRequest(HttpRequestMessage return waiter; } -<<<<<<< HEAD public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen(false)] out TaskCompletionSourceWithCancellation waiter) { if (_queue is not null && _queue.TryPeek(out QueueItem item) && item.Request == request) @@ -2199,14 +2198,13 @@ public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen } // TODO: Remove -======= + // TODO: TryDequeueNextRequest below was changed to not handle request cancellation. // TryFailNextRequest should probably be changed similarly -- or rather, callers should just use the new // TryDequeueRequest call below and then handle canceled requests themselves. // However, we should fix the issue re cancellation failure first because that will affect this code too. // TODO: Link cancellation failure issue here. ->>>>>>> b178c595973... add TryDequeueRequest public bool TryFailNextRequest(Exception e) { Debug.Assert(e is HttpRequestException or OperationCanceledException, "Unexpected exception type for connection failure"); From f40d2f0864db4aee333f373fa268b0fa1ec4cc3b Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 15:32:24 -0800 Subject: [PATCH 06/15] rework ReturnHttp2Connection --- .../SocketsHttpHandler/HttpConnectionPool.cs | 158 ++++++++++-------- 1 file changed, 89 insertions(+), 69 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 8dea1c7771d0c..e177d964c3eb1 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 @@ -1709,8 +1709,10 @@ public void ReturnHttp11Connection(HttpConnection connection, bool isNewConnecti lock (SyncObj) { Debug.Assert(!_availableHttp11Connections.Contains(connection), $"Connection already in available list"); - Debug.Assert(_associatedHttp11ConnectionCount > 0, $"Expected {_associatedHttp11ConnectionCount} > 0"); - Debug.Assert(_associatedHttp11ConnectionCount <= _maxHttp11Connections, $"Expected {_associatedHttp11ConnectionCount} <= {_maxHttp11Connections}"); + Debug.Assert(_associatedHttp11ConnectionCount > _availableHttp11Connections.Count, + $"Expected _associatedHttp11ConnectionCount={_associatedHttp11ConnectionCount} > _availableHttp11Connections.Count={_availableHttp11Connections.Count}"); + Debug.Assert(_associatedHttp11ConnectionCount <= _maxHttp11Connections, + $"Expected _associatedHttp11ConnectionCount={_associatedHttp11ConnectionCount} <= _maxHttp11Connections={_maxHttp11Connections}"); if (isNewConnection) { @@ -1729,11 +1731,10 @@ public void ReturnHttp11Connection(HttpConnection connection, bool isNewConnecti // Add connection to the pool. added = true; _availableHttp11Connections.Add(connection); - Debug.Assert(_availableHttp11Connections.Count <= _associatedHttp11ConnectionCount, $"Expected {_availableHttp11Connections.Count} <= {_associatedHttp11ConnectionCount}"); } // If the pool has been disposed of, we will dispose the connection below outside the lock. - // We do this after processing the queue above so that any queued requests will be handled properly. + // We do this after processing the queue above so that any queued requests will be handled by existing connections if possible. } if (waiter is not null) @@ -1747,19 +1748,19 @@ public void ReturnHttp11Connection(HttpConnection connection, bool isNewConnecti else { Debug.Assert(waiter.Task.IsCanceled); - if (NetEventSource.Log.IsEnabled()) connection.Trace("Discarding canceled request from queue."); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Discarding canceled HTTP/1.1 request from queue."); // Loop and process the queue again } } else if (added) { - if (NetEventSource.Log.IsEnabled()) connection.Trace("Put connection in pool."); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Put HTTP/1.1 connection in pool."); return; } else { Debug.Assert(_disposed); - if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing connection returned to pool. Pool was disposed."); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing HTTP/1.1 connection returned to pool. Pool was disposed."); connection.Dispose(); return; } @@ -1779,91 +1780,109 @@ public void ReturnHttp2Connection(Http2Connection connection, bool isNewConnecti _associatedHttp2ConnectionCount--; } - if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing HTTP/2 connection return to pool. Connection lifetime expired."); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing HTTP2 connection return to pool. Connection lifetime expired."); connection.Dispose(); return; } - bool usable = true; - bool poolDisposed = false; - lock (SyncObj) + while (connection.TryReserveStream()) { - Debug.Assert(_availableHttp2Connections is null || !_availableHttp2Connections.Contains(connection)); - Debug.Assert(_associatedHttp2ConnectionCount > (_availableHttp2Connections?.Count ?? 0)); - - if (isNewConnection) + // Loop in case we get a cancelled request. + while (true) { - Debug.Assert(_pendingHttp2Connection); - _pendingHttp2Connection = false; - } - - while (!_http2RequestQueue.IsEmpty) - { - Debug.Assert((_availableHttp2Connections?.Count ?? 0) == 0, $"With {_availableHttp11Connections.Count} available HTTP2 connections, we shouldn't have a waiter."); - - if (!connection.TryReserveStream()) + TaskCompletionSourceWithCancellation? waiter = null; + bool added = false; + lock (SyncObj) { - usable = false; + Debug.Assert(_availableHttp2Connections is null || !_availableHttp2Connections.Contains(connection), $"HTTP2 connection already in available list"); + Debug.Assert(_associatedHttp2ConnectionCount > (_availableHttp2Connections?.Count ?? 0), + $"Expected _associatedHttp2ConnectionCount={_associatedHttp2ConnectionCount} > _availableHttp2Connections.Count={(_availableHttp2Connections?.Count ?? 0)}"); + // TODO: Update HTTP/1.1 assertion similarly + if (isNewConnection) { - // The new connection could not handle even one request, either because it shut down before we could use it for any requests, - // or because it immediately set the max concurrent streams limit to 0. - // We don't want to get stuck in a loop where we keep trying to create new connections for the same request. - // Fail the next request, if any. - HttpRequestException hre = new HttpRequestException(SR.net_http_http2_connection_not_established); - ExceptionDispatchInfo.SetCurrentStackTrace(hre); - _http2RequestQueue.TryFailNextRequest(hre); + Debug.Assert(_pendingHttp2Connection); + _pendingHttp2Connection = false; + isNewConnection = false; } - break; - } - isNewConnection = false; + if (_http2RequestQueue.TryDequeueRequest(out RequestQueue.QueueItem item)) + { + Debug.Assert((_availableHttp2Connections?.Count ?? 0) == 0, $"With {(_availableHttp2Connections?.Count ?? 0)} available HTTP2 connections, we shouldn't have a waiter."); + waiter = item.Waiter; + } + else if (!_disposed) + { + // Add connection to the pool. + added = true; + _availableHttp2Connections ??= new List(); + _availableHttp2Connections.Add(connection); + } - if (!_http2RequestQueue.TryDequeueNextRequest(connection)) - { - connection.ReleaseStream(); - break; + // If the pool has been disposed of, we will dispose the connection below outside the lock. + // We do this after processing the queue above so that any queued requests will be handled by existing connections if possible. } - if (NetEventSource.Log.IsEnabled()) connection.Trace("Dequeued waiting HTTP/2 request."); - } - - // Since we only inject one connection at a time, we may want to inject another now. - CheckForHttp2ConnectionInjection(); - - if (_disposed) - { - // If the pool has been disposed of, we want to dispose the connection being returned, as the pool is being deactivated. - // We do this after the above in order to satisfy any requests that were queued before the pool was disposed of. - Debug.Assert(_associatedHttp2ConnectionCount > (_availableHttp2Connections?.Count ?? 0)); - _associatedHttp2ConnectionCount--; - poolDisposed = true; - } - else if (usable) - { - if (_availableHttp2Connections is null) + if (waiter is not null) { - _availableHttp2Connections = new List(); + Debug.Assert(!added); + if (waiter.TrySetResult(connection)) + { + if (NetEventSource.Log.IsEnabled()) connection.Trace("Dequeued waiting HTTP2 request."); + break; + } + else + { + Debug.Assert(waiter.Task.IsCanceled); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Discarding canceled HTTP2 request from queue."); + // Loop and process the queue again + } + } + else + { + connection.ReleaseStream(); + if (added) + { + if (NetEventSource.Log.IsEnabled()) connection.Trace("Put HTTP2 connection in pool."); + return; + } + else + { + Debug.Assert(_disposed); + if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing HTTP2 connection returned to pool. Pool was disposed."); + connection.Dispose(); + return; + } } - - // Add connection to the pool. - _availableHttp2Connections.Add(connection); - if (NetEventSource.Log.IsEnabled()) connection.Trace("Put HTTP/2 connection in pool."); - return; } } - if (poolDisposed) + if (isNewConnection) { - if (NetEventSource.Log.IsEnabled()) connection.Trace("Disposing HTTP/2 connection returned to pool. Pool was disposed."); + // The new connection could not handle even one request, either because it shut down before we could use it for any requests, + // or because it immediately set the max concurrent streams limit to 0. + // We don't want to get stuck in a loop where we keep trying to create new connections for the same request. + // So, treat this as a connection failure. + // TODO: Need to flow the request + + if (NetEventSource.Log.IsEnabled()) connection.Trace("New HTTP2 connection is unusable due to no available streams."); connection.Dispose(); - return; - } - Debug.Assert(!usable); + HttpRequestException hre = new HttpRequestException(SR.net_http_http2_connection_not_established); + ExceptionDispatchInfo.SetCurrentStackTrace(hre); + HandleHttp2ConnectionFailure(hre); + } + else + { + // Since we only inject one connection at a time, we may want to inject another now. + lock (SyncObj) + { + CheckForHttp2ConnectionInjection(); + } - // We need to wait until the connection is usable again. - DisableHttp2Connection(connection); + // We need to wait until the connection is usable again. + DisableHttp2Connection(connection); + } } /// @@ -2250,6 +2269,7 @@ public bool TryDequeueNextRequest(T connection) return false; } + // TODO: Just return waiter? public bool TryDequeueRequest(out QueueItem item) { if (_queue is not null) From 1b4dc87cee47f92570e5da2bc580181db236f697 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 15:48:26 -0800 Subject: [PATCH 07/15] improve connection failure handling for HTTP2 --- .../SocketsHttpHandler/HttpConnectionPool.cs | 35 ++++++++++++++----- 1 file changed, 26 insertions(+), 9 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 e177d964c3eb1..daf3b1d9aa2b1 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 @@ -659,12 +659,12 @@ private async Task AddHttp2ConnectionAsync(HttpRequestMessage request) } catch (OperationCanceledException oce) when (oce.CancellationToken == cts.Token) { - HandleHttp2ConnectionFailure(CreateConnectTimeoutException(oce)); + HandleHttp2ConnectionFailure(request, CreateConnectTimeoutException(oce)); return; } catch (Exception e) { - HandleHttp2ConnectionFailure(e); + HandleHttp2ConnectionFailure(request, e); return; } } @@ -674,7 +674,7 @@ private async Task AddHttp2ConnectionAsync(HttpRequestMessage request) ValueTask shutdownTask = connection.WaitForShutdownAsync(); // Add the new connection to the pool. - ReturnHttp2Connection(connection, isNewConnection: true); + ReturnHttp2Connection(connection, request, isNewConnection: true); // Wait for connection shutdown. await shutdownTask.ConfigureAwait(false); @@ -1604,14 +1604,18 @@ private void HandleHttp11ConnectionFailure(HttpRequestMessage request, Exception if (failRequest) { // This may fail if the request was already canceled, but we don't care. - waiter!.TrySetException(e); + Debug.Assert(waiter is not null); + bool succeeded = waiter.TrySetException(e); + Debug.Assert(succeeded || waiter.Task.IsCanceled); } } - private void HandleHttp2ConnectionFailure(Exception e) + private void HandleHttp2ConnectionFailure(HttpRequestMessage request, Exception e) { if (NetEventSource.Log.IsEnabled()) Trace("HTTP2 connection failed"); + bool failRequest; + TaskCompletionSourceWithCancellation? waiter; lock (SyncObj) { Debug.Assert(_associatedHttp2ConnectionCount > 0); @@ -1620,11 +1624,20 @@ private void HandleHttp2ConnectionFailure(Exception e) _associatedHttp2ConnectionCount--; _pendingHttp2Connection = false; - // Fail the next queued request (if any) with this error. - _http2RequestQueue.TryFailNextRequest(e); + // If the request that caused this connection attempt is still pending, fail it. + // Otherwise, the request must have been canceled or satisfied by another connection already. + failRequest = _http2RequestQueue.TryDequeueSpecificRequest(request, out waiter); CheckForHttp2ConnectionInjection(); } + + if (failRequest) + { + // This may fail if the request was already canceled, but we don't care. + Debug.Assert(waiter is not null); + bool succeeded = waiter.TrySetException(e); + Debug.Assert(succeeded || waiter.Task.IsCanceled); + } } /// @@ -1767,10 +1780,12 @@ public void ReturnHttp11Connection(HttpConnection connection, bool isNewConnecti } } - public void ReturnHttp2Connection(Http2Connection connection, bool isNewConnection) + public void ReturnHttp2Connection(Http2Connection connection, HttpRequestMessage? request = null, bool isNewConnection = false) { if (NetEventSource.Log.IsEnabled()) connection.Trace($"{nameof(isNewConnection)}={isNewConnection}"); + Debug.Assert(isNewConnection || request is null, "Shouldn't have a request unless the connection is new"); + if (!isNewConnection && CheckExpirationOnReturn(connection)) { lock (SyncObj) @@ -1859,6 +1874,8 @@ public void ReturnHttp2Connection(Http2Connection connection, bool isNewConnecti if (isNewConnection) { + Debug.Assert(request is not null, "Expect request for a new connection"); + // The new connection could not handle even one request, either because it shut down before we could use it for any requests, // or because it immediately set the max concurrent streams limit to 0. // We don't want to get stuck in a loop where we keep trying to create new connections for the same request. @@ -1870,7 +1887,7 @@ public void ReturnHttp2Connection(Http2Connection connection, bool isNewConnecti HttpRequestException hre = new HttpRequestException(SR.net_http_http2_connection_not_established); ExceptionDispatchInfo.SetCurrentStackTrace(hre); - HandleHttp2ConnectionFailure(hre); + HandleHttp2ConnectionFailure(request, hre); } else { From a55aa91a309fbdaebe9bda6cfe14d3be804c5aae Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 15:52:09 -0800 Subject: [PATCH 08/15] fix test for HTTP2 plaintext --- .../FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs index cdfc33a58c637..2009d29d47f99 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -160,8 +160,8 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => }; using CancellationTokenSource cts = new CancellationTokenSource(); - Task t1 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, cts.Token); - Task t2 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion }, default); + Task t1 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion, VersionPolicy = HttpVersionPolicy.RequestVersionExact }, cts.Token); + Task t2 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion, VersionPolicy = HttpVersionPolicy.RequestVersionExact }, default); // Cancel the first message and wait for it to complete cts.Cancel(); From 9d6fed1c7689af5edaf0a550a5474a85c7aacfab Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 15:53:29 -0800 Subject: [PATCH 09/15] test cleanup --- .../SocketsHttpHandlerTest.Cancellation.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs index 2009d29d47f99..934a1a3baa521 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -141,8 +141,6 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => if (Interlocked.Increment(ref connectCount) == 1) { - Console.WriteLine("First connection failed"); - // Fail the first connection attempt throw new Exception("Failing first connection"); } @@ -151,10 +149,6 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => // Succeed the second connection attempt Socket socket = new Socket(SocketType.Stream, ProtocolType.Tcp) { NoDelay = true }; await socket.ConnectAsync(context.DnsEndPoint, token); - - - Console.WriteLine("Second connection succeeded"); - return new NetworkStream(socket, ownsSocket: true); } }; @@ -167,8 +161,6 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => cts.Cancel(); await Assert.ThrowsAnyAsync(() => t1); - Console.WriteLine("First request canceled"); - // Signal connections to proceed tcsFirstRequestCanceled.SetResult(); From 69435c993a1db8842ae60eb5291e91eaea2071c1 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 16:06:42 -0800 Subject: [PATCH 10/15] cleanup --- .../SocketsHttpHandler/HttpConnectionPool.cs | 102 ++++-------------- 1 file changed, 19 insertions(+), 83 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 daf3b1d9aa2b1..ae07605982108 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 @@ -473,7 +473,7 @@ private void CheckForHttp11ConnectionInjection() { Debug.Assert(HasSyncObjLock); - if (!_http11RequestQueue.TryPeekNextRequest(out HttpRequestMessage? request)) + if (!_http11RequestQueue.TryPeekRequest(out HttpRequestMessage? request)) { return; } @@ -483,8 +483,6 @@ private void CheckForHttp11ConnectionInjection() _http11RequestQueue.Count > _pendingHttp11ConnectionCount && // More requests queued than pending connections _associatedHttp11ConnectionCount < _maxHttp11Connections) // Under the connection limit { -// Console.WriteLine($"Adding new HTTP/1.1 connection. _http11RequestQueue.Count={_http11RequestQueue.Count}, _pendingHttp11ConnectionCount={_pendingHttp11ConnectionCount}"); - _associatedHttp11ConnectionCount++; _pendingHttp11ConnectionCount++; @@ -574,10 +572,11 @@ private async Task HandleHttp11Downgrade(HttpRequestMessage request, Socket? soc _pendingHttp2Connection = false; // Signal to any queued HTTP2 requests that they must downgrade. - while (_http2RequestQueue.TryDequeueRequest(out RequestQueue.QueueItem item)) + while (_http2RequestQueue.TryDequeueWaiter(out TaskCompletionSourceWithCancellation? waiter)) { // We don't care if this fails; that means the request was previously canceled. - item.Waiter.TrySetResult(null); + bool success = waiter.TrySetResult(null); + Debug.Assert(success || waiter.Task.IsCanceled); } if (_associatedHttp11ConnectionCount < _maxHttp11Connections) @@ -686,7 +685,7 @@ private void CheckForHttp2ConnectionInjection() { Debug.Assert(HasSyncObjLock); - if (!_http2RequestQueue.TryPeekNextRequest(out HttpRequestMessage? request)) + if (!_http2RequestQueue.TryPeekRequest(out HttpRequestMessage? request)) { return; } @@ -1594,10 +1593,8 @@ private void HandleHttp11ConnectionFailure(HttpRequestMessage request, Exception // If the request that caused this connection attempt is still pending, fail it. // Otherwise, the request must have been canceled or satisfied by another connection already. - failRequest = _http11RequestQueue.TryDequeueSpecificRequest(request, out waiter); + failRequest = _http11RequestQueue.TryDequeueWaiterForSpecificRequest(request, out waiter); -// Console.WriteLine($"HandleHttp11ConnectionFailure: failRequest={failRequest}, e={e}"); -// Console.WriteLine("CheckForHttp11ConnectionInjection() from InvalidateHttp11Connection"); CheckForHttp11ConnectionInjection(); } @@ -1626,7 +1623,7 @@ private void HandleHttp2ConnectionFailure(HttpRequestMessage request, Exception // If the request that caused this connection attempt is still pending, fail it. // Otherwise, the request must have been canceled or satisfied by another connection already. - failRequest = _http2RequestQueue.TryDequeueSpecificRequest(request, out waiter); + failRequest = _http2RequestQueue.TryDequeueWaiterForSpecificRequest(request, out waiter); CheckForHttp2ConnectionInjection(); } @@ -1653,7 +1650,6 @@ public void InvalidateHttp11Connection(HttpConnection connection, bool disposing _associatedHttp11ConnectionCount--; -// Console.WriteLine("CheckForHttp11ConnectionInjection() from InvalidateHttp11Connection"); CheckForHttp11ConnectionInjection(); } } @@ -1734,10 +1730,9 @@ public void ReturnHttp11Connection(HttpConnection connection, bool isNewConnecti isNewConnection = false; } - if (_http11RequestQueue.TryDequeueRequest(out RequestQueue.QueueItem item)) + if (_http11RequestQueue.TryDequeueWaiter(out waiter)) { Debug.Assert(_availableHttp11Connections.Count == 0, $"With {_availableHttp11Connections.Count} available HTTP/1.1 connections, we shouldn't have a waiter."); - waiter = item.Waiter; } else if (!_disposed) { @@ -1821,10 +1816,9 @@ public void ReturnHttp2Connection(Http2Connection connection, HttpRequestMessage isNewConnection = false; } - if (_http2RequestQueue.TryDequeueRequest(out RequestQueue.QueueItem item)) + if (_http2RequestQueue.TryDequeueWaiter(out waiter)) { Debug.Assert((_availableHttp2Connections?.Count ?? 0) == 0, $"With {(_availableHttp2Connections?.Count ?? 0)} available HTTP2 connections, we shouldn't have a waiter."); - waiter = item.Waiter; } else if (!_disposed) { @@ -2220,7 +2214,7 @@ public TaskCompletionSourceWithCancellation EnqueueRequest(HttpRequestMessage return waiter; } - public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen(false)] out TaskCompletionSourceWithCancellation waiter) + public bool TryDequeueWaiterForSpecificRequest(HttpRequestMessage request, [MaybeNullWhen(false)] out TaskCompletionSourceWithCancellation waiter) { if (_queue is not null && _queue.TryPeek(out QueueItem item) && item.Request == request) { @@ -2233,88 +2227,30 @@ public bool TryDequeueSpecificRequest(HttpRequestMessage request, [MaybeNullWhen return false; } - // TODO: Remove - - // TODO: TryDequeueNextRequest below was changed to not handle request cancellation. - // TryFailNextRequest should probably be changed similarly -- or rather, callers should just use the new - // TryDequeueRequest call below and then handle canceled requests themselves. - // However, we should fix the issue re cancellation failure first because that will affect this code too. - // TODO: Link cancellation failure issue here. - - public bool TryFailNextRequest(Exception e) - { - Debug.Assert(e is HttpRequestException or OperationCanceledException, "Unexpected exception type for connection failure"); - - if (_queue is not null) - { - // Fail the next queued request (if any) with this error. - while (_queue.TryDequeue(out QueueItem item)) - { - // Try to complete the waiter task. If it's been cancelled already, this will fail. - if (item.Waiter.TrySetException(e)) - { - return true; - } - - // Couldn't transfer to that waiter because it was cancelled. Try again. - Debug.Assert(item.Waiter.Task.IsCanceled); - } - } - - return false; - } - - // TODO: Remove this in favor of TryDequeueNextRequest below - - public bool TryDequeueNextRequest(T connection) - { - if (_queue is not null) - { - while (_queue.TryDequeue(out QueueItem item)) - { - // Try to complete the task. If it's been cancelled already, this will return false. - if (item.Waiter.TrySetResult(connection)) - { - return true; - } - - // Couldn't transfer to that waiter because it was cancelled. Try again. - Debug.Assert(item.Waiter.Task.IsCanceled); - } - } - - return false; - } - - // TODO: Just return waiter? - public bool TryDequeueRequest(out QueueItem item) + public bool TryDequeueWaiter([MaybeNullWhen(false)] out TaskCompletionSourceWithCancellation waiter) { - if (_queue is not null) + if (_queue is not null && _queue.TryDequeue(out QueueItem item)) { - return _queue.TryDequeue(out item); + waiter = item.Waiter; + return true; } - item = default; + waiter = null; return false; } - public bool TryPeekNextRequest([NotNullWhen(true)] out HttpRequestMessage? request) + public bool TryPeekRequest([MaybeNullWhen(false)] out HttpRequestMessage request) { - if (_queue is not null) + if (_queue is not null && _queue.TryPeek(out QueueItem item)) { - if (_queue.TryPeek(out QueueItem item)) - { - request = item.Request; - return true; - } + request = item.Request; + return true; } request = null; return false; } - public bool IsEmpty => Count == 0; - public int Count => (_queue?.Count ?? 0); } } From 38f221811cf5fdfbe95ca0399cb43e34461a119b Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 16:27:09 -0800 Subject: [PATCH 11/15] cleanup HandleHttp11Downgrade --- .../SocketsHttpHandler/HttpConnectionPool.cs | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 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 ae07605982108..b5b285758acf0 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 @@ -561,6 +561,7 @@ private async Task HandleHttp11Downgrade(HttpRequestMessage request, Socket? soc if (NetEventSource.Log.IsEnabled()) Trace("Server does not support HTTP2; disabling HTTP2 use and proceeding with HTTP/1.1 connection"); bool canUse = true; + TaskCompletionSourceWithCancellation? waiter = null; lock (SyncObj) { Debug.Assert(_pendingHttp2Connection); @@ -571,14 +572,6 @@ private async Task HandleHttp11Downgrade(HttpRequestMessage request, Socket? soc _associatedHttp2ConnectionCount--; _pendingHttp2Connection = false; - // Signal to any queued HTTP2 requests that they must downgrade. - while (_http2RequestQueue.TryDequeueWaiter(out TaskCompletionSourceWithCancellation? waiter)) - { - // We don't care if this fails; that means the request was previously canceled. - bool success = waiter.TrySetResult(null); - Debug.Assert(success || waiter.Task.IsCanceled); - } - if (_associatedHttp11ConnectionCount < _maxHttp11Connections) { _associatedHttp11ConnectionCount++; @@ -589,6 +582,23 @@ private async Task HandleHttp11Downgrade(HttpRequestMessage request, Socket? soc // We are already at the limit for HTTP/1.1 connections, so do not proceed with this connection. canUse = false; } + + _http2RequestQueue.TryDequeueWaiter(out waiter); + } + + // Signal to any queued HTTP2 requests that they must downgrade. + while (waiter is not null) + { + if (NetEventSource.Log.IsEnabled()) Trace("Downgrading queued HTTP2 request to HTTP/1.1"); + + // We don't care if this fails; that means the request was previously canceled. + bool success = waiter.TrySetResult(null); + Debug.Assert(success || waiter.Task.IsCanceled); + + lock (SyncObj) + { + _http2RequestQueue.TryDequeueWaiter(out waiter); + } } if (!canUse) From acb7918d62c922381dbd36b64d20dc768588b92a Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 16:33:03 -0800 Subject: [PATCH 12/15] cleanup --- .../System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 4 +--- 1 file changed, 1 insertion(+), 3 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 b5b285758acf0..83a1a3b27e23e 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 @@ -1817,7 +1817,6 @@ public void ReturnHttp2Connection(Http2Connection connection, HttpRequestMessage Debug.Assert(_availableHttp2Connections is null || !_availableHttp2Connections.Contains(connection), $"HTTP2 connection already in available list"); Debug.Assert(_associatedHttp2ConnectionCount > (_availableHttp2Connections?.Count ?? 0), $"Expected _associatedHttp2ConnectionCount={_associatedHttp2ConnectionCount} > _availableHttp2Connections.Count={(_availableHttp2Connections?.Count ?? 0)}"); - // TODO: Update HTTP/1.1 assertion similarly if (isNewConnection) { @@ -2027,6 +2026,7 @@ public bool CleanCacheAndDisposeIfUnused() { TimeSpan pooledConnectionLifetime = _poolManager.Settings._pooledConnectionLifetime; TimeSpan pooledConnectionIdleTimeout = _poolManager.Settings._pooledConnectionIdleTimeout; + long nowTicks = Environment.TickCount64; List? toDispose = null; @@ -2047,8 +2047,6 @@ public bool CleanCacheAndDisposeIfUnused() // will be purged next time around. _usedSinceLastCleanup = false; - long nowTicks = Environment.TickCount64; - ScavengeConnectionList(_availableHttp11Connections, ref toDispose, nowTicks, pooledConnectionLifetime, pooledConnectionIdleTimeout); if (_availableHttp2Connections is not null) { From 3f80eb3613efaff48f3626dd517093172d3446c8 Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Thu, 16 Dec 2021 16:38:20 -0800 Subject: [PATCH 13/15] cleanup --- .../System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 3 +-- 1 file changed, 1 insertion(+), 2 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 83a1a3b27e23e..e3061df7136e6 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 @@ -1883,7 +1883,6 @@ public void ReturnHttp2Connection(Http2Connection connection, HttpRequestMessage // or because it immediately set the max concurrent streams limit to 0. // We don't want to get stuck in a loop where we keep trying to create new connections for the same request. // So, treat this as a connection failure. - // TODO: Need to flow the request if (NetEventSource.Log.IsEnabled()) connection.Trace("New HTTP2 connection is unusable due to no available streams."); connection.Dispose(); @@ -2202,7 +2201,7 @@ private void Trace(string? message, [CallerMemberName] string? memberName = null private struct RequestQueue { - public struct QueueItem + private struct QueueItem { public HttpRequestMessage Request; public TaskCompletionSourceWithCancellation Waiter; From 64f34d922a2d24a6ae7edfa57baa9871353d737d Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 5 Jan 2022 12:14:39 -0800 Subject: [PATCH 14/15] properly decrement _associatedHttp2ConnectionCount when pool is disposed --- .../Net/Http/SocketsHttpHandler/HttpConnectionPool.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 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 e3061df7136e6..ca736201a58d3 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 @@ -1829,16 +1829,19 @@ public void ReturnHttp2Connection(Http2Connection connection, HttpRequestMessage { Debug.Assert((_availableHttp2Connections?.Count ?? 0) == 0, $"With {(_availableHttp2Connections?.Count ?? 0)} available HTTP2 connections, we shouldn't have a waiter."); } - else if (!_disposed) + else if (_disposed) + { + // The pool has been disposed. We will dispose this connection below outside the lock. + // We do this check after processing the request queue so that any queued requests will be handled by existing connections if possible. + _associatedHttp2ConnectionCount--; + } + else { // Add connection to the pool. added = true; _availableHttp2Connections ??= new List(); _availableHttp2Connections.Add(connection); } - - // If the pool has been disposed of, we will dispose the connection below outside the lock. - // We do this after processing the queue above so that any queued requests will be handled by existing connections if possible. } if (waiter is not null) From 6dc7b5e8ce0a3a912f6a267ae39b276b6501681a Mon Sep 17 00:00:00 2001 From: Geoffrey Kizer Date: Wed, 5 Jan 2022 12:25:25 -0800 Subject: [PATCH 15/15] fix race issues in test --- .../SocketsHttpHandlerTest.Cancellation.cs | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs index 934a1a3baa521..0c62b3bc3c0c7 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -127,6 +127,7 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => { int connectCount = 0; + TaskCompletionSource tcsFirstConnectionInitiated = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); TaskCompletionSource tcsFirstRequestCanceled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); using (var handler = CreateHttpClientHandler()) @@ -136,10 +137,26 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => var socketsHandler = GetUnderlyingSocketsHttpHandler(handler); socketsHandler.ConnectCallback = async (context, token) => { + // Note we force serialization of connection creation by waiting on tcsFirstConnectionInitiated below, + // so we don't need to worry about concurrent access to connectCount. + bool isFirstConnection = connectCount == 0; + connectCount++; + + Assert.True(connectCount <= 2); + + if (isFirstConnection) + { + tcsFirstConnectionInitiated.SetResult(); + } + else + { + Assert.True(tcsFirstConnectionInitiated.Task.IsCompletedSuccessfully); + } + // Wait until first request is cancelled and has completed await tcsFirstRequestCanceled.Task; - if (Interlocked.Increment(ref connectCount) == 1) + if (isFirstConnection) { // Fail the first connection attempt throw new Exception("Failing first connection"); @@ -155,6 +172,9 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => using CancellationTokenSource cts = new CancellationTokenSource(); Task t1 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion, VersionPolicy = HttpVersionPolicy.RequestVersionExact }, cts.Token); + + // Wait for the connection attempt to be initiated before we send the second request, to avoid races in connection creation + await tcsFirstConnectionInitiated.Task; Task t2 = client.SendAsync(new HttpRequestMessage(HttpMethod.Get, uri) { Version = UseVersion, VersionPolicy = HttpVersionPolicy.RequestVersionExact }, default); // Cancel the first message and wait for it to complete