Open-NET-Libraries / Open.ChannelExtensions

A set of extensions for optimizing/simplifying System.Threading.Channels usage.
https://open-net-libraries.github.io/Open.ChannelExtensions/api/Open.ChannelExtensions.Extensions.html#methods
MIT License
401 stars 25 forks source link

Channels created by `Source` or `ToChannel` do not reach completion when canceled. #22

Closed alitas closed 1 year ago

alitas commented 1 year ago

In the following code, reader.Completion task is never completed, resulting in an infinite await. I would expect the completion task to be canceled. Looking at the source code for Channels, calling TryComplete without reading the remaining items off the queue will not complete this task.

    var cts = new CancellationTokenSource();
    var reader = Enumerable.Range(0, 10_000).ToChannel(10, true, cts.Token);

    try
    {
        cts.Cancel();
        await reader.ReadAll(_ => {}, cts.Token);
    }
    catch (Exception)
    {
        // Catches the OperationCanceledException
    }
    finally
    {
        // Will await forever here
        await reader.Completion;
    }
electricessence commented 1 year ago

Note, there's a flaw in your code. You're using the same cancellation token for both, and there is a risk that once the token is cancelled there will still be items in the channel and it will just wait for the final items to be read.

That said, if you call cancel before you have even awaited (as you have done here) there is flaw in the WriteAllAsync method here: https://github.com/Open-NET-Libraries/Open.ChannelExtensions/blob/ab3697a32de041c03e48e00d82b93fa6e3e63ed3/Open.ChannelExtensions/Extensions.Write.cs#L37

Calling cancel before it starts writing will throw the OperationCancelledException but won't call complete.

I will use your sample code as a test and fix the issue.

electricessence commented 1 year ago

And in this case, .ToChannel without deferredExecution=true will begin writing to the channel immediately, therefore generating (in your sample) 10,000 items (if possible) before yielding to the next line. So your cancel causes the .Read operation to bail immediately but there are still X number of items in the channel. There's no way it will complete. It has to be drained first.

electricessence commented 1 year ago

So I fixed the issue where if you called cancel before calling .ToChannel() that it wouldn't complete. Here are the tests based upon what you wrote: https://github.com/Open-NET-Libraries/Open.ChannelExtensions/blob/master/Open.ChannelExtensions.Tests/SourceTests.cs

Again, note that you will still have to call .ReadAll() with your example (with out the same cancellation token) for it to complete.

electricessence commented 1 year ago

Thank you so much for bringing my attention here. This is the updated version: https://www.nuget.org/packages/Open.ChannelExtensions/6.2.2