3

I've been working on a feature that queues time consuming work in a channel, and there I iterate the channel using e.g. await foreach(var item in channel.Reader.ReadAllAsync(cancellationToken)) {...}

I was expecting that when cancellation is requested through that cancellationToken, ReadAllAsync would throw on the first iteration that follows the cancellation.

As it seems to me, that is not the case. The loop continues until all items are processed, and then it throws an OperationCanceledException.

This looks a bit strange, to say the least. From ChannelReader's github repo one could see that the cancellation token is marked with the [EnumeratorCancellation] attribute, and so it should be passed to the state machine generated around yield return item; (please correct me if I'm wrong).

My question is, is this a (somewhat) normal behavior of ReadAllAsync(CancellationToken), or am I missing something?

Here is a simple test code that demonstrates the issue (try it on dotnetfiddle):

var channel = Channel.CreateUnbounded<int>();
for (int i = 1; i <= 10; i++) channel.Writer.TryWrite(i);
int itemsRead = 0;
var cts = new CancellationTokenSource();
try
{
    await foreach (var i in channel.Reader.ReadAllAsync(cts.Token))
    {
        Console.WriteLine($"Read item: {i}. Requested cancellation: " +
            $"{cts.Token.IsCancellationRequested}");

        if (++itemsRead > 4 && !cts.IsCancellationRequested)
        {
            Console.WriteLine("Cancelling...");
            cts.Cancel();
        }
    }
}
catch (OperationCanceledException)
{
    Console.WriteLine($"Operation cancelled. Items read: {itemsRead}");
}

Here is the output from the above. Note how item fetching continues after it should have been cancelled in the middle:

Read item: 1. Requested cancellation: False
Read item: 2. Requested cancellation: False
Read item: 3. Requested cancellation: False
Read item: 4. Requested cancellation: False
Read item: 5. Requested cancellation: False
Cancelling...
Read item: 6. Requested cancellation: True
Read item: 7. Requested cancellation: True
Read item: 8. Requested cancellation: True
Read item: 9. Requested cancellation: True
Read item: 10. Requested cancellation: True
Operation cancelled. Items read: 10
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
Peter Todorov
  • 51
  • 1
  • 5
  • I think the distinction here is that `ReadAllAsync` can be cancelled between subsequent calls to `WaitToReadAsync`, but it a call to `WaitToReadAsync` succeeds then multiple calls to `TryRead` can happen before the `CancellationToken` is next checked – canton7 May 17 '21 at 12:38
  • @canton7 this is how I see it is happening too, but it's still strange it works that way. Imagine having thousands of time consuming items in the channel, and code that counts on the iteration cancellation... – Peter Todorov May 17 '21 at 12:54
  • 2
    I guess it depends on what you think you're cancelling. You could see it as cancelling the actual asynchronous portion, but I agree the docs do say "*The cancellation token to use to cancel the enumeration.*" which implies that it cancels the actual enumeration. I can't find any discussion on what the token should cancel anywhere, so maybe it's worth an issue in dotnet/runtime to ask – canton7 May 17 '21 at 13:01
  • 1
    `Imagine having thousands of time consuming items in the channel` - it's not the items that are time consuming, but the processing of them. So perhaps the processor (`await foreach` loop body) should be checking the cancellation token. – Stephen Cleary May 17 '21 at 13:21
  • You are missing a break to exit the for loop. After cancelling you are continuing in the for loop. – jdweng May 17 '21 at 13:43
  • 2
    @StephenCleary yes, this is what I meant, the processing may be time consuming. And yes, eventually, checking the token myself and throwing is what I'll do. It still seems like a workaround to me, though. – Peter Todorov May 17 '21 at 14:04
  • 1
    @jdweng, wouldn't you expect ReadAllAsync to throw on the next call given the cancellation is triggered, thus breaking the loop? – Peter Todorov May 17 '21 at 14:06
  • 1
    I too would assume that the iterator loop breaks when the provided token is cancelled. Perhaps this warrants a bug report to the corefx team? – Alek May 17 '21 at 14:12
  • 1
    At the least, I'd raise a request for a doc clarification. I do think the docs contract the actual behaviour at the moment. It may be that the actual behaviour is intended, but the docs are insufficient – canton7 May 17 '21 at 14:17
  • 1
    I posted an issue on GitHub ([link](https://github.com/dotnet/runtime/issues/56820 "The ChannelReader.ReadAllAsync(CancellationToken) method does not react promptly to cancellation")), in order to get feedback from Microsoft. – Theodor Zoulias Aug 04 '21 at 05:42

2 Answers2

3

This behavior is by design. I am copy-pasting Stephen Toub's response from the related GitHub issue:

I would like to ask if this behavior is by design.

It is. There's already data available for reading immediately, so there's effectively nothing to cancel. The implementation of the iterator is just sitting in this tight loop:

while (TryRead(out T? item)) 
{ 
   yield return item; 
}

as long as data is available immediately. As soon as there isn't, it'll escape out to the outer loop, which will check for cancellation.

That said, it could be changed. I don't have a strong opinion on whether preferring cancellation is preferable; I expect it would depend on the use case.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • 1
    Thanks for raising this as an issue. I still assume this behavior questionable (apart form the performance considerations), as there might be scenarios of high-frequency data written to the channel, which is consumed in a way that the channel never remains empty (so that it can react to the cancellation). So a reading task employing ReadAllAsync should check the cancellation token on each reading as a workaround. I issued a request to specify this in the documentation [here](https://github.com/dotnet/dotnet-api-docs/issues/6751) – Peter Todorov Aug 05 '21 at 11:33
  • 1
    @PeterTodorov yeap, taking the current behavior for granted, one can't rely on the `ReadAllAsync` for cancelling the `await foreach` loop in a timely manner. They have to inject a `cancellationToken.ThrowIfCancellationRequested();` as the last line in the loop, for the cancellation to be respected promptly. On the other hand if one already calls a cancelable API inside the loop, for example `await _httpClient.GetStringAsync(url, cancellationToken)`, an extra check inside the `ReadAllAsync` in each iteration would look silly (redundant). – Theodor Zoulias Aug 05 '21 at 11:55
  • Ironically, I found about about this because I didn't have other cancellation means in my loop. As per a possible fix, I didn't go too deep in the code, but I guess also a registration of a cancellation callback might be beneficial. For example, raise some flag that instructs TryRead to return false, thus breaking the while loop and throwing. And yes, this will still require an extra check, unless TryRead does some checks anyways (I believe it should) – Peter Todorov Aug 06 '21 at 08:25
2

More of an update than actual answer, to whoever has had to research the same matter: this behavior is now specified in the documentation regarding ChannelReader.ReadAllAsync() here

The second sentence describing the cancellation token's effect ("If data is immediately ready for reading, then that data may be yielded even after cancellation has been requested.") has been added as a result of raising this subject.

Thanks to everyone involved!

Peter Todorov
  • 51
  • 1
  • 5