ConcurrentModificationException when adding multiple marker

[yaghianth@…]

I am using a for loop to add markers about hotels to my map. But I am getting a ConcurrentModificationException when addind multiple marker.
this is the stack trace:

Exception in thread "AWT-EventQueue-0" java.util.ConcurrentModificationException
	at java.util.LinkedList$ListItr.checkForComodification(Unknown Source)
	at java.util.LinkedList$ListItr.next(Unknown Source)
	at org.openstreetmap.gui.jmapviewer.JMapViewer.paintComponent(JMapViewer.java:629)
	at javax.swing.JComponent.paint(Unknown Source)
	at javax.swing.JComponent.paintToOffscreen(Unknown Source)
	at javax.swing.BufferStrategyPaintManager.paint(Unknown Source)
	at javax.swing.RepaintManager.paint(Unknown Source)
	at javax.swing.JComponent._paintImmediately(Unknown Source)
	at javax.swing.JComponent.paintImmediately(Unknown Source)
	at javax.swing.RepaintManager$4.run(Unknown Source)
	at javax.swing.RepaintManager$4.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(Unknown Source)
	at javax.swing.RepaintManager.paintDirtyRegions(Unknown Source)
	at javax.swing.RepaintManager.paintDirtyRegions(Unknown Source)
	at javax.swing.RepaintManager.prePaintDirtyRegions(Unknown Source)
	at javax.swing.RepaintManager.access$1300(Unknown Source)
	at javax.swing.RepaintManager$ProcessingRunnable.run(Unknown Source)
	at java.awt.event.InvocationEvent.dispatch(Unknown Source)
	at java.awt.EventQueue.dispatchEventImpl(Unknown Source)
	at java.awt.EventQueue.access$400(Unknown Source)
	at java.awt.EventQueue$3.run(Unknown Source)
	at java.awt.EventQueue$3.run(Unknown Source)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(Unknown Source)
	at java.awt.EventQueue.dispatchEvent(Unknown Source)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForFilter(Unknown Source)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.pumpEvents(Unknown Source)
	at java.awt.EventDispatchThread.run(Unknown Source)

Here is the code :

for(int i=0; i < len; i++){
                        hotel = hotelsList.getJSONObject(i);
                        lat = hotel.getDouble("lat");
                        lng = hotel.getDouble("lng");
                        name = hotel.getString("name");
                        JMAP.addHotel(lat, lng, name);
                        }

and this is the addHotel method in JMAP class:

public static void addHotel(double lat, double lng, String name){
map.addMapMarker(
               new MapMarkerDot(name, new Coordinate(lat,lng)));
            }

JMAP is a class created from the DEMO class you offer.

[The111]

There is no issue with standard JMapViewer and adding multiple map markers by the intended procedure. The demo already has many markers on it! And it is very easy to add a few more. Adding these lines to Demo.java:

    map().addMapMarker(new MapMarkerDot("dot1", new Coordinate(1, 1)));
    map().addMapMarker(new MapMarkerDot("dot2", new Coordinate(2, 2)));
    map().addMapMarker(new MapMarkerDot("dot3", new Coordinate(3, 3)));
    map().addMapMarker(new MapMarkerDot("dot4", new Coordinate(4, 4)));

We can see them on the panel at runtime easily.

​http://i.imgur.com/zYulHJ6.png

Somehow you are attempting to concurrently modify the backing list. Admittedly this should probably be prevented through public accessible methods. Please post a snapshot of your current codebase, otherwise it is impossible to say how you're doing this.

[bastiK]

The problem is in the JMapViewer code:

mapMarkerList is looped over in JMapViewer.paintComponent, which runs in EDT. Now if JMapViewer.addMapMarker modifies this list from another thread while the painting operation is in progress, you get ConcurrentModificationException.

There are several ways to solve this, probably the simplest is:

mapMarkerList = Collections.synchronizedList(new LinkedList<>());

Then you still need to synchronize non-atomic operations like list iteration:

synchronized(mapMarkerList) {
   for (MapMarker marker : mapMarkerList) {
       if(marker.isVisible())paintMarker(g, marker);
   }
}

Also I would make a copy and return a copy in setMapMarkerList and getMapMarkerList.

[The111]

Thanks for the tips bastiK. I was able to reproduce the ConcurrentModificationException on my machine.

[o30769] implements usage of synchronized collections and synchronized non-atomic accesses per your suggestions. Regarding your other suggestion on defensive copying, I did not add that just yet. My first instinct would be that public accessors for these lists should not even exist, rather we should just offer add/delete functionality only. But since they are part of the public API already, perhaps they are being used already and we don't want to break any clients? If we do leave them in place and want to add defensive copying, the best way I know to do it with a collection is using Guava's ImmutableList.copyOf(), but I do not know if this community wants to introduce Guava deps into this library. Please feel free to discuss further.

[bastiK]

No further dependency without good reason, unless it is optional and can be excluded for JOSM.

I think it is fine the way it is now and the ticket can be closed.

Just if someone were to use getMapMarkerList, this error will probably pop up again if they loop over the list without synchronization. A simple

synchronized(mapMarkerList){
    return new ArrayList<>(mapMarkerList);
}

should do.