[PATCH] Simplify Way dialog can cause a deadlock

[jim.walseth@…]

What steps will reproduce the problem?

1. Open the attached file "SimplifyCrash.osm" using JOSM.app on Mac (Ventura, 13.5.2)
2. Select the cluster of 5 buildings in the center (screenshot attached).
3. Use Shift+Y to launch simplification dialog.

What is the expected result?

The dialog will 'preview' how many nodes will be removed. Clicking Simplify removes them.

What happens instead?

The dialog is unresponsive. I have to "force quit" JOSM.

Please provide any additional information below. Attach a screenshot if possible.

Revision:18822
Build-Date:2023-08-30 11:52:18

Identification: JOSM/1.5 (18822 en) Mac OS X 13.5.2
OS Build number: macOS 13.5.2 (22G91)
Memory Usage: 454 MB / 4096 MB (248 MB allocated, but free)
Java version: 17.0.8+7-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.apple.laf.AquaLookAndFeel
Screen: Display 69734272 1440×900 (scaling 2.00×2.00) Display 722470485 1920×1080 (scaling 1.00×1.00) Display 724048470 1920×1200 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1200
Best cursor sizes: 16×16→16×16, 32×32→32×32
System property file.encoding: UTF-8
System property sun.jnu.encoding: UTF-8
Locale info: en_US
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Djpackage.app-version=18822, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.base/sun.security.action=ALL-UNNAMED, --add-exports=java.desktop/com.apple.eawt=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, -Djpackage.app-path=/Applications/JOSM.app/Contents/MacOS/JOSM]

Plugins:
+ buildings_tools (36126)
+ mapathoner (1.5.2)
+ scripting (v0.2.10)
+ todo (133)
+ utilsplugin2 (36126)

Map paint styles:
+ ${HOME}/Downloads/josm_styles-master.zip
- https://josm.openstreetmap.de/josmfile?page=Styles/SimpleBuildingTags&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/SimpleRoofTags&zip=1
+ https://github.com/osmlab/appledata/archive/josm_paint_inline_validation.zip
+ https://github.com/MissingMaps/josm_styles/archive/master.zip
+ https://github.com/osmlab/applepaintstyles/archive/main.zip
- https://gitlab.com/peculiar-theater/mapcss/-/archive/main/mapcss-main.zip?path=osm-meta
- https://josm.openstreetmap.de/josmfile?page=Styles/ColorWays&zip=1

Last errors/warnings:
- 00004.481 W: Failed to open file with extension 'mapcss' and namepart 'style' in zip file 'josm_styles-master.zip'. Exception was: java.nio.file.NoSuchFileException: ${HOME}/Downloads/josm_styles-master.zip: java.nio.file.NoSuchFileException: ${HOME}/Downloads/josm_styles-master.zip
- 00004.494 W: Failed to load Mappaint styles from '${HOME}/Downloads/josm_styles-master.zip'. Exception was: java.nio.file.NoSuchFileException: ${HOME}/Downloads/josm_styles-master.zip
- 00004.494 E: java.nio.file.NoSuchFileException: ${HOME}/Downloads/josm_styles-master.zip
- 00004.539 W: Initializing map style ${HOME}/Downloads/josm_styles-master.zip completed in 44 ms (1 errors, 0 warnings)

[jim.walseth@…]

screenshot of the crash/hang

[taylor.smock]

I think I found the problem. It doesn't occur reliably, but it looks like we are getting a write lock in SimplifyWayAction.actionPerformed, then a read lock in the same thread in StyledMapRenderer.render (which is fine). The problem is that we then create a ComputeStyleListWorker and run it in a different thread, which then wants to get a read lock in DataSet.search(Nodes|Ways|Relations), but cannot since the EDT thread has a write lock.

@jim.walseth: I was unable to reproduce without adding the paintstyles. As a workaround until I get this fixed and you update to the fixed version, you can try disabling the additional paintstyles (inline validation, missing maps, and the apple paint style) while simplifying ways.

"AWT-EventQueue-0@3285" tid=0x27 nid=NA waiting
  java.lang.Thread.State: WAITING
	  at jdk.internal.misc.Unsafe.park(Unsafe.java:-1)
	  at java.util.concurrent.locks.LockSupport.park(LockSupport.java:371)
	  at java.util.concurrent.ForkJoinTask.awaitDone(ForkJoinTask.java:461)
	  at java.util.concurrent.ForkJoinTask.join(ForkJoinTask.java:651)
	  at java.util.concurrent.ForkJoinPool.invoke(ForkJoinPool.java:2822)
	  at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer.paintWithLock(StyledMapRenderer.java:1676)
	  at org.openstreetmap.josm.data.osm.visitor.paint.StyledMapRenderer.render(StyledMapRenderer.java:1645)
	  at org.openstreetmap.josm.gui.layer.OsmDataLayer.paint(OsmDataLayer.java:587)
	  at org.openstreetmap.josm.gui.layer.AbstractMapViewPaintable$CompatibilityModeLayerPainter.paint(AbstractMapViewPaintable.java:27)
	  at org.openstreetmap.josm.gui.MapView.paintLayer(MapView.java:476)
	  at org.openstreetmap.josm.gui.MapView.drawMapContent(MapView.java:591)
	  at org.openstreetmap.josm.gui.MapView.paint(MapView.java:498)
	  at javax.swing.JComponent.paintChildren(JComponent.java:961)
	  - locked <0x2615> (a java.awt.Component$AWTTreeLock)
	  at javax.swing.JComponent.paint(JComponent.java:1137)
	  at javax.swing.JComponent.paintToOffscreen(JComponent.java:5318)
	  at javax.swing.RepaintManager$PaintManager.paintDoubleBufferedImpl(RepaintManager.java:1656)
	  at javax.swing.RepaintManager$PaintManager.paintDoubleBuffered(RepaintManager.java:1631)
	  at javax.swing.RepaintManager$PaintManager.paint(RepaintManager.java:1569)
	  at javax.swing.RepaintManager.paint(RepaintManager.java:1336)
	  at javax.swing.JComponent._paintImmediately(JComponent.java:5266)
	  at javax.swing.JComponent.paintImmediately(JComponent.java:5076)
	  at javax.swing.RepaintManager$4.run(RepaintManager.java:878)
	  at javax.swing.RepaintManager$4.run(RepaintManager.java:861)
	  at java.security.AccessController.executePrivileged(AccessController.java:778)
	  at java.security.AccessController.doPrivileged(AccessController.java:400)
	  at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:87)
	  at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:861)
	  at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:834)
	  at javax.swing.RepaintManager.prePaintDirtyRegions(RepaintManager.java:784)
	  at javax.swing.RepaintManager$ProcessingRunnable.run(RepaintManager.java:1897)
	  at java.awt.event.InvocationEvent.dispatch$$$capture(InvocationEvent.java:318)
	  at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:-1)
	  at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:773)
	  at java.awt.EventQueue$4.run(EventQueue.java:720)
	  at java.awt.EventQueue$4.run(EventQueue.java:714)
	  at java.security.AccessController.executePrivileged(AccessController.java:778)
	  at java.security.AccessController.doPrivileged(AccessController.java:400)
	  at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:87)
	  at java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	  at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	  at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	  at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:117)
	  at java.awt.WaitDispatchSupport$2.run(WaitDispatchSupport.java:191)
	  at java.awt.WaitDispatchSupport$4.run(WaitDispatchSupport.java:236)
	  at java.awt.WaitDispatchSupport$4.run(WaitDispatchSupport.java:234)
	  at java.security.AccessController.executePrivileged(AccessController.java:778)
	  at java.security.AccessController.doPrivileged(AccessController.java:319)
	  at java.awt.WaitDispatchSupport.enter(WaitDispatchSupport.java:234)
	  at java.awt.Dialog.show(Dialog.java:1079)
	  at java.awt.Component.show(Component.java:1728)
	  at java.awt.Component.setVisible(Component.java:1675)
	  at java.awt.Window.setVisible(Window.java:1036)
	  at java.awt.Dialog.setVisible(Dialog.java:1015)
	  at org.openstreetmap.josm.gui.ExtendedDialog.setVisible(ExtendedDialog.java:462)
	  at org.openstreetmap.josm.gui.ExtendedDialog.showDialog(ExtendedDialog.java:258)
	  at org.openstreetmap.josm.actions.SimplifyWayAction.askSimplifyWays(SimplifyWayAction.java:183)
	  at org.openstreetmap.josm.actions.SimplifyWayAction.lambda$actionPerformed$2(SimplifyWayAction.java:220)
	  at org.openstreetmap.josm.actions.SimplifyWayAction$$Lambda/0x000000012e70fb50.run(Unknown Source:-1)
	  at org.openstreetmap.josm.data.osm.DataSet.update(DataSet.java:1063)
	  at org.openstreetmap.josm.actions.SimplifyWayAction.actionPerformed(SimplifyWayAction.java:206)
          [...]

"styled-map-renderer-1@9648" tid=0x51 nid=NA waiting
  java.lang.Thread.State: WAITING
	  at jdk.internal.misc.Unsafe.park(Unsafe.java:-1)
	  at java.util.concurrent.locks.LockSupport.park(LockSupport.java:221)
	  at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:754)
	  at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireShared(AbstractQueuedSynchronizer.java:1079)
	  at java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock.lock(ReentrantReadWriteLock.java:738)
	  at org.openstreetmap.josm.data.osm.DataSet.searchNodes(DataSet.java:410)
	  at org.openstreetmap.josm.gui.mappaint.mapcss.Selector$ChildOrParentSelector.visitBBox(Selector.java:505)
	  at org.openstreetmap.josm.gui.mappaint.mapcss.Selector$ChildOrParentSelector.matches(Selector.java:542)
	  at org.openstreetmap.josm.gui.mappaint.mapcss.MapCSSStyleSource.apply(MapCSSStyleSource.java:370)
	  at org.openstreetmap.josm.gui.mappaint.ElemStyles.generateStyles(ElemStyles.java:398)
	  at org.openstreetmap.josm.gui.mappaint.ElemStyles.getImpl(ElemStyles.java:261)
	  at org.openstreetmap.josm.gui.mappaint.ElemStyles.getStyleCacheWithRange(ElemStyles.java:173)
	  - locked <0x2619> (a org.openstreetmap.josm.data.osm.Way)
	  at org.openstreetmap.josm.gui.mappaint.ElemStyles.get(ElemStyles.java:150)
	  at org.openstreetmap.josm.data.osm.visitor.paint.ComputeStyleListWorker.add(ComputeStyleListWorker.java:171)
	  at org.openstreetmap.josm.data.osm.visitor.paint.ComputeStyleListWorker.visit(ComputeStyleListWorker.java:143)
	  at org.openstreetmap.josm.data.osm.Way.accept(Way.java:184)
	  at org.openstreetmap.josm.data.osm.visitor.paint.ComputeStyleListWorker.acceptDrawable(ComputeStyleListWorker.java:129)
	  at org.openstreetmap.josm.data.osm.visitor.paint.ComputeStyleListWorker.computeDirectly(ComputeStyleListWorker.java:116)
	  at org.openstreetmap.josm.data.osm.visitor.paint.ComputeStyleListWorker.compute(ComputeStyleListWorker.java:93)
	  at org.openstreetmap.josm.data.osm.visitor.paint.ComputeStyleListWorker.compute(ComputeStyleListWorker.java:34)
          [...]

[jwalseth]

Way to jump on this Taylor, thank you! Feels exactly like a deadlock. Perhaps I will be able to go back to my practice of selecting many buildings and simplifying them en masse.

[taylor.smock]

I've been playing around with the code a bit, and I think I'm going to have to push this off to milestone:"23.10". I'm doing some defensive programing to deal with some other code changing the selection (or deleting a selected way) while people have the simplify way dialog open.

Right now, I don't think we have any code that does that, but I want to avoid issues in the future.

[taylor.smock]

Handle other threads changing selection better

[taylor.smock]

In 18851/josm:

Fix #23203: Simplify Way dialog can cause a deadlock

This is caused by a write lock being acquired in SimplifyWayAction.actionPerformed
in the EDT and then a read lock is attempted to be acquired in a styled-map-renderer
thread. The styled-map-renderer thread is waited on by the EDT, which causes the
deadlock. The fix is removing the dataset write lock in SimplifyWayAction.actionPerformed
which was added back in 2011. An analysis of the code shows that the commands generated
lock the dataset themselves, so the dataset lock in SimplifyWayAction.actionPerformed
should no longer be necessary.

The deadlock does not happen reliably with only the JOSM default paintstyle; adding
additional paintstyles was required for the initial analysis.

[taylor.smock]

Ticket retargeted after milestone deleted

[GerdP]

See #23399 for a regression.