From 5abde3617b7ae24786d31931ed93ac01546e1cad Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Sun, 18 May 2025 02:19:59 +0300 Subject: [PATCH 1/7] Added an additional yield point --- .../SocketsHttpHandler/SocketsHttpHandler.cs | 16 +++++++++++++--- .../tests/FunctionalTests/DiagnosticsTests.cs | 5 ++--- .../FunctionalTests/SocketsHttpHandlerTest.cs | 5 ++++- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index bfced3b900faaf..89457776c7ced8 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -620,15 +620,25 @@ protected internal override Task SendAsync(HttpRequestMessa return Task.FromCanceled(cancellationToken); } - HttpMessageHandlerStage handler = _handler ?? SetupHandlerChain(); - Exception? error = ValidateAndNormalizeRequest(request); if (error != null) { return Task.FromException(error); } - return handler.SendAsync(request, cancellationToken); + async Task CreateHandlerAndSendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding); + + HttpMessageHandlerStage handler = SetupHandlerChain(); + + return await handler.SendAsync(request, cancellationToken) + .ConfigureAwait(false); + } + + return _handler is { } handler + ? handler.SendAsync(request, cancellationToken) + : CreateHandlerAndSendAsync(request, cancellationToken); } private static Exception? ValidateAndNormalizeRequest(HttpRequestMessage request) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs index 589af982341027..c87238e567b5cf 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs @@ -1120,9 +1120,8 @@ await RemoteExecutor.Invoke(async (useVersion, testAsync) => Exception exception = null; if (bool.Parse(testAsync)) { - Task sendTask = client.SendAsync(request); - Assert.True(sendTask.IsFaulted); - exception = sendTask.Exception.InnerException; + exception = await Record.ExceptionAsync( + () => client.SendAsync(request)); } else { diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 804fb32d5361bb..06d80a276d99eb 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -2595,8 +2595,11 @@ public async Task AfterDisposeSendAsync_GettersUsable_SettersThrow(bool dispose) else { using (var c = new HttpMessageInvoker(handler, disposeHandler: false)) + { await Assert.ThrowsAnyAsync(() => - c.SendAsync(new HttpRequestMessage(HttpMethod.Get, new Uri("/shouldquicklyfail", UriKind.Relative)), default)); + c.SendAsync(new HttpRequestMessage(HttpMethod.Get, new Uri("http://www.some.example/shouldquicklyfail", UriKind.Absolute)), default)); + } + expectedExceptionType = typeof(InvalidOperationException); } From 76ae05f51a4758228a6f7a61f5569e9feddfa659 Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Sun, 18 May 2025 18:46:17 +0300 Subject: [PATCH 2/7] Enqueued setup logic to thread pool Also added an explanation comment --- .../Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 89457776c7ced8..53c409bed636f7 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -20,6 +20,7 @@ public sealed class SocketsHttpHandler : HttpMessageHandler { private readonly HttpConnectionSettings _settings = new HttpConnectionSettings(); private HttpMessageHandlerStage? _handler; + private Task? _handlerChainSetupTask; private Func? _decompressionHandlerFactory; private bool _disposed; @@ -626,11 +627,17 @@ protected internal override Task SendAsync(HttpRequestMessa return Task.FromException(error); } + // SetupHandlerChain may block for a few seconds in some environments. + // (See https://github.com/dotnet/runtime/issues/115301. ) + // The setup procedure is being enqueued to thread pool to prevent the caller from blocking async Task CreateHandlerAndSendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding); + if (Volatile.Read(ref _handlerChainSetupTask) is null) + { + _handlerChainSetupTask = Task.Run(SetupHandlerChain); + } - HttpMessageHandlerStage handler = SetupHandlerChain(); + HttpMessageHandlerStage handler = await _handlerChainSetupTask!.ConfigureAwait(false); return await handler.SendAsync(request, cancellationToken) .ConfigureAwait(false); From 9744b461e153ce0545a1db2d590d883510a87ad0 Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Sun, 18 May 2025 18:58:18 +0300 Subject: [PATCH 3/7] Changed validation order to reduce discrepancies between synchronous and asynchronous calls --- .../System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 53c409bed636f7..7baf52bbb00025 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -599,14 +599,14 @@ protected internal override HttpResponseMessage Send(HttpRequestMessage request, cancellationToken.ThrowIfCancellationRequested(); - HttpMessageHandlerStage handler = _handler ?? SetupHandlerChain(); - Exception? error = ValidateAndNormalizeRequest(request); if (error != null) { throw error; } + HttpMessageHandlerStage handler = _handler ?? SetupHandlerChain(); + return handler.Send(request, cancellationToken); } From 4f3c8478773bce4f0f7dffea170c4fa2e6dae0a9 Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Sun, 18 May 2025 22:57:22 +0300 Subject: [PATCH 4/7] Updated comment Co-authored-by: Miha Zupan --- .../System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 7baf52bbb00025..8a1e7f5726b112 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -628,8 +628,8 @@ protected internal override Task SendAsync(HttpRequestMessa } // SetupHandlerChain may block for a few seconds in some environments. - // (See https://github.com/dotnet/runtime/issues/115301. ) - // The setup procedure is being enqueued to thread pool to prevent the caller from blocking + // E.g. during the first access of HttpClient.DefaultProxy - https://github.com/dotnet/runtime/issues/115301. + // The setup procedure is enqueued to thread pool to prevent the caller from blocking. async Task CreateHandlerAndSendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { if (Volatile.Read(ref _handlerChainSetupTask) is null) From 9cba438150016c988147b0358f0c5d4d9b490add Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Sun, 18 May 2025 23:47:01 +0300 Subject: [PATCH 5/7] Removed Volatile.Read to make the code more readable Co-authored-by: Miha Zupan --- .../Http/SocketsHttpHandler/SocketsHttpHandler.cs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 8a1e7f5726b112..10b37cf40a28d0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -632,15 +632,9 @@ protected internal override Task SendAsync(HttpRequestMessa // The setup procedure is enqueued to thread pool to prevent the caller from blocking. async Task CreateHandlerAndSendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - if (Volatile.Read(ref _handlerChainSetupTask) is null) - { - _handlerChainSetupTask = Task.Run(SetupHandlerChain); - } - - HttpMessageHandlerStage handler = await _handlerChainSetupTask!.ConfigureAwait(false); - - return await handler.SendAsync(request, cancellationToken) - .ConfigureAwait(false); + _handlerChainSetupTask ??= Task.Run(SetupHandlerChain); + HttpMessageHandlerStage handler = await _handlerChainSetupTask.ConfigureAwait(false); + return await handler.SendAsync(request, cancellationToken).ConfigureAwait(false); } return _handler is { } handler From 00ef15be09bf01c567979b035cc6e4ebe0290d8f Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Mon, 19 May 2025 01:51:52 +0300 Subject: [PATCH 6/7] Removed the test that is no longer viable --- .../tests/FunctionalTests/DiagnosticsTests.cs | 76 ------------------- 1 file changed, 76 deletions(-) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs index c87238e567b5cf..7517e324ed82ce 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/DiagnosticsTests.cs @@ -1069,82 +1069,6 @@ await RemoteExecutor.Invoke(async (useVersion, testAsync) => }, UseVersion.ToString(), TestAsync.ToString()).DisposeAsync(); } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public async Task SendAsync_ExpectedDiagnosticSynchronousExceptionActivityLogging() - { - await RemoteExecutor.Invoke(async (useVersion, testAsync) => - { - Exception exceptionLogged = null; - - TaskCompletionSource activityStopTcs = new(TaskCreationOptions.RunContinuationsAsynchronously); - - var diagnosticListenerObserver = new FakeDiagnosticListenerObserver(kvp => - { - if (kvp.Key.Equals("System.Net.Http.HttpRequestOut.Stop")) - { - Assert.NotNull(kvp.Value); - GetProperty(kvp.Value, "Request"); - TaskStatus requestStatus = GetProperty(kvp.Value, "RequestTaskStatus"); - Assert.Equal(TaskStatus.Faulted, requestStatus); - activityStopTcs.SetResult(); - } - else if (kvp.Key.Equals("System.Net.Http.Exception")) - { - Assert.NotNull(kvp.Value); - exceptionLogged = GetProperty(kvp.Value, "Exception"); - } - }); - - using (DiagnosticListener.AllListeners.Subscribe(diagnosticListenerObserver)) - { - diagnosticListenerObserver.Enable(); - - using (HttpClientHandler handler = CreateHttpClientHandler(useVersion)) - using (HttpClient client = CreateHttpClient(handler, useVersion)) - { - // Set a ftp proxy. - // Forces a synchronous exception for SocketsHttpHandler. - // SocketsHttpHandler only allow http & https & socks scheme for proxies. - handler.Proxy = new WebProxy($"ftp://foo.bar", false); - var request = new HttpRequestMessage(HttpMethod.Get, InvalidUri) - { - Version = Version.Parse(useVersion) - }; - - // We cannot use Assert.Throws(() => { SendAsync(...); }) to verify the - // synchronous exception here, because DiagnosticsHandler SendAsync() method has async - // modifier, and returns Task. If the call is not awaited, the current test method will continue - // run before the call is completed, thus Assert.Throws() will not capture the exception. - // We need to wait for the Task to complete synchronously, to validate the exception. - - Exception exception = null; - if (bool.Parse(testAsync)) - { - exception = await Record.ExceptionAsync( - () => client.SendAsync(request)); - } - else - { - try - { - client.Send(request); - } - catch (Exception ex) - { - exception = ex; - } - Assert.NotNull(exception); - } - - await activityStopTcs.Task; - - Assert.IsType(exception); - Assert.Same(exceptionLogged, exception); - } - } - }, UseVersion.ToString(), TestAsync.ToString()).DisposeAsync(); - } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public async Task SendAsync_ExpectedDiagnosticSourceNewAndDeprecatedEventsLogging() { From a6b81dc8966e8919153567877b83722285c38595 Mon Sep 17 00:00:00 2001 From: Sergey Ivanov Date: Mon, 19 May 2025 17:09:43 +0300 Subject: [PATCH 7/7] Moved the nested method to adhere to the code style --- .../Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs index 10b37cf40a28d0..1a3961d9300924 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs @@ -627,6 +627,10 @@ protected internal override Task SendAsync(HttpRequestMessa return Task.FromException(error); } + return _handler is { } handler + ? handler.SendAsync(request, cancellationToken) + : CreateHandlerAndSendAsync(request, cancellationToken); + // SetupHandlerChain may block for a few seconds in some environments. // E.g. during the first access of HttpClient.DefaultProxy - https://github.com/dotnet/runtime/issues/115301. // The setup procedure is enqueued to thread pool to prevent the caller from blocking. @@ -636,10 +640,6 @@ async Task CreateHandlerAndSendAsync(HttpRequestMessage req HttpMessageHandlerStage handler = await _handlerChainSetupTask.ConfigureAwait(false); return await handler.SendAsync(request, cancellationToken).ConfigureAwait(false); } - - return _handler is { } handler - ? handler.SendAsync(request, cancellationToken) - : CreateHandlerAndSendAsync(request, cancellationToken); } private static Exception? ValidateAndNormalizeRequest(HttpRequestMessage request)