Modify

Opened 17 months ago

Closed 6 months ago

Last modified 4 months 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 15 months ago.
12030-cowal.patch (9.2 KB) - added by simon04 6 months ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 17 months ago by Don-vip

Component: PluginCore
Milestone: 15.10

comment:2 Changed 17 months ago by Don-vip

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 Changed 17 months ago by Don-vip

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

comment:5 Changed 16 months ago by Don-vip

Milestone: 15.1115.12

comment:6 Changed 15 months ago by Don-vip

Milestone: 15.1216.01

Changed 15 months ago by simon04

Attachment: testParallelAccess.patch added

comment:7 Changed 15 months ago by 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.

comment:8 Changed 15 months ago by wiktorn

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 Changed 15 months ago by Don-vip

Milestone: 16.01

comment:10 Changed 6 months ago by michael2402

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 Changed 6 months ago by simon04

Milestone: 16.0916.10

comment:12 Changed 6 months ago by simon04

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

comment:13 Changed 6 months ago by simon04

Milestone: 16.1016.11

Milestone renamed

Changed 6 months ago by simon04

Attachment: 12030-cowal.patch added

comment:14 Changed 6 months ago by simon04

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 Changed 6 months ago by 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).

comment:16 Changed 6 months ago by simon04

Resolution: fixed
Status: reopenedclosed

In 11122/josm:

fix #12030 - ConcurrentModificationException in findShortcut

Use a CopyOnWriteArrayList to store the shortcuts.

comment:17 in reply to:  15 Changed 6 months ago by simon04

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 Changed 6 months ago by michael2402

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

comment:19 Changed 5 months ago by Klumbumbus

see #13808 for a regression

comment:20 Changed 5 months ago by simon04

In 11170/josm:

fix #13808 see #12030 - Custom shortcuts are broken

comment:21 Changed 5 months ago by simon04

In 11207/josm:

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

comment:22 Changed 4 months ago by simon04

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. Next status will be 'closed'.
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.