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

Different/Incorrect Behaviour in .NET Framework #17

Closed md-ae closed 2 years ago

md-ae commented 2 years ago

The below example works "as expected" on netcoreapp2.1, net5.0 etc but hangs on .NET Framework (tested net472, net48).

"as expected" means items are streamed through the pipeline as soon as items are available i.e. StartProcessingTask2(...) returns immediately and the int values from queue go through ReadAll(IncrementCount2) as soon as the first int is available and before all ints have been added to queue.

When your nuget package is referenced by .NET Framework projects, StartProcessingTask2(...) does not return because queue.CompleteAdding() has not been called, your library is waiting for the IEnumerable given to .Source(source, true) to complete. If it's body is wrapped in a Task so that StartProcessingTask2(...) may return and subsequently queue can be populated, IncrementCount2(int c) is not called until all items from queue have been read completely into some intermediate buffer, only then will ReadAll(IncrementCount2) execute IncrementCount2.

When I cloned your code and referenced it as a project, it worked "as expected". Therefore I believe there is a bug/incompatibility with how msbuild builds this library for netstandard2.0, netstandard2.1, those dlls do not function correctly with .NET Framework.

I suggest/ask that you include .NET Framework specific dlls in the nuget package e.g. add dlls built specifically for 4.6.2, 4.7, 4.7.1, 4.7.2, 4.8.

Great ideas executed in this library, thank you!

Open.ChannelExtensions.Tests.zip

using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Channels;
using System.Threading.Tasks;
using NUnit.Framework;

namespace Open.ChannelExtensions.Tests
{
        [Test]
        public void Example()
        {
            var queue = new BlockingCollection<int>();
            var processingTask = StartProcessingTask2(queue.GetConsumingEnumerable());

            for (var i = 0; i < 100000000; i++)
                queue.Add(i);

            queue.CompleteAdding();

            processingTask.Wait();
        }

        private int count_;

        private Task StartProcessingTask2(IEnumerable<int> source)
        {
            return Channel.CreateUnbounded<int>()
                .Source(source, true)
                .ReadAll(IncrementCount2)
                .AsTask();
        }

        private void IncrementCount2(int c)
        {
            Interlocked.Increment(ref count_);
        }
    }
}
electricessence commented 2 years ago

I will take a look at this ASAP! Thank you for doing this!

electricessence commented 2 years ago

First off: I would consider that .NET Framework may be missing the proper requirements to handle ValueTasks and therefore may behave different (sub-optimal) but not in a way that is breaking. That said, if it's "hanging" and it's unbounded, there's a definite concern here.

electricessence commented 2 years ago

@md-ae, I've implemented your test here: https://github.com/Open-NET-Libraries/Open.ChannelExtensions/blob/master/Open.ChannelExtensions.Tests/BasicTests.cs#L334-L360

I've enabled targeting for net472.

electricessence commented 2 years ago

I've been unable to get this to "hang" in debug. It appears to behave as expected. Regardless of if you have deferred execution: The read loop is setup and waits for the consuming enumerable to release results. There is no guarantee that multiple threads will be running as the BlockingCollection can fill completely before a thread is offered to the read loop. I have seen what appears to be a hang in release mode and it doesn't require so many ints. Continuing to investigate.

electricessence commented 2 years ago

Please try this branch as I am unable to replicate. https://github.com/Open-NET-Libraries/Open.ChannelExtensions/blob/net472-test/TestPossibleLoadingIssueInNET472/Program.cs In some cases it may simply feel like it takes a long time. The other thing is that it needs to be real-world and not behind xUnit or NUnit to be sure the TestExplorer isn't doing something funny.

md-ae commented 2 years ago

Thank you for responding with the detail!

Running as a console app, it works "as expected" in .NET Framework, it is only when running through NUnit that the IEnumerable<int> source from GetConsumingEnumerable() must finish before ReadAll() starts executing. I tested versions 5.1.3 and 5.2.0. I also tested xUnit, it worked "as expected" via xUnit.

Summary of Testing

.NET Framework .NET Core Onwards
NUnit No Yes
Console Yes Yes
xUnit Yes Yes

It looks like NUnit (or NUnit+ReSharper?+VS2019?) is not functioning correctly with this code referenced via the nuget package dlls. It's not something I would have expected, sorry about that.

  1. Would you have any idea how NUnit might run this code differently when it's loaded via a pre-built DLL but correctly when referenced as a project?
  2. If you were to guess, what might NUnit be doing? For example, could it be affecting the way await Task.Yield(); functions for deferredExecution?

Any help is appreciated, I'll have a root around to see if I can see any smoking guns. With the MIT license I can reference the code as a project but it would be easier managed and preferable as a Nuget reference to the official source.

Thank you again.

md-ae commented 2 years ago

A little bit more investigation and as you mentioned, a Release mode build causes this issue.

Summary of Testing

.NET Framework (Debug) .NET Framework (Release) .NET Core Onwards
NUnit Yes No Yes
Console Yes Yes Yes
xUnit Yes Yes Yes
md-ae commented 2 years ago

I've opened a bug report with NUnit.

electricessence commented 2 years ago

Yeah. This is one of those tricky ones that you kinda have to 'monitor' through logging to be sure what's going on since debugging seems to work every time. Good luck with that one!