Reactive: AsyncEnumerable ToAsyncEnumerable starts new thread

[clairernovotny]

The ToAsyncEnumerable method starts a new thread for converting an IEnumerable to async.

https://github.com/Reactive-Extensions/Rx.NET/blob/master/Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs#L24

Converting this to the AsyncEnumeratorAdapter, which is synchronous, causes one/some of the tests to deadlock, probably due to a Wait().

Question for @bartdesmet and @mattpodwysocki: should this avoid creating a new thread and we need to identify/fix the hanging test(s) or leave it as-is?

[bartdesmet]

This behavior is intentional to prevent blocking, which is the whole point of conversion to async. Originally we had a dependency on IScheduler to allow for configuration of concurrency, but we did away with this at some point.

[bartdesmet]

It could do it on one thread with signaling logic to the async MoveNext calls, provided the background thread does not run ahead of the requested enumeration calls (and with proper support for cancellation etc.). I'd suggest to defer to a subsequent release though given it may be non-trivial and error-prone.

[clairernovotny]

All for deferring this one, but I'll keep the issue open to track it.

[aelij]

I've added an AsAsyncEnumerable method (note As vs To) which does a same-thread conversion of an enumerable in my PR (#170) - see here.

Am I missing something? Seems pretty trivial to me. It is missing cancellation support but a call to ct.ThrowIfCancellationRequested() should do the trick.

[jnm2]

The thing that's really bugging me is that async is all about idiomatically managing callbacks and _not_ about threading per se. In my opinion, ToAsyncEnumerable shouldn't start running my enumerator on a background thread; it should just provide a facade with Task.FromResult and ignore the cancellation token. Even code that you await typically blocks for CPU unless there's something massively heavy being calculated.

I keep seeing new[] { 1, 2, 3, 4, 5 }.ToAsyncEnumerable() in the test code and knowing that each element access is wrapped in Task.Run just makes me cringe. That's way too granular to be queuing up work on the thread pool. Also asyncified sync is always a design smell. And what happens if the enumerator is on a non-threadsafe list, like... every mutable list that has an enumerator?

So I suppose what I'm saying is I'm highly skeptical of ToAsyncEnumerable in the first place as anything more than a facade. If there is a version that Task.Runs every single MoveNext, it's probably a design flaw on the user's part. If the user is unable to fix it and it's a common situation, or it's too hard to do manually in a less granular fashion, a ToThreadPoolAsyncEnumerable spells the danger more clearly. As it is, I would probably have used ToAsyncEnumerable without reading the docs, never suspecting that my enumerator was running cross-thread.

I'm still new to this scene so I could be way off base, but my suggestion: If you need to enumerate something where blocking takes too long, use IAsyncEnumerable all the way down just like you are supposed to use async all the way down. Otherwise, there still is a need for an interface adapter from IEnumerable to IAsyncEnumerable which adapts an interface where its MoveNext will block and will not cancel which is fine for collection enumerators or generators with light CPU, something like:

c# private sealed class AsyncEnumerableSampleImplementation<T> : IAsyncEnumerator { public Task<bool> MoveNext(CancellationToken cancellationToken) => Task.FromResult(enumerator.MoveNext()); }

[clairernovotny]

Fixed as part of #237