Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix resumability of iteration over IAsyncEnumerable<'T> #42

Merged
merged 27 commits into from
Oct 28, 2022

Conversation

abelbraaksma
Copy link
Member

@abelbraaksma abelbraaksma commented Oct 22, 2022

This is a continuation of #36. That PR merely added a bunch of tests and some small attempts at fixing this. These tests are deliberately failing as they showcase the issue at hand.

TODO:

  • Fix allowing multiple calls to GetAsyncEnumerator() of the taskSeq CE and using it as an iterator
  • When looping twice over an original taskSeq result must be equal, or, for side effects, show the side effects again
  • Fix calling MoveNextAsync of the taskSeq CE after it returns false: it should continue to return false
  • Fix calling Current before the first call to MoveNextAsync (the docs say its behavior is undefined, but similar to how IEnumerator<_> is implemented usually, just return Unchecked.defaultof).
  • Similarly to the previous point, allow calling Current after completion of a finite sequence (i.e. when MoveNextAsync has returned false).
  • Fix delayed (i.e., yielded) tasks. If result is not immediate, MoveNextAsync() called on a copied enumerator will return Faulted with an NRE

The above scenarios presently return an InvalidOperationException, as described in detail in issue #39.

@abelbraaksma abelbraaksma self-assigned this Oct 22, 2022
@abelbraaksma abelbraaksma added bug fix investigation Requires further or deeper investigation labels Oct 22, 2022
abelbraaksma added a commit that referenced this pull request Oct 24, 2022
abelbraaksma added a commit that referenced this pull request Oct 24, 2022
abelbraaksma added a commit that referenced this pull request Oct 24, 2022
@abelbraaksma
Copy link
Member Author

abelbraaksma commented Oct 24, 2022

Ok. So, here's what we're at now:

  • The exceptions raised when calling MoveNextAsync() too often, or Current too early or after-the-end are gone, which is a big win
  • Synchronous sequences appear to work just fine now. With synchronous I mean where there is no task yielding during processing, like taskSeq { yield 1; yield 2 }.
  • Asynchronous sequences (like taskSeq { do! Task.Delay 200 }) fail now in mysterious ways. Where previously there were early exceptions, now it appears to sometimes end in a deadlock, even with a (nested) empty sequence. This is somewhat worrying.

To emphasize the last point, I duplicated all tests we had so far, and made a synchronous and asynchronous batch. It is clear that the state of the ValueTask<bool> is incorrectly yielded and/or read again when already yielded before, leading, in most cases, to an NRE inside the ValueTaskSourceCore, and in some cases to deadlocks or other behavior leading to XUnit crashing.

@abelbraaksma abelbraaksma force-pushed the fix-resumability-of-iteration branch 2 times, most recently from 9b8a459 to cd8af87 Compare October 24, 2022 12:54
…is, partially fixed

Issue has to do with the MemberwiseClone() for shadowing the enumerator is not enough to reset the necessary states. Furthermore, after this is sorta fixed, the ValueTask that is used to keep the MoveNext boolean gets accessed twice asynchronously, which is not allowed. It also seems that using 'use' on the taskSeq.GetAsyncEnumerator fails by double disposing. This can probably be considered "by design" but should carefully be considered.
…h solves calling MoveNextAsync() multiple times after completion and removes the InvalidStateException
…cessed in lock step, or individually to completion
…w Enumerator, after a full loop through all items
@abelbraaksma
Copy link
Member Author

abelbraaksma commented Oct 28, 2022

Thanks and credits to @dsyme for helping me out today, and explaining more of, the resumable state machine logic. See #49 of that initial work to get resumption back to zero. This removes the hack that I used here, through Unchecked.defaultof<_>, that only partially worked.

There's still some cleaning up to do and apply suggested optimizations to the GetAsyncEnumerator() bits, but I'll do that in a follow-up PR, i.e. #51.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix investigation Requires further or deeper investigation
Projects
None yet
2 participants