Skip to content

Added an additional yield point prior to possibly long running initialization #115688

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

Merged
merged 10 commits into from
May 20, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public sealed class SocketsHttpHandler : HttpMessageHandler
{
private readonly HttpConnectionSettings _settings = new HttpConnectionSettings();
private HttpMessageHandlerStage? _handler;
private Task<HttpMessageHandlerStage>? _handlerChainSetupTask;
private Func<HttpConnectionSettings, HttpMessageHandlerStage, HttpMessageHandlerStage>? _decompressionHandlerFactory;
private bool _disposed;

Expand Down Expand Up @@ -598,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);
}

Expand All @@ -620,15 +621,25 @@ protected internal override Task<HttpResponseMessage> SendAsync(HttpRequestMessa
return Task.FromCanceled<HttpResponseMessage>(cancellationToken);
}

HttpMessageHandlerStage handler = _handler ?? SetupHandlerChain();

Exception? error = ValidateAndNormalizeRequest(request);
if (error != null)
{
return Task.FromException<HttpResponseMessage>(error);
}

return handler.SendAsync(request, cancellationToken);
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.
async Task<HttpResponseMessage> CreateHandlerAndSendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
_handlerChainSetupTask ??= Task.Run(SetupHandlerChain);
Copy link
Contributor

@pentp pentp May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to optimize this a bit more and also remove the chance of double-setup you could use new Task(s => SetupHandlerChain(s), this) + CompareExchange + Task.Start (conditionally) and you don't need the method result here (so a void Task is sufficient), can use _handler after the setup task finishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable.
Already merged though. Do you think it justifies a separate issue? I'm not certain, because repeated initialization has been deemed not important.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth the extra logic.

HttpMessageHandlerStage handler = await _handlerChainSetupTask.ConfigureAwait(false);
return await handler.SendAsync(request, cancellationToken).ConfigureAwait(false);
}
}

private static Exception? ValidateAndNormalizeRequest(HttpRequestMessage request)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1069,83 +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<HttpRequestMessage>(kvp.Value, "Request");
TaskStatus requestStatus = GetProperty<TaskStatus>(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<Exception>(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<Exception>(() => { 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))
{
Task sendTask = client.SendAsync(request);
Assert.True(sendTask.IsFaulted);
exception = sendTask.Exception.InnerException;
}
else
{
try
{
client.Send(request);
}
catch (Exception ex)
{
exception = ex;
}
Assert.NotNull(exception);
}

await activityStopTcs.Task;

Assert.IsType<NotSupportedException>(exception);
Assert.Same(exceptionLogged, exception);
}
}
}, UseVersion.ToString(), TestAsync.ToString()).DisposeAsync();
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public async Task SendAsync_ExpectedDiagnosticSourceNewAndDeprecatedEventsLogging()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2595,8 +2595,11 @@ public async Task AfterDisposeSendAsync_GettersUsable_SettersThrow(bool dispose)
else
{
using (var c = new HttpMessageInvoker(handler, disposeHandler: false))
{
await Assert.ThrowsAnyAsync<Exception>(() =>
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);
}

Expand Down
Loading