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
416 stars 25 forks source link

Test involving a BatchingChannelReader with 'WithTimeout' integration keeps failing #23

Closed f0w9hef09sd0f9hs0d9fh closed 2 years ago

f0w9hef09sd0f9hs0d9fh commented 2 years ago

I do not understand why this test involving a Channel Reader keeps reaching case 1: in the switch case and still has two elements in the batch. Any ideas ? Every time I reach batch.Should().HaveCount(1), the batch still has 2 items left. Same result whether I do Run Test or Debug Test so I am assuming it's not a timing issue ? I don't understand this (intern so quite new to all of these libraries).

Thanks.

[Fact]
public async Task BatchTimeoutIsReached()
{
    var c = Channel.CreateUnbounded<int>(new UnboundedChannelOptions { SingleReader = false, SingleWriter = false });
    _ = Task.Run(async () =>
    {
        c.Writer.TryWrite(1);
        c.Writer.TryWrite(2);
        await Task.Delay(200);
        c.Writer.TryWrite(3);
        await Task.Delay(200);
        c.Writer.TryWrite(4);
        c.Writer.TryWrite(5);
        c.Writer.TryWrite(6);
    });

    using var tokenSource = new CancellationTokenSource();
    BatchingChannelReader<int, System.Collections.Generic.List<int>> batchReaderWithTimeout = c.Reader.Batch(2).WithTimeout(TimeSpan.FromMilliseconds(100));
    await batchReaderWithTimeout.ReadAllAsync(async (batch, i) =>
        {
            switch (i)
            {
                case 0:
                    batch.Should().HaveCount(2);
                    Assert.Equal(1, batch[0]);
                    Assert.Equal(2, batch[1]);
                    break;
                case 1:
                    batch.Should().HaveCount(1);
                    Assert.Equal(3, batch[0]);
                    break;
                case 2:
                    batch.Should().HaveCount(2);
                    Assert.Equal(4, batch[0]);
                    Assert.Equal(5, batch[1]);
                    break;
                case 3:
                    batch.Should().HaveCount(1);
                    Assert.Equal(6, batch[0]);
                    c.Writer.Complete();
                    break;
                default:
                    throw new Exception("Shouldn't arrive here.");
            }

            await Task.Delay(500);
        });
}

image

electricessence commented 2 years ago

I'll take a look!

electricessence commented 2 years ago

CancellationTokenSource isn't used. And you've never called .Complete() so it will never finish.

You set the batch size to 2, so as soon as 1 and 2 are added, that batch is available and the following timeout effectively does nothing.

By adding the delay at the end (500), you throw everything off.

If you added logging, you could easily see:

And because the nature of awaiting and threading, you might have some ms deviation and it might get past batch 1.

I think you're over thinking it. It works as expected. What are you trying to accomplish?

electricessence commented 2 years ago

Closing for now, but happy to help you figure anything out.

f0w9hef09sd0f9hs0d9fh commented 2 years ago

We had an extension on the UntilTimeout method that needed to be updated since your library's method signature was changed. Thanks.

electricessence commented 2 years ago

Yes. Some things due to suggestions have changed. The timeout feature took a long time to get right. Also note that you can get batches as queues now as they have some advantages when dealing with ownership and fail-over. I prefer queues because you can 'peek' and attempt to do something with an item, and then if the operation fails, you can pass the remaining items to a fail-over routine and still assume that any previous items removed from the queue were successful.