[QUIC] Stream write cancellation #53304
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsCreated a test to verify write cancellation behavior. Fixed MockStream to also work as expected. Added throwing on msquic returning write canceled status. Fixes #32077
|
|
|
||
| CancellationTokenSource cts = new CancellationTokenSource(); | ||
| var task = stream.WriteAsync(new byte[1024 * 1024], cts.Token); | ||
| cts.Cancel(); |
There was a problem hiding this comment.
This way of testing cancellation is not reliable. Sometimes it seems that task finishes before cancellation occurs. Does anyone have an idea on how to make sure write is not over before cancellation? Pre-cancelling before calling WriteAsync is not appealing to me...
There was a problem hiding this comment.
I can only think of ways which touch MsQuicStream code so no help, sorry.
But I think that adding extra test for pre-cancellation is actually a good idea. I'm looking at the code and it looks like we will send the data even if the token has been cancelled beforehand. Is that expected behavior? Should we do some best-effort token check before we call into msquic? Or did I just overlook something 😊
There was a problem hiding this comment.
Sending 64M instead of 1M helped, at least I didn't get any failures on my setup while running it ~20 times. If someone believes it is not reliable enough, the only option I see here is to remove this test completely. I've added a second one that checks pre-cancelled token.
There was a problem hiding this comment.
Rewrote to an infinite loop until canceled, as per @geoffkizer suggestion
There was a problem hiding this comment.
Could that infinite loop be somehow finite, but still big enough for the test? I already see hanging tests in CI :)
|
Pre-cancellation indeed doesn't work as expected (for several reasons). Will change this PR to draft until I come up with a fix. |
| { | ||
| throw GetConnectionAbortedException(_state); | ||
| } | ||
| await _state.SendResettableCompletionSource.GetTypelessValueTask().ConfigureAwait(false); |
There was a problem hiding this comment.
If I don't wait for it before creating CancellationTokenRegistration, in case of pre-cancelled token, SendResettableCompletionSource.CompleteException fails with InvalidOperationException: Operation is not valid due to the current state of the object. 🤔
There was a problem hiding this comment.
Yeah, we guard the ResettableCompletionSource completion with SendState, but it "breaks" here since we're (ab)using SendResettableCompletionSource for START_COMPLETE as well.
I don't have any better suggestions than what you did here.
| } | ||
|
|
||
| // a write would eventually be canceled | ||
| await Assert.ThrowsAsync<OperationCanceledException>(() => WriteUntilCanceled()); |
There was a problem hiding this comment.
Should we call WriteUntilCanceled().WaitAsync(DefaultTestTimeout) or something similar to ensure that if this does fail we get a proper exception instead of a hanging test? We'd have to be careful the ThrowsAsync doesn't swallow the test timeout.
There was a problem hiding this comment.
I will try that, or making a loop big enough but finite, as @ManickaP suggested, and see what will work best
There was a problem hiding this comment.
LGTM.
I'd still prefer if somebody else (@scalablecory @geoffkizer) give this look as well though.
| { | ||
| throw GetConnectionAbortedException(_state); | ||
| } | ||
| await _state.SendResettableCompletionSource.GetTypelessValueTask().ConfigureAwait(false); |
There was a problem hiding this comment.
Yeah, we guard the ResettableCompletionSource completion with SendState, but it "breaks" here since we're (ab)using SendResettableCompletionSource for START_COMPLETE as well.
I don't have any better suggestions than what you did here.
|
|
||
| CancellationTokenSource cts = new CancellationTokenSource(); | ||
| var task = stream.WriteAsync(new byte[1024 * 1024], cts.Token); | ||
| cts.Cancel(); |
There was a problem hiding this comment.
Could that infinite loop be somehow finite, but still big enough for the test? I already see hanging tests in CI :)
|
@scalablecory @geoffkizer will you take a look or shall I merge? |
ghost
locked as resolved and limited conversation to collaborators
Created a test to verify write cancellation behavior. Fixed MockStream to also work as expected. Added throwing on msquic returning write canceled status.
Fixes #32077