OperationCanceledException handling in ReadUntilCancelledAsync() and ReadAllAsEnumerablesAsync()

[mrak1990]

Now ReadUntilCancelledAsync() and ReadAllAsEnumerablesAsync() catches all OperationCanceledException exceptions, even if they were thrown by user code in receiver.

using var httpClient = new HttpClient();
httpClient.Timeout = TimeSpan.FromSeconds(5);
var url = "https://reqres.in/api/users?delay=10"; // responds with a delay of 10 seconds

async ValueTask LoadAndPrint()
{
    var json = await httpClient.GetStringAsync(url, ct); // throws TaskCanceledException due to timeout
    Console.WriteLine(json[..10]);
}

// must throw TaskCanceledException
_ = await Enumerable.Repeat(0, 10)
    .ToChannel(cancellationToken: ct)
    .ReadUntilCancelledAsync(ct, (_, _) => LoadAndPrint());

// must throw TaskCanceledException
await Enumerable.Repeat(0, 10)
    .ToChannel(cancellationToken: ct)
    .ReadAllAsEnumerablesAsync(_ => LoadAndPrint(), cancellationToken: ct);

[electricessence]

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.

[mrak1990]

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.

[electricessence]

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.

[electricessence]

Fixed: https://www.nuget.org/packages/Open.ChannelExtensions/8.4.0

https://github.com/Open-NET-Libraries/Open.ChannelExtensions/commit/018def45d8ab78adda0faa3af7b5fa1edf475381

[electricessence]

I will also update the minor versions for 6 and 7.

[electricessence]

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

[mrak1990]

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.

[mrak1990]

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)

[electricessence]

LGTM

The update is already in for the previous examples. I'll need some time to investigate the one you just posted.

[electricessence]

@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.

[electricessence]

Investigating now.

[electricessence]

As I understand it, the problem is how ContinueWith handles OperationCancelledException (t.Exception = null and t.Status = Cancelled here)

You are correct. This one is interesting.

[electricessence]

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

[electricessence]

I'll update the v6 and v7 versions soon.