AsyncEnumerable ToAsyncEnumerable starts new thread #202

Activity

1. clairernovotny

   added

   [area] Ix

   on Jun 22, 2016
2. clairernovotny

   added this to the 3.0.1This milestone has been deleted milestone on Jun 22, 2016
3. bartdesmet commented on Jun 22, 2016

   

   bartdesmet

   on Jun 22, 2016

   Collaborator

   More actions

   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.
4. bartdesmet commented on Jun 22, 2016

   

   bartdesmet

   on Jun 22, 2016

   Collaborator

   More actions

   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.
5. clairernovotny commented on Jun 22, 2016

   

   clairernovotny

   on Jun 22, 2016

   ContributorAuthor

   More actions

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

   modified the milestones: Future, 3.0.1This milestone has been deleted on Jun 24, 2016
7. aelij commented on Jul 4, 2016

   

   aelij

   on Jul 4, 2016

   Contributor

   More actions

   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.
8. jnm2 commented on Jul 19, 2016

   

   jnm2

   on Jul 19, 2016

   Last edited by jnm2

   Contributor

   More actions

   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:

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

   👍React with 👍3Reacted by Claire Novotny, quetzalcoatl and Arsène von Wyss
9. clairernovotny commented on Sep 17, 2016

   

   clairernovotny

   on Sep 17, 2016

   ContributorAuthor

   More actions

   Fixed as part of #237
10. clairernovotny

    closed this as completedon Sep 17, 2016
11. clairernovotny

    modified the milestones: 3.1, Future on Sep 17, 2016