Fixed invocation cancellation to correctly cancel the payload read #1640
Conversation
| Exception? completeException = null; | ||
| using var invocationCancelSource = CancellationTokenSource.CreateLinkedTokenSource( |
There was a problem hiding this comment.
Should we use "cts" like Microsoft frequently does in runtime? Could be invocationCts, _dispatchesAndInvocationsCts.
I grepped runtime for CancelSource and there is not a single match.
There was a problem hiding this comment.
Yes, let's do this in another PR to fix all our CTS names.
| @@ -301,20 +298,12 @@ private protected override async ValueTask DisposeAsyncCore() | |||
| } | |||
| finally | |||
| { | |||
| if (completeException is not null) | |||
| if (completeException is not null && stream is not null) | |||
There was a problem hiding this comment.
I find this catch/catch/finally code weird, especially since the only purpose of the completeException variable is to feed it to the finally block.
Maybe we could instead use a helper local function void CompleteStreamAsync(Exception exception)?
There was a problem hiding this comment.
We use this optional exception catch/finally pattern is multiple places. I don't see why we would use a helper function here just because the finally block doesn't perform other cleanup.
To me, either we change all the code using this pattern to use a helper function for exception related cleanup or we stick to this pattern even in this case.
| }); | ||
| } | ||
|
|
||
| [Ignore("see issue #1638")] |
There was a problem hiding this comment.
The test is incorrect. If you want to keep reading the incoming request's payload after returning a response, you need to "detach" (swap) this payload.
There was a problem hiding this comment.
I'll fix this in another PR.
This PR fixes #1610 and adds tests for invocation cancellation.