Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix SelectArrayIterator.MoveNext after Dispose#12523

Merged
stephentoub merged 5 commits intodotnet:masterfrom
jamesqo:patch-3
Oct 12, 2016
Merged

Fix SelectArrayIterator.MoveNext after Dispose#12523
stephentoub merged 5 commits intodotnet:masterfrom
jamesqo:patch-3

Conversation

@jamesqo
Copy link
Copy Markdown
Contributor

@jamesqo jamesqo commented Oct 10, 2016

Just realized that there was a small issue with my changes in #11841: if someone tries to MoveNext on a SelectArrayIterator after it's Disposed, then an IOOR exception will be generated because we try to access index -2. This is the fix.

Repro:

var s = new int[1].Select(a => a);
using (var e = s.GetEnumerator())
{
    e.MoveNext();
    e.MoveNext(); // Disposes
    e.MoveNext(); // before called SelectArrayIterator.Dispose again, now throws
}

cc @stephentoub, @VSadov

@stephentoub
Copy link
Copy Markdown
Member

Thanks. Can you please add a test?

@jamesqo
Copy link
Copy Markdown
Contributor Author

jamesqo commented Oct 12, 2016

@stephentoub All done. I also made the fix a bit shorter.

Comment thread src/System.Linq/tests/SelectTests.cs Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert.False(e.MoveNext());

Comment thread src/System.Linq/tests/SelectTests.cs Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a TheoryData rather than a normal MemberData?

Copy link
Copy Markdown
Contributor Author

@jamesqo jamesqo Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like TheoryData a bit better because it guarantees type/arity safety, e.g. you can't pass in the wrong type or the wrong number of arguments to it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. But we have close to zero use of it across the repo, whereas InlineData and MemberData are used pervasively. I would rather we not diverge from that except where we have very good reasons for doing so. Different people can prefer different things.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@stephentoub
Copy link
Copy Markdown
Member

Thanks, @jamesqo.

@stephentoub stephentoub merged commit afd1df7 into dotnet:master Oct 12, 2016
@jamesqo jamesqo deleted the patch-3 branch October 12, 2016 18:53
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants