Reduce thread consumption for sync HTTP/2 reads #32917
Conversation
The response Stream returned from SocketsHttpHandler.SendAsync supports all of the various forms of read operations, including synchronous reads. For HTTP/1.x, we're able to implement these as sync all the way down to synchronous calls on the underlying Socket, as every request/response owns its own connection (at least for the duration of the operation; the connection may be returned to the pool for subsequent reuse after). For HTTP/2, however, a single connection is shared across all requests multiplexed onto it, and as such, reads on those individual responses need to coordinate, which can mean waiting one's turn. Such waiting for asynchronous operations is implemented via async/await and a ManualResetValueTaskSource-based implementation. For the sync operations, though, they're implemented as simply blocking waiting on the async equivalent, aka sync-over-async. This not only blocks the calling thread, but because the MRVTS-based implementation specifies RunContinuationsAsynchronously==true (which is important for reliability with how the async code paths are structured) it means that, in order to unblock this waiter, another thread pool thread needs to be available to run the small continuation that will in turn unblock the sync primitive being blocked on. This PR tweaks the use of MRVTS so that when we perform a synchronous read, we set MRVTS.RunContinuationsAsynchronously to false. Since the only action that will be taken as part of the continuation is to set a sync primitive, we don't need to worry about arbitrary code running as part of the completing call stack. This in turn means that in the common case, sync reads will still end up blocking the calling thread _but_ won't require an additional thread pool thread to unblock it, other than for the task running the main connection loop.
wfurt
left a comment
wfurt
left a comment
There was a problem hiding this comment.
LGTM.
Is there a way how to add test that this is finishing synchronously (e.gh. similar to what you did while back on SslStream)
This case is a bit different. There, we were validating that the whole call chain was actually happening synchronously, down to the underlying calls on the underlying Stream. Here, we know the calls are happening asynchronously and we're blocking to accomodate that; the goal of the fix is really just to wake up that blocking without queueing an additional work item that will need a thread pool thread to run it. I can't think of a reasonable way to validate that with a functional test with the way things are configured today. |
|
thanks for clarification @stephentoub |
|
/azp list |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Outerloop failure is #2131 |
ghost
locked as resolved and limited conversation to collaborators
2 participants
The response Stream returned from SocketsHttpHandler.SendAsync supports all of the various forms of read operations, including synchronous reads.
For HTTP/1.x, we're able to implement these as sync all the way down to synchronous calls on the underlying Socket, as every request/response owns its own connection (at least for the duration of the operation; the connection may be returned to the pool for subsequent reuse after).
For HTTP/2, however, a single connection is shared across all requests multiplexed onto it, and as such, reads on those individual responses need to coordinate, which can mean waiting one's turn. Such waiting for asynchronous operations is implemented via async/await and a ManualResetValueTaskSource-based implementation. For the sync operations, though, they're implemented as simply blocking waiting on the async equivalent, aka sync-over-async. This not only blocks the calling thread, but because the MRVTS-based implementation specifies RunContinuationsAsynchronously==true (which is important for reliability with how the async code paths are structured) it means that, in order to unblock this waiter, another thread pool thread needs to be available to run the small continuation that will in turn unblock the sync primitive being blocked on.
This PR tweaks the use of MRVTS so that when we perform a synchronous read, we set MRVTS.RunContinuationsAsynchronously to false. Since the only action that will be taken as part of the continuation is to set a sync primitive, we don't need to worry about arbitrary code running as part of the completing call stack. This in turn means that in the common case, sync reads will still end up blocking the calling thread but won't require an additional thread pool thread to unblock it, other than for the task running the main connection loop.
cc: @scalablecory, @ManickaP
Related to #32125