Closed mrak1990 closed 2 months ago
I don't think this is going to change, because it's intentional to "just bail" if there's a cancellation token that has been cancelled for these specific methods.
The token itself is effectively "owned/borrowed" by the caller, and instead of a try catch, you simply call token.ThrowIfCancellationRequested()
.
Take a look at the differences between the other various methods. It's intentional. There are plenty of cases where it absolutely will throw when the token is cancelled because that's expected for something as simple as .ReadAllAsync()
or the like.
And again, for .ReadAllAsEnumerables[Async]
: "cancellelation" is a 'signal' to bail out. And then the caller can or should own what they want to do with it.
The token itself is effectively "owned/borrowed" by the caller, and instead of a try catch, you simply call
token.ThrowIfCancellationRequested()
.
I'm not sure I got the idea right.
In the example above, the exception TaskCanceledException
(inherited from OperationCanceledException
) is thrown inside the HttpClient.GetStringAsync()
method due to a timeout. At the same time, the ct token is not cancelled.
As a result, ReadAllAsEnumerablesAsync()
is executed without exception (although not the whole pipeline is worked out) and ct.ThrowIfCancellationRequested();
will not throw an exception.
I think I see the nuanced edge case here... You are suggesting that if the delegate itself throws a cancellation exception, but the CancellationToken is not cancelled, then it should not trap the exception.
I will also update the minor versions for 6 and 7.
Another few changes to make things a bit more correct: https://www.nuget.org/packages/Open.ChannelExtensions/8.4.1
If you'd be so kind as to review these changes too: https://github.com/Open-NET-Libraries/Open.ChannelExtensions/commit/b1f4df088aa22f030b65471baf6f2006477f9e66
LGTM
As I understand, the changes to ReadUntilCancelledAsync
/ReadAllAsEnumerablesAsync
(WaitToReadOrCancelAsync -> WaitToReadAsync
) change the behaviour on explicit cancellation. Now these methods throw OperationCancelledException
if the token passed above in chain is canceled.
This makes sense, now the behavior is similar to the methods from BCL.
I found another example of incorrect behavior when throwing OperationCancelledException
.
ValueTask<int> AsyncReceiver()
{
throw new OperationCanceledException("test exception");
}
// OK, throw exception
_ = await Enumerable.Repeat(0, 10)
.ToChannel()
.ReadAllAsync(async _ => await AsyncReceiver());
// NOT OK, doesn't throw exception
_ = await Enumerable.Repeat(0, 10)
.ToChannel()
.PipeAsync(1, async _ => await AsyncReceiver())
.ReadAll(_ => { });
As I understand it, the problem is how ContinueWith
handles OperationCancelledException
(t.Exception = null
and t.Status = Cancelled
here)
LGTM
The update is already in for the previous examples. I'll need some time to investigate the one you just posted.
@mrak1990 I strongly suggest in the future, if you have a fork and want to merge a PR, start with adding a test to the test project that proves the current behavior is incorrect. Then I can pull that down and decide what to do and ultimately merge the fix.
Investigating now.
As I understand it, the problem is how
ContinueWith
handlesOperationCancelledException
(t.Exception = null
andt.Status = Cancelled
here)
You are correct. This one is interesting.
Fixed and released to 8.4.2. Great catch. Thank you.
Added special and simple handling that if the task is cancelled, but not by the token, then be sure to tell the channel why by passing it an OperationCancelledException. https://github.com/Open-NET-Libraries/Open.ChannelExtensions/blob/master/Open.ChannelExtensions/Extensions.Pipe.cs#L11
I'll update the v6 and v7 versions soon.
Now ReadUntilCancelledAsync() and ReadAllAsEnumerablesAsync() catches all OperationCanceledException exceptions, even if they were thrown by user code in receiver.