Modify

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10674 closed defect (fixed)

ConcurrentModificationException when adding multiple marker

Reported by: yaghianth@… Owned by: The111
Priority: normal Milestone:
Component: JMapViewer Version: latest
Keywords: ConcurrentModificationException addMapMarker Cc:

Description (last modified by Don-vip)

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.

Attachments (0)

Change History (6)

comment:1 Changed 5 years ago by The111

Priority: majorminor
Type: defectenhancement

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.

comment:2 Changed 5 years ago by bastiK

Priority: minornormal
Type: enhancementdefect

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.

comment:3 Changed 5 years ago by Don-vip

Description: modified (diff)

comment:4 Changed 5 years ago by Don-vip

Summary: ConcurrentModificationException when addind multiple markerConcurrentModificationException when adding multiple marker

comment:5 Changed 5 years ago by The111

Resolution: fixed
Status: newclosed

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.

comment:6 Changed 5 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain The111.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.