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 RepeatedFieldTest.Enumerator test #603

Conversation

jtattermusch
Copy link
Contributor

on my Windows machine RepeatedFieldTest.Enumerator is failing with error:

Test FullName:  Google.Protobuf.Collections.RepeatedFieldTest.Enumerator
Test Source:    c:\Users\jtattermusch\github\protobuf\csharp\src\ProtocolBuffers.Test\Collections\RepeatedFieldTest.cs : line 240
Result Message: System.InvalidOperationException : Operation is not valid due to the current state of the object.
Result StackTrace:  
at Google.Protobuf.Collections.RepeatedField`1.Enumerator.get_Current() in c:\Users\jtattermusch\github\protobuf\csharp\src\ProtocolBuffers\Collections\RepeatedField.cs:line 506
at Google.Protobuf.Collections.RepeatedFieldTest.Enumerator() in c:\Users\jtattermusch\github\protobuf\csharp\src\ProtocolBuffers.Test\Collections\RepeatedFieldTest.cs:line 246

it looks like all changes to the enumerator variable in that test are immediately forgotten.
I eventually found the cause, which seems to be:
http://stackoverflow.com/questions/4642665/why-does-capturing-a-mutable-struct-variable-inside-a-closure-within-a-using-sta
and this bug report
https://connect.microsoft.com/VisualStudio/feedback/details/635745

Just removing the using block fixes the problem (as well are removing the lambda would), but it makes me wondering if we actually want RepeatedField.Enumerator to be a struct. Exposing enumerator struct makes as susceptible to subtle issues like these:
https://www.simple-talk.com/blogs/2010/05/19/why-enumerator-structs-are-a-really-bad-idea/
https://www.simple-talk.com/blogs/2010/05/20/why-enumerator-structs-are-a-really-bad-idea-redux/

(I think with Reset() method we are actually fine, because we don't implement IEnumerator.Reset() explicitly, unlike System.Collections.Generic.List, but I am wondering how much of a performance gain the struct-based enumerator actually is.

Switching the enumerator to private class might save us some headache in the future (MapField already has an enumerator class).

@jskeet
Copy link
Contributor

jskeet commented Jul 16, 2015

The struct-based enumerator can provide a significant win, particularly if the list is small or empty.

Now I'm not sure we're using foreach over the list anywhere any more - we can validate that.

@jtattermusch
Copy link
Contributor Author

To fix the test, we basically have these options
-- remove the using statement
-- stop using the lambda in the test
-- change the iterator type to class

Which one do you prefer?

@jskeet
Copy link
Contributor

jskeet commented Jul 16, 2015

Will see if changing to use try/finally explicitly fixes it instead, which I think I'd prefer. But will investigate the current benefits in the runtime, too.

@jskeet
Copy link
Contributor

jskeet commented Jul 16, 2015

Replaced with #610

@jskeet jskeet closed this Jul 16, 2015
taoso pushed a commit to taoso/protobuf that referenced this pull request Aug 1, 2018
The new table-drive unmarshaler never emits "{Unknown}",
so that comment is stale.

Also, there is no reason to document that the name of "first" missing
required field is stored:
* There is no programmatic way to access this field, so it is just for user debugging.
* "First" is not well defined. First encounted while parsing? First listed in proto file? First listed in the Go struct?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants