[Patch] ConcurrentModificationException in findShortcut

[wiktorn]

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)

[Don-vip]

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.

[Don-vip]

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

[wiktorn]

But then you still need to synchronise iteration:
​http://stackoverflow.com/questions/13151166/collections-synchronizedmapnew-linkedhashmap-is-not-making-map-threadsafe

[simon04]

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.

[wiktorn]

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

[michael2402]

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.

[simon04]

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

[simon04]

Milestone renamed

[simon04]

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?

[michael2402]

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

[simon04]

In 11122/josm:

fix #12030 - ConcurrentModificationException in findShortcut

Use a CopyOnWriteArrayList to store the shortcuts.

[simon04]

Replying to michael2402:

listAll: "core:none" filter is missing.

Thanks for spotting this glitch.

I committed the CopyOnWriteArrayList variant. Feel free to refactor.

[michael2402]

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

[Klumbumbus]

see #13808 for a regression

[simon04]

In 11170/josm:

fix #13808 see #12030 - Custom shortcuts are broken

[simon04]

In 11207/josm:

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

[simon04]

In 11340/josm:

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