Sync API for HttpClient #32125

ManickaP · 2020-02-11T16:29:07Z

Provide sync API on HttpClient, which currently has only async methods.

Motivation

Azure SDK currently exposes synchronous API which does sync-over-async on HttpClient (see HttpClientTransport.cs). It would be much better if we provided them with sync API with proper sync implementation, where feasible.

Also there are lots of existing code bases that have very deep synchronous call stacks. Developers are simply not willing to rewrite these code bases to be asynchronous. If they need to call async only API in these synchronous methods, they use sync-over-async, which then in turn causes "soft" deadlock. We want to provide synchronous APIs for these developers because synchronous APIs, although inefficient, can can help in avoiding these soft deadlocks.

Another advantage of sync API is that it is much easier to grasp. Especially for people with no prior knowledge of asynchronous processing. If someone is starting with HttpClient, they also have to be knowledgeable of C# async/await. Also many examples with ``HttpClient` might be simplified and thus making the entry into .NET easier.

Proposed API

Minimal Necessary Change

This change is based on @stephentoub prototype in stephentoub/corefx@0e4d640. The prototype introduces synchronous API for SocketsHttpHandler and also properly implements it for HTTP 1.1 scenarios (for HTTP 2 does sync-over-async). This proposal extends the existing prototype with sync API on HttpClient. Following changes are a minimal set to achieve synchronous HttpClient.Send behaving trully synchronously at least for HTTP 1.1.

Note that CancellationToken is used in synchronous methods in order to propagate HttpClient timeout and cancellations. For HttpContent it's up for discussion whether to add other overload to CopyTo and SerializeToStream to match the async counterparts.

The prototype/early implementation for the minimal change can be found in my branch https://github.com/ManickaP/runtime/tree/mapichov/sync_http_api.

// The main classes where we want the sync API.
public partial class HttpMessageInvoker : IDisposable
{
    ...
    // Existing async method
    public virtual Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken);

    // Proposed new sync methods
    public virtual HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}
public partial class HttpClient : HttpMessageInvoker
{
    ...
    // Existing async methods
    public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request);
    public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption);
    public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationToken cancellationToken);
    public override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken);
    
    // Proposed new sync methods
    public HttpResponseMessage Send(HttpRequestMessage request, HttpCompletionOption completionOption = default, CancellationToken cancellationToken = default);
    public override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}

// The changes bellow enable proper implementation of the sync API.

// HttpMessageHandler and derived classes
public abstract partial class HttpMessageHandler : IDisposable
{
    ...
    // Existing async method
    protected internal abstract Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken);
    
    // Proposed new sync method
    protected internal virtual HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}
public abstract partial class DelegatingHandler : HttpMessageHandler
{
    ...
    // Proposed new sync method
    protected internal override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}
public abstract partial class HttpClientHandler : HttpMessageHandler
{
    ...
    // Proposed new sync method
    protected internal override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}
public abstract partial class SocketsHttpHandler : HttpMessageHandler
{
    ...
    // Proposed new sync method
    protected internal override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}
public abstract partial class MessageProcessingHandler : DelegatingHandler
{
    ...
    // Proposed new sync method
    protected internal sealed override HttpResponseMessage Send(HttpRequestMessage request, CancellationToken cancellationToken);
}

// HttpContent and derived classes
public abstract partial class HttpContent : IDisposable
{
    ...
    // Existing async methods
    public Task CopyToAsync(Stream stream);
    public Task CopyToAsync(Stream stream, TransportContext context);
    public Task CopyToAsync(Stream stream, TransportContext context, CancellationToken cancellationToken);
    public Task CopyToAsync(Stream stream, CancellationToken cancellationToken);

    protected abstract Task SerializeToStreamAsync(Stream stream, TransportContext context);
    protected virtual Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken);

    // Proposed new sync methods
    public void CopyTo(Stream stream, TransportContext context, CancellationToken cancellationToken);

    protected virtual void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken);
}
public partial class ByteArrayContent : HttpContent
{
    ...
    // Proposed new sync method
    protected override void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken);
}
public partial class MultipartContent : HttpContent, IEnumerable<HttpContent>, IEnumerable
{
    ...
    // Proposed new sync method
    protected override void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken);
}
public sealed partial class ReadOnlyMemoryContent : HttpContent
{
    ...
    // Proposed new sync method
    protected override void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken);
}
public partial class StreamContent : HttpContent
{
    ...
    // Proposed new sync method
    protected override void SerializeToStream(Stream stream, TransportContext context, CancellationToken cancellationToken);
}

Full Change

This includes sync counterparts to all async HttpClient methods. Since their implementation delegates to Send underneath, there's no need to add any other supporting sync methods.
Note that all the sync methods on HttpClient (except for HttpMessageInvoker.Send override) do not need oveloads with CancellationToken. Whether to include them or not is up for discussion.

public partial class HttpClient : HttpMessageInvoker
{
    ...
    // Existing async methods
    public Task<HttpResponseMessage> DeleteAsync(string requestUri);
    public Task<HttpResponseMessage> DeleteAsync(string requestUri, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> DeleteAsync(Uri requestUri);
    public Task<HttpResponseMessage> DeleteAsync(Uri requestUri, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> GetAsync(string requestUri);
    public Task<HttpResponseMessage> GetAsync(string requestUri, HttpCompletionOption completionOption);
    public Task<HttpResponseMessage> GetAsync(string requestUri, HttpCompletionOption completionOption, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> GetAsync(string requestUri, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> GetAsync(Uri requestUri);
    public Task<HttpResponseMessage> GetAsync(Uri requestUri, HttpCompletionOption completionOption);
    public Task<HttpResponseMessage> GetAsync(Uri requestUri, HttpCompletionOption completionOption, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> GetAsync(Uri requestUri, CancellationToken cancellationToken);
    public Task<byte[]> GetByteArrayAsync(string requestUri);
    public Task<byte[]> GetByteArrayAsync(string requestUri, CancellationToken cancellationToken);
    public Task<byte[]> GetByteArrayAsync(Uri requestUri);
    public Task<byte[]> GetByteArrayAsync(Uri requestUri, CancellationToken cancellationToken);
    public Task<Stream> GetStreamAsync(string requestUri);
    public Task<Stream> GetStreamAsync(string requestUri, CancellationToken cancellationToken);
    public Task<Stream> GetStreamAsync(Uri requestUri);
    public Task<Stream> GetStreamAsync(Uri requestUri, CancellationToken cancellationToken);
    public Task<string> GetStringAsync(string requestUri);
    public Task<string> GetStringAsync(string requestUri, CancellationToken cancellationToken);
    public Task<string> GetStringAsync(Uri requestUri);
    public Task<string> GetStringAsync(Uri requestUri, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> PatchAsync(string requestUri, HttpContent content);
    public Task<HttpResponseMessage> PatchAsync(string requestUri, HttpContent content, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> PatchAsync(Uri requestUri, HttpContent content);
    public Task<HttpResponseMessage> PatchAsync(Uri requestUri, HttpContent content, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> PostAsync(string requestUri, HttpContent content);
    public Task<HttpResponseMessage> PostAsync(string requestUri, HttpContent content, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> PostAsync(Uri requestUri, HttpContent content);
    public Task<HttpResponseMessage> PostAsync(Uri requestUri, HttpContent content, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> PutAsync(string requestUri, HttpContent content);
    public Task<HttpResponseMessage> PutAsync(string requestUri, HttpContent content, CancellationToken cancellationToken);
    public Task<HttpResponseMessage> PutAsync(Uri requestUri, HttpContent content);
    public Task<HttpResponseMessage> PutAsync(Uri requestUri, HttpContent content, CancellationToken cancellationToken);
    
    // Proposed new sync methods
    public HttpResponseMessage Delete(string requestUri);
    public HttpResponseMessage Delete(string requestUri, CancellationToken cancellationToken);
    public HttpResponseMessage Delete(Uri requestUri);
    public HttpResponseMessage Delete(Uri requestUri, CancellationToken cancellationToken);
    public HttpResponseMessage Get(string requestUri);
    public HttpResponseMessage Get(string requestUri, HttpCompletionOption completionOption);
    public HttpResponseMessage Get(string requestUri, HttpCompletionOption completionOption, CancellationToken cancellationToken);
    public HttpResponseMessage Get(string requestUri, CancellationToken cancellationToken);
    public HttpResponseMessage Get(Uri requestUri);
    public HttpResponseMessage Get(Uri requestUri, HttpCompletionOption completionOption);
    public HttpResponseMessage Get(Uri requestUri, HttpCompletionOption completionOption, CancellationToken cancellationToken);
    public HttpResponseMessage Get(Uri requestUri, CancellationToken cancellationToken);
    public byte[] GetByteArray(string requestUri);
    public byte[] GetByteArray(string requestUri, CancellationToken cancellationToken);
    public byte[] GetByteArray(Uri requestUri);
    public byte[] GetByteArray(Uri requestUri, CancellationToken cancellationToken);
    public Stream GetStream(string requestUri);
    public Stream GetStream(string requestUri, CancellationToken cancellationToken);
    public Stream GetStream(Uri requestUri);
    public Stream GetStream(Uri requestUri, CancellationToken cancellationToken);
    public string GetString(string requestUri);
    public string GetString(string requestUri, CancellationToken cancellationToken);
    public string GetString(Uri requestUri);
    public string GetString(Uri requestUri, CancellationToken cancellationToken);
    public HttpResponseMessage Patch(string requestUri, HttpContent content);
    public HttpResponseMessage Patch(string requestUri, HttpContent content, CancellationToken cancellationToken);
    public HttpResponseMessage Patch(Uri requestUri, HttpContent content);
    public HttpResponseMessage Patch(Uri requestUri, HttpContent content, CancellationToken cancellationToken);
    public HttpResponseMessage Post(string requestUri, HttpContent content);
    public HttpResponseMessage Post(string requestUri, HttpContent content, CancellationToken cancellationToken);
    public HttpResponseMessage Post(Uri requestUri, HttpContent content);
    public HttpResponseMessage Post(Uri requestUri, HttpContent content, CancellationToken cancellationToken);
    public HttpResponseMessage Put(string requestUri, HttpContent content);
    public HttpResponseMessage Put(string requestUri, HttpContent content, CancellationToken cancellationToken);
    public HttpResponseMessage Put(Uri requestUri, HttpContent content);
    public HttpResponseMessage Put(Uri requestUri, HttpContent content, CancellationToken cancellationToken);
}

@stephentoub @KrzysztofCwalina @dotnet/ncl

The text was updated successfully, but these errors were encountered:

antonfirsov · 2020-02-11T16:42:14Z

Do we have any existing synchronous API methods in .NET accepting CancellationToken?

alnikola · 2020-02-11T16:56:39Z

It would be better to clearly separate the public cancellationToken parameter purpose from HttpClient's own timeout which is set via Timeoutproperty. HttpClient's timeout will get a special handling logic once the PR #2281 is completed whereas the cancellationToken method parameter enables the caller to cancel request as they deem needed.

davidsh · 2020-02-11T18:11:57Z

Do we have any existing synchronous API methods in .NET accepting CancellationToken?

We do not. CancellationToken's are something that was introduced into .NET as part of adding async/await/Task APIs.

In general, we have avoided adding new sync APIs for I/O. But as described above, there is reason now to support sync API for broadly used APIs like HttpClient.

I am curious, though, how the CancellationToken parameter will be used in these APIs. Sync APIs by their nature are difficult to cancel.

Tratcher · 2020-02-11T18:23:10Z

I am curious, though, how the CancellationToken parameter will be used in these APIs. Sync APIs by their nature are difficult to cancel.

How is it any more difficult than implementing timeout support?

scalablecory · 2020-02-11T18:25:24Z

We will need to make the sync HTTP/1 implementation dispose the Socket, as the cancelable async methods won't be used. I think that's probably fine.

Tratcher · 2020-02-11T18:27:15Z

We will need to make the sync HTTP/1 implementation dispose the Socket, as the cancelable async methods won't be used. I think that's probably fine.

That approach has a long history of AVs. Have those been dealt with?

scalablecory · 2020-02-11T18:30:38Z

We will need to make the sync HTTP/1 implementation dispose the Socket, as the cancelable async methods won't be used. I think that's probably fine.

That approach has a long history of AVs. Have those been dealt with?

Does it? I hope so!

scalablecory · 2020-02-11T18:39:06Z

We should consider using default parameters here:

public Task<HttpResponseMessage> GetAsync(string requestUri);
public Task<HttpResponseMessage> GetAsync(string requestUri, HttpCompletionOption completionOption);
public Task<HttpResponseMessage> GetAsync(string requestUri, HttpCompletionOption completionOption, CancellationToken cancellationToken);
public Task<HttpResponseMessage> GetAsync(string requestUri, CancellationToken cancellationToken);

into

public Task<HttpResponseMessage> GetAsync(string requestUri, HttpCompletionOption completionOption = HttpCompletionOption.ResponseContentRead, CancellationToken cancellationToken = default);

ManickaP · 2020-02-11T20:59:26Z

Add CancelationToken: we do not need to add all the HttpClient sync methods with it, we need only the HttpMessageInvoker.Send overload.

Also the CancelationToken handles not just timeout but request cancellation as well (HttpClient.CancelPendingRequests).

Finally, in the prototype, the CancelationToken is regularly checked for cancellation wherever it cannot be passed further down in the call chain.

@alnikola: I've seen your PR, the timeout is still propagated by CancelationToken which would nicely work with the proposed solution.

karelz · 2020-02-11T22:00:30Z

Team discussion:

~~Use default values in HttpClient.Send~~ done
We propose to NOT take the convenience overloads ("full change") - @stephentoub did you have some arguments for taking them?
We are OK to take CancellationTokens for synchronous APIs - we should honor ALSO existing timeouts (e.g. HttpClient.Timeout and stream Timeouts)
- UPDATE on 3/19 -- stream timeouts: We will do the Timeouts prototype separately from sync APIs. We need prototype to see the impact on perf and code complexity. Default value of timeouts should be infinite/-1 to avoid allocating timer/CancellationToken.

tmds · 2020-02-12T06:45:32Z

Is the motivation strong enough for adding sync methods?

C# has great support for async programming.
And sync APIs means blocking threads...

MihaMarkic · 2020-02-12T17:53:06Z

One reason would be legacy code.

Drawaes · 2020-02-12T18:17:02Z

I think adding sync methods is a mistake and if upstream apis are exposing things as sync when there is underlying async that is their mistake to make, if you put these methods in I can see people falling into the trap of using them in a ecosystem that has embraced async. What is the scenario (outside of legacy) that you need a sync API these days. Servers are mostly async (Asp.net core being the most used in this space). UIs should definitely be using async

benaadams · 2020-02-12T18:21:44Z

They aren't sync; they are blocking apis; could they be prefixed or postfixed as such?

 public HttpResponseMessage BlockingGet(string requestUri);

or

 public HttpResponseMessage GetBlocking(string requestUri);

stephentoub · 2020-02-12T18:22:27Z

What is the scenario (outside of legacy) that you need a sync API these days.

Two:

Simple use cases where the scalability of async isn't required, but more importantly...
Consuming APIs that are sync, and where forcing sync-over-async is worse than just sync. This isn't just "legacy", e.g. brand new Azure SDK clients exposing sync methods.

I'll let @Petermarcu and @KrzysztofCwalina speak to Azure SDK, though it's just one example.

stephentoub · 2020-02-12T18:24:03Z

They aren't sync

How so?

stephentoub · 2020-02-12T18:25:39Z

We propose to NOT take the convenience overloads ("full change") - @stephentoub did you have some arguments for taking them?

I'm fine with that (though it hampers the "getting started" case).

davidfowl · 2020-02-12T18:27:59Z

How is HTTP/2 going to work?

stephentoub · 2020-02-12T18:29:29Z

How is HTTP/2 going to work?

There will be some blocking involved; that's unavoidable. HTTP/1.x should be sync all the way down.

Drawaes · 2020-02-12T18:34:29Z

I guess my main issue is now the first thing people will see is the "Send" method and until now async only methods on HttpClient has been a pit of success story, I fear that will change.

stephentoub · 2020-02-12T18:37:07Z

has been a pit of success story

That's exactly the problem... it hasn't been. Lots of call sites that require sync are forced to figure out how to block and then when they do they fall over at even small loads.

stephentoub · 2020-02-12T18:43:32Z

the first thing people will see is the "Send" method

I don't think it is. Forget the alphabetical ordering, we're talking about 1 method on HttpClient among more than 30. GetAync/PostAsync are much more likely to be the "first thing people will see".

We're simply making it possible for sync clients to not suck as much as they currently do. And sometimes you have no choice but to be sync.

jkotas · 2020-02-12T18:47:05Z

We're simply making it possible for sync clients to not suck as much as they currently do.

This API is a performance optimization for a specific use. Do we have any estimates to quantify the improvement we expect to get out this API for these specific use cases, e.g. microbenchmark that demonstrates the improvement?

stephentoub · 2020-02-12T18:54:00Z

This API is a performance optimization for a specific use. Do we have any estimates to quantify the improvement we expect to get out this API for these specific use cases, e.g. microbenchmark that demonstrates the improvement?

Yes. The key is it ends up requiring many more threads in order to sustain throughput. And it can take a long time for the thread pool to ramp up to the necessary level, while in the interim the system can essentially be deadlocked. That means either the app-level dev needs to explicitly set a high min thread count to force the ramp up, or we need to make the pool way more aggressive at thread injection, which harms other cases.

GeraudFabien · 2020-03-23T20:59:38Z

Maybe there could be an external library with the extention methods to add sync to httpclient. I wouldn't put them in .Net for many reason:

If you put in .net for every method async you will have to do an sync method.
- When i was a beginner it was the fact that some sync method exist that make me lost a lot of time. The fact they exist made me believe that i must'n use async method on a sync environment (that why this thread impact me).
- If the common case exist in sync but not a peculiar case you will thinks that it's a complex case to handle or a bad practice and will be lost
- You will have a lot of issue open to ask you for
If you encounter a bug and has to do a lot of work to handle a very uncommon case. If it's not a .net feature you may not handle it or just raise exception. If it's the framework you will have to fix it

There is also the others solutions :

To document httpclient pages to show how to make the call syncronously.
Add to intellisence the cleverness that if a method is sync it propose a code snippet
Add in the documentation of async method a link to a way to do it syncronously

Even after reading the thread i still thinks this is for :

legacy app that will not pass to net framework because they have no budget sync httpclient or not. I highly doubt that sync call will be even discuss since it's only maintenance
People that cannot use async lib. I never accounter one. Even the people that never touch C# find really quickly how to use it. (They start by just calling .Result most of the times). Like benaadams stat e JS on the other hand as just async. I add to that no way to easly found how to await synchronously (We never found it so i don't even know if it's possible).
Framework that don't agree with a best practice that exist for 8 years !!!
Small app for me is not even a good point since it's not more complex to do async await on small app. I agree debug on async await can be harder be it really happend on complex app. When you're app do one or two request in a foreach and you have one layer of async it's unlikely to be hard to debug

-- edit
When i first look into dotnet core (dnx at the time). It was sell as .Net Framework open source where we allow small breaking change.

jhudsoncedaron · 2020-03-23T21:08:54Z

@GeraudFabien : Sometimes you cannot use an async call because you're implementing an interface that isn't async. Note that .Result and .GetAwaiter().GetResult() can deadlock if there's an async method above you on the stack. This isn't about normal complexity problems anymore. This problem hasn't gone away because sometimes you really can't use async because the context doesn't allow it. It is impossible to build an async application that calls a sync library that turns around and calls an async library, and this particular API draws a lot of sync libraries trying to use it.

jhudsoncedaron · 2020-03-23T21:12:29Z

Add in the documentation of async method a link to a way to do it syncronously

I've followed three different recommendations on this, they were all wrong. The real answer is it's impossible to run an async method synchronously.

valeriob · 2020-03-23T21:51:00Z

I'll add my point of view to the motivation.
There are many AspNet MVC 5 applications that where born before async was a thing and are actively developed today (think tens of mloc), we can bring them to aspnet core, and find out that the same code suffers of thread starvation.

GeraudFabien · 2020-03-23T22:28:33Z

@jhudsoncedaron My main point was there are other solution thant implemented it in the runtime. Documentation can point to other implementation that cover the need and are syncronous. I didn't use it for years but https://github.com/restsharp/RestSharp/wiki/Getting-Started isn't async (and seems to be the only real sync client library). There is also https://github.com/jeffijoe/httpclientgoodies.net https://flurl.dev/ that are async but allow to read simply the value in a syncronus way (but i believe this two have the same drawback than .result and end up with deadlock).

@valeriob yes. My point was did your client give you money to migrate you're application. Wether it was yes or no in both case it's not just because of async in Httpclient that you make you're choice. But on the ROI for your projet and the global risk taken. In a nutshell can you promise me that if we add for sync in the native HttpClient will you migrate all you're legacy to dotnet core.

Sync api are just a very very small problem to the migration. The fear of breaking change will block most of the application to migrate until they have a really good reason to. And when they will have a reason to they may consider thinks like nuget change, project.json, tooling upgrade, learning curve, ... Before even thinking of the lack of sync api on httpclient.

davidfowl · 2020-03-24T02:17:27Z

There are many AspNet MVC 5 applications that where born before async was a thing and are actively developed today (think tens of mloc), we can bring them to aspnet core, and find out that the same code suffers of thread starvation.

The thread pool will still starve, because you're still blocking a thread pool thread. It'll just happen slightly slower than if you didn't have the sync APIs.

valeriob · 2020-03-24T07:44:11Z

The thread pool will still starve, because you're still blocking a thread pool thread. It'll just happen slightly slower than if you didn't have the sync APIs.

Since sync over async will consume 2 threads instead of one, that's the core of the problem, am i right ? I'm not sure how it will translate in the performance characteristics of the application.

valeriob · 2020-03-24T08:06:30Z

@valeriob yes. My point was did your client give you money to migrate you're application. Wether it was yes or no in both case it's not just because of async in Httpclient that you make you're choice. But on the ROI for your projet and the global risk taken. In a nutshell can you promise me that if we add for sync in the native HttpClient will you migrate all you're legacy to dotnet core.

You are absolutly right @GeraudFabien, i've migrated many small applications already, and a big one. It all depends on how heavy is the usage of http apis. If your database has an http interface, you will starve pretty quickly when switching to aspnet core, if you use EntityFramework for example, it still have sync api, so the application will behave the same and no change needed.

Anyway my point is, if you do have a sync codebase with heavy usage of httpclient, the client today has to do sync over async, so you need to raise the threadpool limits pretty quickly (we had to do it as soon as we went from MVC to aspnetcore) to survive.
So now there are 2 options going forward :

Migrate the codebase to async (pretty expensive)
If http client sync api lands, and library autors adopt it you gain instant benefits

For example, this is the code that a library has to implement to allow sync apis on top of httpclient async ones, but maybe it's just too late, the cat is out of the bag :D
https://github.com/ravendb/ravendb/blob/9b5313689310f4938c0c2f9a4385e9fad2708cfe/src/Raven.Client/Util/AsyncHelpers.cs

It would be interesting to hear the opinion of libraries autors that are close to customers, for example is it just me suffering from having a sync solution using databases over http ? 😄

KrzysztofCwalina · 2020-03-24T15:47:17Z

The thread pool will still starve, because you're still blocking a thread pool thread. It'll just happen slightly slower than if you didn't have the sync APIs.

I thought starvation is different and better than the "soft deadlock" we are talking about here. A sync app, at some point, will run out of thread-pool threads and so work will be advancing slowly (up to and including timeouts), but it will be advancing. In sync-over-async case, there are fairness issues between the I/O completion threads and threads picking up new work, and so the app might not be able to make progress. At least this is what I understood after talking to @stephentoub.

stephentoub · 2020-03-24T15:55:17Z

I thought starvation is different and better than the "soft deadlock" we are talking about here.

Yes. Here's a (maybe-overly-simplified-but-gets-the-point-across) example. Uncomment just one of Sync/SyncOverAsync/Async and run it:

using System;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

class Program
{
    static void Main()
    {
        var sw = Stopwatch.StartNew();
        Sync();
        //SyncOverAsync();
        //Async();
        Console.WriteLine(sw.Elapsed);
    }

    static void Sync() => Task.WaitAll(Enumerable.Range(0, Environment.ProcessorCount * 10).Select(_ => Task.Run(() => Thread.Sleep(100))).ToArray());
    static void SyncOverAsync() => Task.WaitAll(Enumerable.Range(0, Environment.ProcessorCount * 10).Select(_ => Task.Run(() => Task.Delay(100).Wait())).ToArray());
    static void Async() => Task.WaitAll(Enumerable.Range(0, Environment.ProcessorCount * 10).Select(_ => Task.Run(async () => await Task.Delay(100))).ToArray());
}

I get results like this:

Sync:          00:00:01.1129462
SyncOverAsync: 00:00:08.1362978
Async:         00:00:00.1313958

jhudsoncedaron · 2020-03-24T15:59:59Z

@KrzysztofCwalina The following is actually a hard deadlock. I've hit it.

async ValueTask<string> Method() {
    await Task.Delay(1);
    using (var h = new HttpClient())
        return h.DownloadStringAsync("some url");
}

Everybody's saying it's a soft deadlock. But sometimes the thread pool can't make new threads. Everything grinds to a halt.

(Nobody is disputing the correct fix is to use return await here. Try it with an intervening interface like IReadOnlyDictionary<> and you'll find out you can't.)

stephentoub · 2020-03-24T16:00:58Z

Everybody's saying it's a soft deadlock. But sometimes the thread pool can't make new threads.

On .NET Core? Please share a repro. I'm skeptical it's hitting a thread pool limit, unless you've got other code elsewhere beyond the shown sample (which presumably is missing an await after the return) that's blocking thread pool threads, e.g. if the caller of Method blocked waiting for it synchronously.

jhudsoncedaron · 2020-03-24T16:04:03Z

@stephentoub : We ran out of nonpaged memory due to overload. It took awhile to get any diagnostics because Remote Desktop was dead meat and nothing was being logged. (No nonpaged memory -> no new threads.)

stephentoub · 2020-03-24T16:04:53Z

We ran out of nonpaged memory due to overload.

Sure, if you run out of memory, that's an entirely different ball of wax.

Introduces sync version of HttpClient.Send and all necessary changes to make it work synchronously (e.g.: HttpContent, HttpMessageHandler and their child classes). The change works properly only for HTTP1.1, for unsupported scenarios like HTTP2 throws. Testing the change uses existing tests calling HttpClient.SendAsync and introduces test argument calling the same test twice, once with HttpClient.SendAsync and then with HttpClient.Send. Resolves #32125

Introduces sync version of HttpClient.Send and all necessary changes to make it work synchronously (e.g.: HttpContent, HttpMessageHandler and their child classes). The change works properly only for HTTP1.1, for unsupported scenarios like HTTP2 throws. Testing the change uses existing tests calling HttpClient.SendAsync and introduces test argument calling the same test twice, once with HttpClient.SendAsync and then with HttpClient.Send. Resolves dotnet#32125

casperOne · 2020-10-16T18:20:36Z

While I dislike the idea of introducing a sync mechanism, I understand the need to address real pain points that people are expressing.

However, everything I've seen in this thread suggests adding methods to HttpClient. Has it been considered to introduce a completely new class which exposes the sync methods?

I like this over an interface on HttpClient or adding the sync methods to HttpClient:

It seems that everyone is in agreement that sync methods should only be used in limited situations (prototyping or high performance scenarios); encapsulation in another class would be a great warning sign saying "beware all ye who enter here"
Analyzers would have an easier time determining if sync code is used in async code; when putting it all on HttpClient we'll certainly need an attribute to point to other methods for each async/sync method pair. If it's tucked away in another class, it's much neater (and an attribute would be able to target the sync/async version very accurately because we can use typeof in the attribute)
This allows library authors much more freedom (and doesn't impose restrictions on sync/async support being on the same class) in determining how they want to support async and sync (although I suspect many will not support it at all).
This has smells of ConfigureAwait all over again, except in reverse; MS chose to make synchronization context capturing the default because at the time, most applications that were being developed were desktop applications. Scenarios that didn't use synchronization contexts (remember, ASP.NET even had one) were few and far between. Now, this has changed radically and that decision puts a lot of friction on developers to always use ConfigureAwait in anything not called in a synchronization context (which for .NET core, is almost everything). I'd very much like to prevent that from happening here.

In summary: Give the people sync, but don't muddy up HttpClient or people who have followed the guidance which applies to what I suspect is the majority of application developers out there; I don't doubt that many face real issues because of this, but I can't help but think there's some sort of survivorship bias thing going on with the metrics.

jhudsoncedaron · 2020-10-20T19:54:18Z

Considering on analysis that we shouldn't use the same instance of HttpClient for both sync and async calls (need to disable HTTP/2 for sync calls to really be sync), casperOne is most likely correct. Too bad this idea appeared so late.

kekyo · 2020-11-11T11:19:04Z

Exactly agreed too, casperOne. I feel people will confuse and can't understand how to implement better asynchronous and scalability for safer. (In my experience, they can't understand what is topic for 'logical Task' at now and the interface will mislead)

valeriob · 2020-11-12T14:10:36Z

I've been looking at the httpclient sync api, i'm surprised about the implementation, it's just a GetAwaiter().GetResult() behind a method ? how does this help anybody suffering from thread pool starvation ?

https://github.com/ManickaP/runtime/blob/7844fbf39812bd591dc10dc0fdfbd5fc1137d507/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs#L464

stephentoub · 2020-11-12T14:12:01Z

I've been looking at the httpclient sync api, i'm surprised about the implementation, it's just a GetAwaiter().GetResult() behind a method ? how does this help anybody suffering from thread pool starvation ?

See the Debug.Assert on the previous line. That GetAwaiter().GetResult() isn't blocking because the task will have completed synchronously. The GetResult() is just there to propagate any exceptions.

valeriob · 2020-11-12T14:46:18Z

Thanks @stephentoub

rellis-of-rhindleton · 2020-11-30T18:46:48Z

I know this is closed and I don't want to beat a dead horse... but for the benefit of anybody still wondering why we would want a sync Send method, here's one reason.

In ASP.NET Core (or any app that uses IHostBuilder I assume) you can use ConfigureAppConfiguration to add your own custom configuration providers. Unfortunately this arrangement does not support asynchronous operations. So if you write your own custom provider that, for example, queries a config server over HTTP, that provider has to run synchronously.

A better solution to that particular problem would be to support asynchronous configuration providers. But I can't find that support today.

ManickaP added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http labels Feb 11, 2020

ManickaP self-assigned this Feb 11, 2020

Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Feb 11, 2020

karelz removed the untriaged New issue has not been triaged by the area owner label Feb 11, 2020

karelz added this to the 5.0 milestone Feb 11, 2020

jozkee mentioned this issue Apr 1, 2020

Address .NET 5-specific suggestions for System.Net.Http.Json #34355

Closed

This was referenced Apr 7, 2020

Make sync SslStream.AuthenticateAs overload with SslOptions public #34638

Closed

Sync HttpClient Send #34948

Merged

ManickaP mentioned this issue Jun 5, 2020

HttpContent.ReadAsStream sync API #37495

Closed

ManickaP closed this as completed in #34948 Jun 24, 2020

ManickaP mentioned this issue Jul 2, 2020

Implementations of new HttpContent sync methods. #38635

Merged

ghost locked as resolved and limited conversation to collaborators Dec 30, 2020

Sync API for HttpClient #32125

Sync API for HttpClient #32125

Comments

ManickaP commented Feb 11, 2020 • edited Loading

Motivation

Proposed API

Minimal Necessary Change

Full Change

antonfirsov commented Feb 11, 2020

alnikola commented Feb 11, 2020

davidsh commented Feb 11, 2020

Tratcher commented Feb 11, 2020

scalablecory commented Feb 11, 2020

Tratcher commented Feb 11, 2020

scalablecory commented Feb 11, 2020

scalablecory commented Feb 11, 2020

ManickaP commented Feb 11, 2020

karelz commented Feb 11, 2020 • edited Loading

tmds commented Feb 12, 2020

MihaMarkic commented Feb 12, 2020

Drawaes commented Feb 12, 2020

benaadams commented Feb 12, 2020

stephentoub commented Feb 12, 2020

stephentoub commented Feb 12, 2020

stephentoub commented Feb 12, 2020 • edited Loading

davidfowl commented Feb 12, 2020

stephentoub commented Feb 12, 2020

Drawaes commented Feb 12, 2020

stephentoub commented Feb 12, 2020

stephentoub commented Feb 12, 2020 • edited Loading

jkotas commented Feb 12, 2020

stephentoub commented Feb 12, 2020 • edited Loading

GeraudFabien commented Mar 23, 2020 • edited Loading

jhudsoncedaron commented Mar 23, 2020

jhudsoncedaron commented Mar 23, 2020

valeriob commented Mar 23, 2020 • edited Loading

GeraudFabien commented Mar 23, 2020 • edited Loading

davidfowl commented Mar 24, 2020

valeriob commented Mar 24, 2020

valeriob commented Mar 24, 2020

KrzysztofCwalina commented Mar 24, 2020 • edited Loading

stephentoub commented Mar 24, 2020 • edited Loading

jhudsoncedaron commented Mar 24, 2020

stephentoub commented Mar 24, 2020 • edited Loading

jhudsoncedaron commented Mar 24, 2020 • edited Loading

stephentoub commented Mar 24, 2020

casperOne commented Oct 16, 2020 • edited Loading

jhudsoncedaron commented Oct 20, 2020

kekyo commented Nov 11, 2020

valeriob commented Nov 12, 2020

stephentoub commented Nov 12, 2020

valeriob commented Nov 12, 2020

rellis-of-rhindleton commented Nov 30, 2020

ManickaP commented Feb 11, 2020 •

edited

Loading

karelz commented Feb 11, 2020 •

edited

Loading

stephentoub commented Feb 12, 2020 •

edited

Loading

stephentoub commented Feb 12, 2020 •

edited

Loading

stephentoub commented Feb 12, 2020 •

edited

Loading

GeraudFabien commented Mar 23, 2020 •

edited

Loading

valeriob commented Mar 23, 2020 •

edited

Loading

GeraudFabien commented Mar 23, 2020 •

edited

Loading

KrzysztofCwalina commented Mar 24, 2020 •

edited

Loading

stephentoub commented Mar 24, 2020 •

edited

Loading

stephentoub commented Mar 24, 2020 •

edited

Loading

jhudsoncedaron commented Mar 24, 2020 •

edited

Loading

casperOne commented Oct 16, 2020 •

edited

Loading