Modify

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#12030 closed defect (fixed)

[Patch] ConcurrentModificationException in findShortcut

Reported by: wiktorn Owned by: team
Priority: normal Milestone: 16.10
Component: Core Version: latest
Keywords: Cc:

Description

I've got following exception during startup. I guess, that we need to change HashMap to ConcurrentHashMap here, as we have parallel plugin load. Version 8964.

Caused by: java.util.ConcurrentModificationException
	at java.util.LinkedHashMap$LinkedHashIterator.nextNode(Unknown Source)
	at java.util.LinkedHashMap$LinkedValueIterator.next(Unknown Source)
	at org.openstreetmap.josm.tools.Shortcut.findShortcut(Shortcut.java:266)
	at org.openstreetmap.josm.tools.Shortcut.registerShortcut(Shortcut.java:443)
	at org.openstreetmap.josm.tools.Shortcut.registerShortcut(Shortcut.java:430)
	at org.openstreetmap.josm.plugins.mapillary.actions.MapillaryDownloadAction.<init>(MapillaryDownloadAction.java:32)
	at org.openstreetmap.josm.plugins.mapillary.MapillaryPlugin.<init>(MapillaryPlugin.java:110)

Attachments (2)

testParallelAccess.patch (4.8 KB ) - added by simon04 9 years ago.
12030-cowal.patch (9.2 KB ) - added by simon04 8 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 by Don-vip, 9 years ago

Component: PluginCore
Milestone: 15.10

comment:2 by Don-vip, 9 years ago

Milestone: 15.1015.11

not so easy!
We currently use LinkedHashMap, we must check if it's for its ordering (results are returned in the same order they were inserted).

ConcurrentHashMap results are unsorted. Moreover they treat null differently, we must ensure that null is never used as a key or value if we want to switch.

So no rushing, let's think about this in next release.

comment:3 by Don-vip, 9 years ago

Collections.synchronizedMap(new LinkedHashMap(...)); might also be an option to consider.

comment:5 by Don-vip, 9 years ago

Milestone: 15.1115.12

comment:6 by Don-vip, 9 years ago

Milestone: 15.1216.01

by simon04, 9 years ago

Attachment: testParallelAccess.patch added

comment:7 by simon04, 9 years ago

I tried to initialize all actions from org.openstreetmap.josm.actions in parallel (using Executors.newFixedThreadPool()) with attachment:testParallelAccess.patch​. However, I could not produce a ConcurrentModificationException.

comment:8 by wiktorn, 9 years ago

Resolution: irreproducible
Status: newclosed

I tried to play with your tests some more and I've failed to reproduce, so I close this ticket. Thanks.

comment:9 by Don-vip, 9 years ago

Milestone: 16.01

comment:10 by michael2402, 8 years ago

Milestone: 16.09
Resolution: irreproducible
Status: closedreopened

I triggered it again:

Sep 17, 2016 4:10:43 PM org.openstreetmap.josm.Main error
SEVERE: org.openstreetmap.josm.plugins.PluginException: An error occurred in plugin fieldpapers. Cause: java.lang.reflect.InvocationTargetException. Cause: java.util.ConcurrentModificationException
org.openstreetmap.josm.plugins.PluginException: An error occurred in plugin fieldpapers
	at org.openstreetmap.josm.plugins.PluginInformation.load(PluginInformation.java:330)
	at org.openstreetmap.josm.plugins.PluginHandler.loadPlugin(PluginHandler.java:715)
	at org.openstreetmap.josm.plugins.PluginHandler.loadPlugins(PluginHandler.java:767)
	at org.openstreetmap.josm.plugins.PluginHandler.loadLatePlugins(PluginHandler.java:806)
	at org.openstreetmap.josm.gui.MainApplication.loadLatePlugins(MainApplication.java:395)
	at org.openstreetmap.josm.gui.MainApplication.main(MainApplication.java:328)
Caused by: java.lang.reflect.InvocationTargetException
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
	at org.openstreetmap.josm.plugins.PluginInformation.load(PluginInformation.java:327)
	... 5 more
Caused by: java.util.ConcurrentModificationException
	at java.util.LinkedHashMap$LinkedHashIterator.nextNode(LinkedHashMap.java:711)
	at java.util.LinkedHashMap$LinkedValueIterator.next(LinkedHashMap.java:739)
	at org.openstreetmap.josm.tools.Shortcut.findShortcut(Shortcut.java:279)
	at org.openstreetmap.josm.tools.Shortcut.registerShortcut(Shortcut.java:456)
	at org.openstreetmap.josm.tools.Shortcut.registerShortcut(Shortcut.java:443)
	at org.openstreetmap.josm.gui.MainMenu.addMenu(MainMenu.java:632)
	at org.openstreetmap.josm.gui.MainMenu.addMenu(MainMenu.java:617)
	at org.openstreetmap.josm.plugins.fieldpapers.FieldPapersPlugin.<init>(FieldPapersPlugin.java:30)
	... 10 more

JOSM asks to disable the plugin.

comment:11 by simon04, 8 years ago

Milestone: 16.0916.10

comment:12 by simon04, 8 years ago

Ticket #13756 has been marked as a duplicate of this ticket.

comment:13 by simon04, 8 years ago

Milestone: 16.1016.11

Milestone renamed

by simon04, 8 years ago

Attachment: 12030-cowal.patch added

comment:14 by simon04, 8 years ago

Milestone: 16.1116.10
Summary: ConcurrentModificationException in findShortcut[Patch] ConcurrentModificationException in findShortcut

attachment:12030-cowal.patch​ stores the registered shortcuts in a CopyOnWriteArrayList. Since registerSystemShortcut and registerShortcut invoked findShortcut which caused a linear search on the registered shortcuts, little performance degradation should be notable (except for copying the array for 168 times = number of shortcut registrations).

Any thoughts?

comment:15 by michael2402, 8 years ago

findShortcutByKeyOrShortText -> Uses a null parameter to filter. Why not move the predicate out of that function? getKeyStrokeForShortKey has similar logic (although there is less duplicate code than before ;-) )

listAll: "core:none" filter is missing.

I don't really see much need for a hash map - we do only have a few hundred shortcuts per keyboard and we need to scan the list of shortcuts often to detect duplicates. The only place where I see a performance problem is with actions that define a shortcut in their constructor and that are created often. I don't know if any such actions exist.

I don't really like the way registerSystemShortcut/registerShortcut works. It won't crash but it is not correctly synchronized and may lead to
unpredictable results. The main problem I see is that in the above situation, a conflict may not be noticed due to a race condition.

I'd suggest to synchronize the registerShortcut and the registerSystemShortcut methods. If we synchronize all access, we can even remove the CopyOnWriteArrayList and simply use an ArrayList (if listAll uses a filter).

comment:16 by simon04, 8 years ago

Resolution: fixed
Status: reopenedclosed

In 11122/josm:

fix #12030 - ConcurrentModificationException in findShortcut

Use a CopyOnWriteArrayList to store the shortcuts.

in reply to:  15 comment:17 by simon04, 8 years ago

Replying to michael2402:

listAll: "core:none" filter is missing.

Thanks for spotting this glitch.

I committed the CopyOnWriteArrayList variant. Feel free to refactor.

comment:18 by michael2402, 8 years ago

The only thing I would add is a synchronized for register*Shortcut - we might have a rare race condition inside this function.

comment:19 by Klumbumbus, 8 years ago

see #13808 for a regression

comment:20 by simon04, 8 years ago

In 11170/josm:

fix #13808 see #12030 - Custom shortcuts are broken

comment:21 by simon04, 8 years ago

In 11207/josm:

fix #13808 see #12030 - Custom system shortcuts are broken

comment:22 by simon04, 8 years ago

In 11340/josm:

fix #13950 see #12030 - Custom interchanged shortcuts are broken

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.