Fix ConcurrentModificationException on removeAllMarkers()

[paxdei]

No description provided.

[aranega]

+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).

[ainslec]

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:

1. Change the removeAllMarkers() change to use an array:

            Integer[] copyOfMarkers = this.markers.keySet().toArray(new Integer[this.markers.size()]);
            for (Integer id : copyOfMarkers) {
  ....
                         }

I also prefer this type of approach for debugging rather than chaining operations together,

2. Change "getMarkers()" to return :

Collections.unmodifiableMap(this.markers)

[aranega]

Thanks for the clarification!

[paxdei]

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

[paxdei]

Found it and agreed

[ainslec]

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.

[paxdei]

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

[ainslec]

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.

[paxdei]

Then I will just wrap it in a new HashMap, breaks no code and solves the issue, with a little overhead nonetheless

[aranega]

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 getMarkers() could be used to add and remove markers which can already introduced errors (instead of using [add/remove]Marker() method).
Moreover, to take the pull into account, a new compilation is needed and the modifications is trivial.

EDIT> Woops, too late.
I would stand for the break as the final wrapping in the new HashMap will be equivalent to the initial solution (or I'm missing something).

[ainslec]

OK, I'm happy with Gottfried's (paxfei) new pull request. Are you also happy Vincent?

[aranega]

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).

[paxdei]

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

[ainslec]

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.