dotnet / reactive

The Reactive Extensions for .NET
http://reactivex.io
MIT License
6.73k stars 751 forks source link

AsyncEnumerator.MoveNextAsync extension method considered harmful? #2175

Open Tragetaschen opened 1 month ago

Tragetaschen commented 1 month ago

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 commented 1 month ago

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