Always wait for the request content to complete #1271
Conversation
Improve content cancellation
| if (requestContent is not null && requestContent.Started) | ||
| { | ||
| await requestContent.ConsumptionTask; | ||
| } | ||
|
|
||
| // The transforms callback decided that the response body should be discarded. | ||
| destinationResponse.Dispose(); |
There was a problem hiding this comment.
This scenario is really interesting. The caller has decided to discard the response body but it's unclear what the intent is for the request. In a worst case bidirectional streaming scenario the request content might never finish. An echo server would hang since the response isn't being consumed or disposed yet.
At a minimum, dispose the response before waiting for the request so the server knows you're not going to consume it. I think you still need to cancel the request content since it might be waiting for client input.
There was a problem hiding this comment.
Looks like HttpClient will cancel the request content if you dispose the response without reading it.
The server responded in a way that the proxy doesn't want the client to see. Presumably, the server doesn't care about the request either (in which case we should cancel it)? The alternative of potentially waiting for a long time is arguably worse as you mention.
There was a problem hiding this comment.
Makes sense. The one thing to watch out for is if canceling the request body causes the client to error out and not see the proxy response at all.
There was a problem hiding this comment.
if canceling the request body causes the client to error out
As in Kestrel having side effects if a read to its original request body is canceled?
There was a problem hiding this comment.
Right. That will depend a lot on the request and the timing, so it's something we'll need to watch out for in the future to see if it becomes a problem.
Naming, consistency
Also force cancel the request content
samsp-msft
added
the
samsp_list
|
There are two main cases where we try to forcefully cancel the request content:
If
The interesting case is if the user chooses not to copy the body. It appears we may need a CTS we can cancel manually in all cases, not just 3.1. |
Yeah, I think we were going to end up in that position regardless when I did my ActivityTimeout work. |
Avoid canceling the CTS when not required
|
Changed the logic to use the request CTS for manual cancellations. Added a manual cancel to |
Fixes #1100
Ensure that we always wait for the request content copy to complete before returning from
SendAsync.When using HTTP/2, the request content copy is done in parallel. It may still be going on even if
SendAsyncthrows.In such cases, the cancellation token passed to
SerializeToStreamAsyncis signaled. This PR makes sure we combine the tokens appropriately.On 3.1, there is no overload of
SerializeToStreamAsyncthat takes a CT, so we create an extra CTS that we cancel manually in case of an exception.Perf:
On 3.1 we can't detect whether HTTP/1.1 is being used, so we always allocate an extra CTS.
On 5.0+, we allocate an extra CTS only when using HTTP/2 (it would be trivial to pool it).