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

Enumerators incorrectly allow continued enumerator after disposal #76078

Open
jcouv opened this issue Nov 25, 2024 · 3 comments · May be fixed by #76090
Open

Enumerators incorrectly allow continued enumerator after disposal #76078

jcouv opened this issue Nov 25, 2024 · 3 comments · May be fixed by #76090
Assignees
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented Nov 25, 2024

The current implementation allows continuing the enumeration of an enumerator after its disposal:

// Prints: True42 disposed True43
// Expected: True42 disposed False42

var enumerator = C.GetEnumerator();

System.Console.Write(enumerator.MoveNext());
System.Console.Write(enumerator.Current);

enumerator.Dispose();
System.Console.Write(" disposed ");

System.Console.Write(enumerator.MoveNext());
System.Console.Write(enumerator.Current);

class C
{

    public static System.Collections.Generic.IEnumerator<int> GetEnumerator()
    {
        yield return 42;
        yield return 43;
    }
}

According to Spec for MoveNext and Dispose for enumerators, MoveNext should return false after disposal:

The precise action performed by MoveNext depends on the state of the enumerator object when MoveNext is invoked:
[...]
If the state of the enumerator object is after, invoking MoveNext returns false.

The Dispose method is used to clean up the iteration by bringing the enumerator object to the after state.

  • If the state of the enumerator object is before, invoking Dispose changes the state to after.
  • If the state of the enumerator object is running, the result of invoking Dispose is unspecified.
  • If the state of the enumerator object is suspended, invoking Dispose:
    • Changes the state to running.
    • Executes any finally blocks as if the last executed yield return statement were a yield break statement. If this causes an exception to be thrown and propagated out of the iterator body, the state of the enumerator object is set to after and the exception is propagated to the caller of the Dispose method.
    • Changes the state to after.
  • If the state of the enumerator object is after, invoking Dispose has no affect.
@jcouv jcouv self-assigned this Nov 25, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 25, 2024
@jcouv jcouv added this to the 17.13 milestone Nov 26, 2024
@jcouv jcouv added the Bug label Nov 26, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged Issues and PRs which have not yet been triaged by a lead label Nov 26, 2024
@jaredpar
Copy link
Member

Do we have a driving customer scenario for breaking behavior here?

@jcouv
Copy link
Member Author

jcouv commented Nov 26, 2024

@jaredpar This surfaced when the runtime repo tried to update their reference to roslyn. Since last week, we include logic to clear locals in the Dispose() method. This caused one or two runtime tests to crash due to NRE.

The options are:

  1. let customers encounter such regressions and fix their code
  2. revert the change to clear locals during disposal from last week
  3. enforce that we cannot re-enter user code after disposal (ie. the proposed fix)

@Sergio0694
Copy link
Contributor

Just curious, but could you explain this bit in the changelog?

"The state machine for enumerators incorrectly allowed resuming execution after the enumerator was disposed"

Why was that incorrect? The spec explicitly says doing so is UB, no? By eg. updating the logic to do manual cleanup from Dispose(), isn't that introducing overhead just to further allow code to take a dependency on an implementation detail for what is undefined behavior? 😅

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

Successfully merging a pull request may close this issue.

3 participants