From 9fb197d5b726c693f73b69b2116de258ce8513cf Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 13 Apr 2022 17:31:54 +0200 Subject: [PATCH] Avoid issuing connection attempts for already cancelled requests (#66992) (#67226) * Avoid issuing connection attempts for already canceled requests * Cancelled => Canceled * Guard SocketsHttpHandler tests under SocketsHttpHandler.IsSupported --- .../SocketsHttpHandler/HttpConnectionPool.cs | 20 ++++-- .../SocketsHttpHandlerTest.Cancellation.cs | 65 +++++++++++++++++++ .../FunctionalTests/SocketsHttpHandlerTest.cs | 2 +- 3 files changed, 80 insertions(+), 7 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 c51adc80d5c..b5e9778ab54 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 @@ namespace System.Net.Http { Debug.Assert(HasSyncObjLock); - if (!_http11RequestQueue.TryPeekNextRequest(out HttpRequestMessage? request)) + if (!_http11RequestQueue.TryPeekUncanceledRequest(this, out HttpRequestMessage? request)) { return; } @@ -681,7 +681,7 @@ namespace System.Net.Http { Debug.Assert(HasSyncObjLock); - if (!_http2RequestQueue.TryPeekNextRequest(out HttpRequestMessage? request)) + if (!_http2RequestQueue.TryPeekUncanceledRequest(this, out HttpRequestMessage? request)) { return; } @@ -2186,14 +2186,22 @@ namespace System.Net.Http return false; } - public bool TryPeekNextRequest([NotNullWhen(true)] out HttpRequestMessage? request) + public bool TryPeekUncanceledRequest(HttpConnectionPool pool, [MaybeNullWhen(false)] out HttpRequestMessage request) { if (_queue is not null) { - if (_queue.TryPeek(out QueueItem item)) + while (_queue.TryPeek(out QueueItem item)) { - request = item.Request; - return true; + if (item.Waiter.Task.IsCanceled) + { + if (NetEventSource.Log.IsEnabled()) pool.Trace("Discarding canceled request from queue."); + _queue.Dequeue(); + } + else + { + request = item.Request; + return true; + } } } 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 e232ee94419..5cade37f911 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.Cancellation.cs @@ -11,6 +11,7 @@ using Xunit.Abstractions; namespace System.Net.Http.Functional.Tests { + [ConditionalClass(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))] public abstract class SocketsHttpHandler_Cancellation_Test : HttpClientHandler_Cancellation_Test { protected SocketsHttpHandler_Cancellation_Test(ITestOutputHelper output) : base(output) { } @@ -104,6 +105,70 @@ namespace System.Net.Http.Functional.Tests options: new GenericLoopbackOptions() { UseSsl = false }); } + [Fact] + public async Task RequestsCanceled_NoConnectionAttemptForCanceledRequests() + { + if (UseVersion == HttpVersion.Version30) + { + // HTTP3 does not support ConnectCallback + return; + } + + bool seenRequest1 = false; + bool seenRequest2 = false; + bool seenRequest3 = false; + + var uri = new Uri("https://example.com"); + HttpRequestMessage request1 = CreateRequest(HttpMethod.Get, uri, UseVersion, exactVersion: true); + HttpRequestMessage request2 = CreateRequest(HttpMethod.Get, uri, UseVersion, exactVersion: true); + HttpRequestMessage request3 = CreateRequest(HttpMethod.Get, uri, UseVersion, exactVersion: true); + + TaskCompletionSource connectCallbackEntered = new(TaskCreationOptions.RunContinuationsAsynchronously); + TaskCompletionSource connectCallbackGate = new(TaskCreationOptions.RunContinuationsAsynchronously); + + using HttpClientHandler handler = CreateHttpClientHandler(); + handler.MaxConnectionsPerServer = 1; + GetUnderlyingSocketsHttpHandler(handler).ConnectCallback = async (context, cancellation) => + { + if (context.InitialRequestMessage == request1) seenRequest1 = true; + if (context.InitialRequestMessage == request2) seenRequest2 = true; + if (context.InitialRequestMessage == request3) seenRequest3 = true; + + connectCallbackEntered.TrySetResult(); + + await connectCallbackGate.Task.WaitAsync(TestHelper.PassingTestTimeout); + + throw new Exception("No connection"); + }; + using HttpClient client = CreateHttpClient(handler); + + Task request1Task = client.SendAsync(TestAsync, request1); + await connectCallbackEntered.Task.WaitAsync(TestHelper.PassingTestTimeout); + Assert.True(seenRequest1); + + using var request2Cts = new CancellationTokenSource(); + Task request2Task = client.SendAsync(TestAsync, request2, request2Cts.Token); + Assert.False(seenRequest2); + + Task request3Task = client.SendAsync(TestAsync, request3); + Assert.False(seenRequest2); + Assert.False(seenRequest3); + + request2Cts.Cancel(); + + await Assert.ThrowsAsync(() => request2Task).WaitAsync(TestHelper.PassingTestTimeout); + Assert.False(seenRequest2); + Assert.False(seenRequest3); + + connectCallbackGate.SetResult(); + + await Assert.ThrowsAsync(() => request1Task).WaitAsync(TestHelper.PassingTestTimeout); + await Assert.ThrowsAsync(() => request3Task).WaitAsync(TestHelper.PassingTestTimeout); + + Assert.False(seenRequest2); + Assert.True(seenRequest3); + } + [OuterLoop("Incurs significant delay")] [Fact] public async Task Expect100Continue_WaitsExpectedPeriodOfTimeBeforeSendingContent() diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index bfd1e83bfad..80336c5c212 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -1044,7 +1044,7 @@ namespace System.Net.Http.Functional.Tests public SocketsHttpHandlerTest_Cookies_Http11(ITestOutputHelper output) : base(output) { } } - [SkipOnPlatform(TestPlatforms.Browser, "ConnectTimeout is not supported on Browser")] + [ConditionalClass(typeof(SocketsHttpHandler), nameof(SocketsHttpHandler.IsSupported))] public sealed class SocketsHttpHandler_HttpClientHandler_Http11_Cancellation_Test : SocketsHttpHandler_Cancellation_Test { public SocketsHttpHandler_HttpClientHandler_Http11_Cancellation_Test(ITestOutputHelper output) : base(output) { } -- 2.34.1