Avoid cancellation exceptions in intermediate operations#55138
Avoid cancellation exceptions in intermediate operations#55138sharwell merged 6 commits intodotnet:mainfrom
Conversation
| else | ||
| { | ||
| return new SerializableSourceText(await state.GetTextAsync(cancellationToken).ConfigureAwait(false)); | ||
| return SpecializedTasks.TransformWithoutIntermediateCancellationExceptionAsync( |
There was a problem hiding this comment.
i think it woudl be good to have a comment here.
There was a problem hiding this comment.
My intent is to have the method name eliminate the need for a separate comment. Open to suggestions for improving upon the current name.
| // Synchronous fast path if 'func' completes synchronously | ||
| var result = intermediateResult.Result; | ||
| if (cancellationToken.IsCancellationRequested) | ||
| return new ValueTask<TResult>(Task.FromCanceled<TResult>(cancellationToken)); |
There was a problem hiding this comment.
there's no ValueTask constructor to cancel it?
There was a problem hiding this comment.
Don't think so? Will double check.
| // | Direct fault¹ | Not cancelled | Fault result without applying transform | | ||
| // | Direct fault¹ | Cancelled | Cancel result without applying transform | | ||
| // | Indirect fault | Not cancelled | Fault result without applying transform | | ||
| // | Indirect fault | Cancelled | Cancel result without applying transform | |
There was a problem hiding this comment.
what'st eh difference of Direct vs Indirect fault?
There was a problem hiding this comment.
Direct fault means func throws its exception directly (a non-async implementation that throws). Indirect fault means func completes to the point of returning a ValueTask<TIntermediate> instance which (immediately or eventually) transitions to the faulted state. Methods marked async always result in indirect faults.
There was a problem hiding this comment.
gotcha. can you add that comment?
There was a problem hiding this comment.
➡️ Expanded comment to cover this case
| // | Ran to completion | Not cancelled | Apply transform | | ||
| // | Ran to completion | Cancelled | Cancel result without applying transform | | ||
| // | Cancelled (matching token) | Cancelled | Cancel result without applying transform | | ||
| // | Cancelled (mismatch token) | Not cancelled | Cancel result without applying transform | |
There was a problem hiding this comment.
this one is the most interesting. woudl that indicate a bug?
There was a problem hiding this comment.
➡️ This is almost the same behavior you have for normal async/await. The only difference is this helper will produce a result task with the input cancellation token to the method instead of using the one included in the nested cancellation.
We could get an exact match, but it would require changing the implementation of UnwrapAndTransformAsync to use the asynchronous form of ContinueWith followed by Unwrap, which results in a performance penalty for the common case.
| // | Cancelled (mismatch token) | Not cancelled | Cancel result without applying transform | | ||
| // | Cancelled (mismatch token) | Cancelled | Cancel result without applying transform | | ||
| // | Direct fault¹ | Not cancelled | Fault result without applying transform | | ||
| // | Direct fault¹ | Cancelled | Cancel result without applying transform | |
There was a problem hiding this comment.
this seems fine. but it feels a bit lossy. is cancellation preferred here over bubbling the fault up?
There was a problem hiding this comment.
Passing through the fault is heavily preferred, but we can't do that without adding a performance cost to the common cases. See #55138 (comment).
There was a problem hiding this comment.
❗ This documentation is not correct. These are the results for indirect faults. I will fix.
There was a problem hiding this comment.
OK now the table is correct for these two. The unfortunate case is the final case (indirect fault combined with a cancellation request).
|
|
||
| gate.Set(); | ||
| var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => task.AsTask()); | ||
| Assert.Same(fault, exception); |
There was a problem hiding this comment.
i skimmed. but i assume you're testing the table below?
There was a problem hiding this comment.
It was my attempt to cover the full table, including synchronous and asynchronous forms of each possibility of intermediateResult.
|
This looks great to me. Also tagging @stephentoub for any thoughts he has, or ideas on patterns that maybe TPL coudl adopt to help out here. |
8a976c0 to
20decaa
Compare
|
still looks great to me. thanks for the corrections and clarifications. |
20decaa to
0d079d4
Compare
Addresses the following exceptions: