Use HttpClient.Send instead of AsyncHelpers.RunSync()

I have noticed that the synchronous Execute methods are just wrapping native HttpClient.SendAsync,

public RestResponse Execute(RestRequest request) => AsyncHelpers.RunSync(() => ExecuteAsync(request));

I was wondering if there already is an implementation or if there is a plan to use the native HttpClient.Send, which was only introduced in NET 5 and onwards?

A some related discussion is here:
https://stackoverflow.com/questions/53529061/whats-the-right-way-to-use-httpclient-synchronously

Is there a problem with current implementation? I am 99.9% sure that HttpClient sync methods are just wrappers over async API. Making RestSharp use .NET 5+ code requires more pragma #if's and there are already too many of those. If I understand if it actually makes sense, and it works better compared with the current implementation, I'd be ready to make a change.

It was my general understanding that async code needs to "spread" through the calling stack and calling the async method synchronously is bad practice. We have this as a coding standard and we won't be using RestSharp for cases like this. If HttpClient is calling async code synchronously, then it would also be concerning, but less so because Microsoft would be managing it and maybe making it true synchronous one day too.

We simply want to save ourselves from hard-to-debug / not-replicable issues that this can cause.

I am not sure. It seems like loads of work. Because it starts with a copy-paste implementation of ExecuteRequestAsync, which bubbles up to ExecuteAsync and DownloadDataAsync, which then bubbles up to all the extensions.

calling the async method synchronously is bad practice

I am not sure how using Send will help. Here's the code from System.Net.Http.HttpMessageHandlerStage.Send, which is called by SocketHttpMessageHandler.Send:

protected internal sealed override HttpResponseMessage Send(HttpRequestMessage request,
    CancellationToken cancellationToken)
{
    ValueTask<HttpResponseMessage> sendTask = SendAsync(request, async: false, cancellationToken);
    return sendTask.IsCompleted ?
        sendTask.Result :
        sendTask.AsTask().GetAwaiter().GetResult();
}

It is way less comprehensive compared to RestSharp async wrapper, which does a lot more than getting an awaiter and then getting it's result.

I am not sure. It seems like loads of work. Because it starts with a copy-paste implementation of ExecuteRequestAsync, which bubbles up to ExecuteAsync and DownloadDataAsync, which then bubbles up to all the extensions.

calling the async method synchronously is bad practice

I am not sure how using Send will help. Here's the code from System.Net.Http.HttpMessageHandlerStage.Send, which is called by SocketHttpMessageHandler.Send:
protected internal sealed override HttpResponseMessage Send(HttpRequestMessage request,
    CancellationToken cancellationToken)
{
    ValueTask<HttpResponseMessage> sendTask = SendAsync(request, async: false, cancellationToken);
    return sendTask.IsCompleted ?
        sendTask.Result :
        sendTask.AsTask().GetAwaiter().GetResult();
}
It is way less comprehensive compared to RestSharp async wrapper, which does a lot more than getting an awaiter and then getting it's result.

I think what you sharing is not running async, and the call stack is not awaited.
See the async: false. If we go a little deeper, none of the await methods would be called:

response = async ?
    await base.SendAsync(request, cts.Token).ConfigureAwait(false) :
    base.Send(request, cts.Token);

    if (async)
    {
        await response.Content.LoadIntoBufferAsync(_maxResponseContentBufferSize, cts.Token).ConfigureAwait(false);
    }
    else
    {
        response.Content.LoadIntoBuffer(_maxResponseContentBufferSize, cts.Token);
    }

This makes me believe that while method signatures are async, the running code never awaits anything, staying on the same thread and is synchronous.

Can you link to the file with the above code? What I get from source link isn't the same.

The .NET 8 code works like this:
HttpClient.Send -> HttpMessageInvoker.Send -> SocketHttpHandler.Send -> HttpMessageHandlerStage.Send

And the last one simply does .GetAwaiter.GetResult(), the code I posted in my first comment.

The above examples were from .NET 5.
I used ReSharper to "step in" using F12 on the "Send" method and browsed the assembly that way.

System.Net.Http.HttpClient.Send(HttpRequestMessage request, HttpCompletionOption completionOption,CancellationToken cancellationToken)
Inside this method it calls SendAsyncCore and has below section:

                // Wait for the send request to complete, getting back the response.
                response = async ?
                    await base.SendAsync(request, cts.Token).ConfigureAwait(false) :
                    base.Send(request, cts.Token);
                if (response == null)
                {
                    throw new InvalidOperationException(SR.net_http_handler_noresponse);
                }

Searching online a bit, I found a Different place that uses similar code at DiagnosticsHandler:
https://source.dot.net/#System.Net.Http/System/Net/Http/DiagnosticsHandler.cs,147

But here is where I end up by browsing using ReSharper. the method starts at line 658, namespace System.Net.Http, public class HttpClient : HttpMessageInvoker

private async ValueTask<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request, HttpCompletionOption completionOption,
    bool async, bool emitTelemetryStartStop, CancellationToken cancellationToken)
{
    // Combines given cancellationToken with the global one and the timeout.
    CancellationTokenSource cts = PrepareCancellationTokenSource(cancellationToken, out bool disposeCts, out long timeoutTime);

    bool buffered = completionOption == HttpCompletionOption.ResponseContentRead &&
                    !string.Equals(request.Method.Method, "HEAD", StringComparison.OrdinalIgnoreCase);

    bool telemetryStarted = false, responseContentTelemetryStarted = false;
    if (HttpTelemetry.Log.IsEnabled())
    {
        if (emitTelemetryStartStop && request.RequestUri != null)
        {
            HttpTelemetry.Log.RequestStart(request);
            telemetryStarted = true;
        }
    }

    // Initiate the send.
    HttpResponseMessage? response = null;
    try
    {
        // Wait for the send request to complete, getting back the response.
        response = async ?
            await base.SendAsync(request, cts.Token).ConfigureAwait(false) :
            base.Send(request, cts.Token);
        if (response == null)
        {
            throw new InvalidOperationException(SR.net_http_handler_noresponse);
        }

        // Buffer the response content if we've been asked to and we have a Content to buffer.
        if (buffered && response.Content != null)
        {
            if (HttpTelemetry.Log.IsEnabled() && telemetryStarted)
            {
                HttpTelemetry.Log.ResponseContentStart();
                responseContentTelemetryStarted = true;
            }

            if (async)
            {
                await response.Content.LoadIntoBufferAsync(_maxResponseContentBufferSize, cts.Token).ConfigureAwait(false);
            }
            else
            {
                response.Content.LoadIntoBuffer(_maxResponseContentBufferSize, cts.Token);
            }
        }

        if (NetEventSource.Log.IsEnabled()) NetEventSource.ClientSendCompleted(this, response, request);
        return response;
    }
    catch (Exception e)
    {
        LogRequestFailed(telemetryStarted);

        response?.Dispose();

        if (e is OperationCanceledException operationException && TimeoutFired(cancellationToken, timeoutTime))
        {
            HandleSendTimeout(operationException);
            throw CreateTimeoutException(operationException);
        }

        HandleFinishSendAsyncError(e, cts);
        throw;
    }
    finally
    {
        if (HttpTelemetry.Log.IsEnabled() && telemetryStarted)
        {
            if (responseContentTelemetryStarted)
            {
                HttpTelemetry.Log.ResponseContentStop();
            }

            HttpTelemetry.Log.RequestStop();
        }

        HandleFinishSendCleanup(cts, disposeCts);
    }
}

This one is just a wrapper, it does nothing more than adding telemetry. I have looked at the one that actually does the work, although it's not easy to find out what is called where because of interfaces and, therefore, implicit resolution.

Still, certainly, in latest .NET that's SocketHttpHandler that does the work. In .NET Framework it's WinHttpHandler.

No interfaces, and the flow is very straightforward in the above example (again, NET 5).
Please ignore the previous "DiagnosticsHandler" URL, I shouldn't have shared it.
Finally found the NET 5 source code here (had to choose NET 5 tag):

https://github.com/dotnet/runtime/blob/4aadfea70082ae23e6c54a449268341e9429434e/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs#L658

I am not looking at .NET 5, it's out of support for two years now.

HttpClient does more or less nothing. HttpClient.Send in main calls the HttpMessageInvoker.Send, which calls _handler.Send(request, cancellationToken);. What is _handler? It depends on the target platform. In most cases, it's SocketHttpHandler, but could be WinHttpHandler or something else for WASM.
That's all HttpClient does. I already described the flow here #2160 (comment).

It goes through here
https://github.com/dotnet/runtime/blob/3eb94d1a816f941dd66f01e62c1af25c108014bc/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs#L578-L586
and then here
https://github.com/dotnet/runtime/blob/3eb94d1a816f941dd66f01e62c1af25c108014bc/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpMessageHandlerStage.cs#L11-L18

That's exactly what I wrote before.

I am a little lost now... This issue is requesting to use HttpClient's Send method under the hood, instead of wrapping SendAsync method using AsyncHelper. Is that not a good improvement for all NET 5+ (NET 6, 8 etc) for Microsoft to handle the "Sync" their way (and prone to change) so that all sync-related issues could be passed to them? Especially seeing that Microsoft handles async stuff differently per versions of .NET.

What I mean is that I don't see any advantage of using Send as its implementation is worse compared to what we have now.

Also, using async helper allows us to easily create extensions for sync calls by combining the helper with async functions. For using Send the whole chain of calls needs to be duplicated. It's quite a lot of work. I would be convinced if there's a clear demonstration that Send does things better, but it doesn't 🤷‍♂️

Navigation Menu

Repository navigation

Use HttpClient.Send instead of AsyncHelpers.RunSync() #2160

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Use HttpClient.Send instead of AsyncHelpers.RunSync() #2160

Description

Activity

alexeyzimarev commented on Dec 15, 2023

Edgaras91 commented on Dec 19, 2023

alexeyzimarev commented on Jan 14, 2024

Edgaras91 commented on Jan 15, 2024

alexeyzimarev commented on May 26, 2024

Edgaras91 commented on May 28, 2024

alexeyzimarev commented on May 28, 2024

Edgaras91 commented on May 29, 2024

alexeyzimarev commented on May 29, 2024

Edgaras91 commented on May 29, 2024

alexeyzimarev commented on May 29, 2024

alexeyzimarev commented on May 29, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions