-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HTTP/3] Cancellation of response download not reported correctly on server #56129
Comments
Tagging subscribers to this area: @dotnet/ncl Issue Details
Logs
|
UPD: Ok, I've re-read the description again. So, as I understand the description, client already has sent the request and now is reading a response. Why would server's Please note that some first
|
In a duplex scenario, the client and server sends are still open. The client hasn't finished sending. I'll update the description in the first post to be more explicit. I'm not experienced with reading the logs from HTTP and QUIC event sources but you should be able to see that low-level behavior there. |
Triage: If it is not specific to Duplex, we should try it in 6.0. |
From online discussion: This is not specific to duplex, it will most probably show on disposing not finished request (e.g. upload?) Dispose on Quic stream will send FIN flag on write side - that's why server's Read was returning 0. We need to add HTTP/3 level logic to be sure to differentiate between fully sent and canceled request, and in case of canceled, call AbortWrite on Quic stream before disposing it. |
I've confirmed that it is not specific to duplex. Canceling an in-progress GET request shows the same problem. https://github.com/dotnet/aspnetcore/tree/jamesnk/http3-httpclientbug-disposeresponse Test: GET_ClientCancellationAfterResponseHeaders_RequestAbortRaised |
Thanks @JamesNK! |
Triage: @geoffkizer could you summarize your thoughts about this and I can take a look at fixing this tomorrow. |
There are several ways to cancel an in-flight request. One is to dispose the response body before reading to the end. When this happens, we need to abort the underlying QUIC stream with the appropriate error code (I think it's H3_REQUEST_CANCELLED or something like that). We should abort both the read and the write side of the QUIC stream -- the write side only matters for duplex, but it should be a no op for non-duplex since the write will have already completed by the time the user gets the HttpResponseMessage. Other ways for the user to cancel the request include:
Also note that other failures while processing the request need to result in aborting the underlying QUIC stream as well. For example, if there's an error processing headers, we need to abort with the appropriate H3 error code for invalid headers. If we get the wrong frame type, we need to abort with the appropriate H3 error for that. (Some of these may actually be connection-level errors, meaning they should kill the connection -- not sure). It looks like we handle this today, but we should double check and also ensure we have tests for this. |
Reopening to track fix in 6.0 for RC2. |
I finally got a chance to test this change and I think the fix isn't complete. I tested my original unit test to see if it was fixed and it still fails. No abort reaches the server. I believe the issue is the fix only works if the request or response has a length specified. runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs Lines 1225 to 1237 in a28a614
|
Hmm, my guess might be wrong. It looks like _requestContentLengthRemaining is set to -1 if there isn't a content-length specified: runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs Lines 362 to 363 in a28a614
I'll keep investigating. It might be an issue in Kestrel. |
Yes, it should work for unknown content lengths. Please let me know if it still doesn't work for you, maybe my tests are covering everything. |
I've looked into this more and I believe the problem is Kestrel doesn't have a way to get notification of the request cancellation from QuicStream in certain situations. Discussing with some folks offline. |
The follow up problem tracked separately in #58229, closing this one. |
Justification for 6.0: Causes data loss / data corruption on server side.
Reproduction: #56129 (comment)
HttpResponseMessage.Dispose()
)QuicStream.ReadAsync
doesn't throwQuicStreamAbortedException
. InsteadQuicStream.ReadAsync
returns a read count of 0.HttpContext.RequestAbort
cancellation token because it never saw the abort.Logs
The text was updated successfully, but these errors were encountered: