rework HTTP version handling #51454
rework HTTP version handling #51454
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsRework and try to simplify some of the HTTP version handling in HttpConnectionPool.cs. This is preparation for request queuing changes I'm working on.
|
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Did this add more state machines on the common path? |
Possibly, yeah. I tried to avoid state machines where possible. That said, this is an intermediate step in bigger changes, and I don't think it makes sense to micro-optimize state machine allocation until we know where the code is going to end up. That of course is the problem with state machine optimizations; they are fragile and often break with any sort of refactoring. They are fundamentally at odds with good code hygiene. Which is unfortunate. That said, all this will be fixed with state machine caching, right? :) |
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs
Outdated
Show resolved
Hide resolved
| return await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| HttpConnection http11Connection = await ConstructHttp11ConnectionAsync(async, socket, stream!, transportContext, request, cancellationToken).ConfigureAwait(false); | ||
| ReturnConnection(http11Connection); |
There was a problem hiding this comment.
So we create H/11 connection and immediately return it to the pool. So if I get it right, under certain circumstances, another request can take this connection in the meantime and the currently processed request will get queued instead? And that's what we want, or not, or we do not care?
There was a problem hiding this comment.
Yes, that's possible.
It's a weird scenario because you would have to be issuing both HTTP/1.1 and HTTP/2 requests against the same server (otherwise you wouldn't have any existing HTTP/1.1 requests queued). So I'm not sure we can that much...
But if you do that, and you do have HTTP/1.1 requests queued already, I'd argue that those queued requests should be serviced first. That's what will happen for any other HTTP2 requests that are implicitly queued by the HTTP2 connection creation semaphore. So I'd argue this behavior is both better and more consistent than existing behavior.
|
Actually I didn't notice that |
|
I think that having just one message is perfectly sufficient here. The original thinking was to distinguish between:
But I think it's needlessly detailed. If someone wants to dig into the exact reason, the call stack will tell. |
|
Makes sense, thanks. That said, I suspect the message we want here is |
|
Yes, seems better. |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
Rework and try to simplify some of the HTTP version handling in HttpConnectionPool.cs.
This is preparation for request queuing changes I'm working on.