-
Notifications
You must be signed in to change notification settings - Fork 68
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 ConcurrentModificationException on removeAllMarkers() #25
Conversation
+1 This should definitely be merged. I introduced this bug, so shame on me (fixed on my fork, but totally forgot to create a pull request). |
First, please can "paxdei" (sorry I don't know your real name), can sign the contributor statement here : https://github.com/daveho/AceGWT/issues/18 Thank you. Secondly, at the risk of being pedantic, I don't think this would fix the issue completely. It is possible to be iterating over the keyset via getMarkers(), during which time, calling one of the marker removal methods, then there would still be a ConcurrentModificationException. Certainly this pattern of use is unlikely, but possible. Also, copying the keys of the map into a HashSet is a much heavier operation than copying them into an array. Especially in GWT. Therefore I would suggest:
I also prefer this type of approach for debugging rather than chaining operations together,
|
Thanks for the clarification! |
You are right, especially with exposing the naked markers map. I'll implement your input in my pull request. The link for the contributor statement goes to nowhere, I will check your wiki |
Found it and agreed |
Thanks, and thanks. Also, you may wish to enquire with the JSQLParser team about support GWT natively, without additional JRE emulated classes. GWT compatible compiler generation was added in JavaCC 6.1. |
Thanks for the tip! I will look into that! There's one problem with the change of getMarkers(): I would need to break the signature by returning Map instead of HashMap. Would be cleaner, but could break existing user code |
In this case, I personally feel think it is worth an API break. It won't be a silent failure, and silent breaks are the real danger. Also add the information that the returned map is immutable to the JavaDoc. There is a danger that other people were already using the mutable HashMap to remove markers, but if they did that, their code would have been buggy anyway, so by changing the contract, we are drawing attention to that bug. Which is a plus. Anyone using a newer version would have to be recompiling anyway and it would be a trivial downstream change. In your new pull request description, please highlight the breaking change. |
Then I will just wrap it in a new HashMap, breaks no code and solves the issue, with a little overhead nonetheless |
The break seems really needed here. I went for "HashSet" in the signature as I read that the finer the better for GWT (avoiding a lot of checks). But here, the EDIT> Woops, too late. |
OK, I'm happy with Gottfried's (paxfei) new pull request. Are you also happy Vincent? |
Hey, you're the boss ;) (but yep, sounds good to me). Thanks for all the details, it was very interesting (I now have to go make change in my GWT projects). |
Fix ConcurrentModificationException on removeAllMarkers() Breaking change on getMarkers() -- now returns immutable java.util.Map rather than mutable java.util.HashMap Change was required to remove possibility of concurrent modification without overhead of copying the map every time it was accessed.
Good to see everybody happy. I personally find Map much cleaner as return type. Narrowing types down is only more efficient for rpc as far as I'm aware |
Map is the correct return type too I think here. It affords a lot more flexibility on the implementation without an API break. Thanks for your effort. |
No description provided.