-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[QUIC] Support cancellation of QUIC writes #32077
Comments
Is it documented what the state of the stream is when cancellation occurs? Is it defined what portion of the data was sent? |
@scalablecory @geoffkizer |
Proper reaction to cancellation token passed to WriteAsync is intention of this issue. |
Do we want this cancellation to work as aborting all subsequent writes, or it should only affect current write and allow the next one? Though the second way seems to be conventional for streams (I've checked FileStream behavior), right now I don't see a way to actually cancel a single write we've sent to msquic, it's not in msquic API. If we just stop waiting for SEND_COMPLETE event to arrive, we would still have to wait for it before attempting the next write, and it doesn't make sense from a user's perspective. I may try to investigate how to implement cancel in msquic... this SEND_COMPLETE.Canceled=true now is only happening on aborting the stream or in case of protocol errors like sending after FIN. Maybe this code could be extracted and reused. The main question is whether it will be beneficial to HTTP/3 in any way now? It seems to me that it will not, as the user cancellation would be for a whole request, so we don't need any subsequent writes on that stream. From what I see in the code, we do explicitly abort the stream in case of user's cancellation runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs Lines 259 to 266 in d3c77a5
What we have in QuicStream code now, we switch to Aborted state inside, i.e. go the first route but don't abort the stream upon cancellation. We may choose to send abort for writes upon cancellation, or we may choose to do nothing for now as it doesn't seem to affect HTTP/3, and address cancellation properly in future? (in the meanwhile, reaction to SEND_COMPLETE.Canceled=true still should be implemented) |
I think it's fine to abort all subsequent writes. It's true that some (many?) Streams allow you to perform another write after cancelling a previous one. But with connected streams like QuicStream or NetworkStream, you usually can't rely on the state of the stream after a write cancellation in general. We can always revisit in the future if we want, but I'm not sure there's much value in supporting write after write cancellation. |
Essentially the caller can't know how much was written before cancellation, so the only safe thing for a caller to do is dispose the stream. In my current PR for shutdown support, reads will go into aborted state once canceled. Writes should too. |
As writes already go into aborted state, I guess nothing much to do here then? I will double-check everything works as expected though. The only question I have left, do we want to send AbortWrite implicitly inside QuicStream, or do we want to leave HTTP/3 code explicitly aborting it? I am asking mainly because HTTP/3 code supplies error code inside, so I guess it's better to leave as is? |
Yeah we can't actually protocol-wise abort. The way I'm treating this in reads is that it is "soft aborted" (exceptions will be thrown if you do more reads, etc.) with the expectation that next step caller will take is to actually call |
We could if the user provided an abort code somehow, as we will need them to do for aborting read anyway... |
It feels kind of weird to me that cancelling the write leaves the QuicStream in an "aborted" state, but that the stream on the wire is not actually aborted. Is this true for read as well? |
I do remember a discussion about "default abort code" that we would use e.g. in Dispose in case we are unable do a graceful shutdown. I don't remember what/if we decided about it. |
Yes. If you take a look at the "recommended dispose pattern" sample in the top comment here you'll see why this is probably okay. #52929 A reasonable alternative that I'd be happy with is to have some way to specify which abort codes to use when cancellation happens, but I haven't put in time to see how we can do that and remain flexible for non-HTTP/3 use. |
Yeah, that pattern makes a lot of sense. Thanks.
We have talked about this in the past, specifically with regard to Dispose/Close and aborting the read side. Where are we landing on that? (I should probably just look at your PR before I ask any more questions...) |
Dispose of stream doesn't ever abort - it's graceful like other streams. We do still need to figure out what dispose of connection does, I don't recall if we landed on something. |
Okay, so what will happen if I Dispose a QuicStream without either (a) reading to EOF or (b) aborting the read side myself? |
We wait for full graceful shutdown, which includes receipt of FIN. It'll drain the stream. |
Drain meaning, we are actively reading to EOF? |
Add tests to check write cancellation behavior, fix pre-cancelled writes and fix mock stream. Add throwing on msquic returning write canceled status. Fixes #32077
Current APIs do not report cancellation to the user and should throw an
OperationCanceledException
:runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs
Lines 758 to 759 in 65587ba
The text was updated successfully, but these errors were encountered: