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

AsyncEnumerator.MoveNextAsync extension method considered harmful? #2175

Open
Tragetaschen opened this issue Oct 28, 2024 · 1 comment
Open

Comments

@Tragetaschen
Copy link

Problem

We have run at least twice into issues by accidentally using the AsyncEnumerator.MoveNextAsync extension method and I have trouble finding a scenario where it would be applicable.

Which subcomponent library (Ix, Async.Ix)?

Async.Ix

Which library version?

6.0.1

What is the problem?

  1. The extension method is very easy to discover and use, it lives in a namespace that is part of the default global usings nowadays.
  2. It speaks to a correctness argument: "Yes, of course I want cancellation when I call that async method".
  3. During code review everything looks perfectly fine.
  4. It doesn't do what you can reasonably expect. The cancellation token is only synchronously checked when entering the method, but never used along the async chain of the underlying async enumerator.

We have run into issues where the async enumerator was created without cancellation token and the expectation was that calling MoveNextAsync would cancel. However, the resulting code would only cancel when the async enumerator made forward progress and re-entered the next MoveNextAsync call. That didn't happen and we were left with a deadlock or memory leak.

What is the expected outcome?

I would vote for removing / deprecating this extension method, but I also realize that this is a breaking change.

I have put in the global namespace another overload of that extension method with the same signature and ObsoleteAttribute, so it is always preferred by the compiler and disables my own usage.

@julealgon
Copy link

Why does this method exist in the first place, considering IAsyncEnumerable now exists as a first-class citizen in the framework?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants