fix issue where HTTP/1.1 connections were not being created asynchronously

[geoffkizer]

Fixes #56477 (hopefully)

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #56477 (hopefully)

Author:	geoffkizer
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[geoffkizer]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ManickaP]

I found few more places to consider:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Line 599 in 40ed818

http11Connection = await ConstructHttp11ConnectionAsync(false, socket, stream, transportContext, request, cancellationToken).ConfigureAwait(false);

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Line 624 in 40ed818

(Socket? socket, Stream stream, TransportContext? transportContext) = await ConnectAsync(request, false, cts.Token).ConfigureAwait(false);

---

Slightly off-topic, but why do we have bool async in H/2 and H/3 methods? They do not support sync AFAIK and we are throwing if sync and H/2 | H/3 is requested.

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Line 978 in 40ed818

response = await TrySendUsingHttp3Async(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false);

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

Line 991 in 40ed818

response = await TrySendUsingHttp2Async(request, async, doRequestAuth, cancellationToken).ConfigureAwait(false);

[geoffkizer]

I found few more places to consider:

Thanks, I will look at these tomorrow. They definitely look suspicious.

Slightly off-topic, but why do we have bool async in H/2 and H/3 methods?

I honestly don't know. You are right, there shouldn't be any non-async paths in H2 or H3. It may be a legacy from previous code that we just have never cleaned up.

[wfurt]

How would this work for the Sync HttpClient? I did not see any AddHttp11Connection counterpart.
This is not my area but I was under impression that we want to do alway sync operations for the sync client and async for "normal" case. Having the value fixed makes me wonder....

[stephentoub]

The only call site to this method is just queueing it via a Task.Run, so there won't ever be a sync caller.

However, this does raise the point that I think Geoff's previous change around connection pooling does mean that we're never creating connections synchronously when we used to, which has a couple ramifications we should consider. First, it means every socket on Unix will be in a non-blocking state, which will make the synchronous operations performed on that connection more expensive. Second, it means that some number of sync requests will be done sync-over-async as they synchronously block waiting for a connection to be available, whereas previously they would have actually been creating the connection synchronously.

[geoffkizer]

However, this does raise the point that I think Geoff's previous change around connection pooling does mean that we're never creating connections synchronously when we used to

Yes, that's correct.

In the new approach, we don't really have a direct correlation between a connection being created and a request that will use that connection. All requests get put in the queue; any connection that becomes available will be used to handle that request. So requests that cause a new connection to be created are no longer just waiting on that connection to be created; they are waiting on any connection to become available. This benefits both sync and async usage; but as you point out, there are some downsides in the sync case specifically.

First, it means every socket on Unix will be in a non-blocking state, which will make the synchronous operations performed on that connection more expensive. Second, it means that some number of sync requests will be done sync-over-async as they synchronously block waiting for a connection to be available, whereas previously they would have actually been creating the connection synchronously.

Yep, agreed. And unfortunately I don't see great mitigations for either of these issues, short of going back to the old model. I can imagine some minor tweaks, but all of them add nontrivial complexity and I'm not sure they really help the general situation much.

Thoughts?

EDIT: Further thoughts:

I don't think that the second issue is that big of a deal in general. Once you get to steady state usage, creating a new connection isn't that common; hopefully connections are reused frequently, and only fail/timeout occasionally.

The first issue could be mitigated by creating the connection synchronously if the request that causes it to be created is a synchronous request. Assuming that you only use sync APIs, this would mean the socket would stay in sync mode. (And if you are using a mix of sync APIs and async APIs, you are inducing this problem anyway.) However, it has the downside that connection creation will now tie up another threadpool thread, since it would happen synchronously.

[geoffkizer]

Slightly off-topic, but why do we have bool async in H/2 and H/3 methods? They do not support sync AFAIK and we are throwing if sync and H/2 | H/3 is requested.

Yeah, we should probably remove this. I suspect it's a legacy of refactoring and just never got cleaned up.

[geoffkizer]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ManickaP]

LGTM, thanks. Let get this in.
We can clean up the non-sensical async args in 7.0, they don't hurt anything here.