From faadcb49231b0a639a0e1e47783423381b8c9022 Mon Sep 17 00:00:00 2001 From: Michael Zangl Date: Thu, 6 Aug 2015 14:02:04 +0200 Subject: [PATCH 1/3] Revert "Temporary fix lockups on data layer removal when Validation layer is present" This reverts commit f3f0ab13d3c24ceb75494616fb4fbb187dc1eac0. --- src/org/openstreetmap/josm/gui/MapView.java | 220 +++++++++++++++++++--------- 1 file changed, 154 insertions(+), 66 deletions(-) diff --git a/src/org/openstreetmap/josm/gui/MapView.java b/src/org/openstreetmap/josm/gui/MapView.java index bbe824a..ef6d0e8 100644 --- a/src/org/openstreetmap/josm/gui/MapView.java +++ b/src/org/openstreetmap/josm/gui/MapView.java @@ -31,6 +31,7 @@ import java.util.List; import java.util.ListIterator; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.locks.ReentrantReadWriteLock; import javax.swing.AbstractButton; import javax.swing.ActionMap; @@ -200,30 +201,44 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @param newLayer The new active layer. */ protected void fireActiveLayerChanged(Layer oldLayer, Layer newLayer) { + checkLayerLockNotHeld(); for (LayerChangeListener l : layerChangeListeners) { l.activeLayerChange(oldLayer, newLayer); } } protected void fireLayerAdded(Layer newLayer) { + checkLayerLockNotHeld(); for (MapView.LayerChangeListener l : MapView.layerChangeListeners) { l.layerAdded(newLayer); } } protected void fireLayerRemoved(Layer layer) { + checkLayerLockNotHeld(); for (MapView.LayerChangeListener l : MapView.layerChangeListeners) { l.layerRemoved(layer); } } protected void fireEditLayerChanged(OsmDataLayer oldLayer, OsmDataLayer newLayer) { + checkLayerLockNotHeld(); for (EditLayerChangeListener l : editLayerChangeListeners) { l.editLayerChanged(oldLayer, newLayer); } } /** + * This is a simple invariant check that tests if the {@link #layerLock} is not write locked. + * This should be the case whenever a layer listener is invoked. + */ + private void checkLayerLockNotHeld() { + if (layerLock.isWriteLockedByCurrentThread()) { + Main.warn("layerLock is write-held while a listener was called."); + } + } + + /** * A list of all layers currently loaded. Locked by {@link #layerLock}. */ private final transient List layers = new ArrayList<>(); @@ -234,7 +249,7 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer *

* The read lock is always held while those fields are read or while layer change listeners are fired. */ - //private final ReentrantReadWriteLock layerLock = new ReentrantReadWriteLock(); + private final ReentrantReadWriteLock layerLock = new ReentrantReadWriteLock(); /** * The play head marker: there is only one of these so it isn't in any specific layer @@ -360,7 +375,8 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @param layer the GPX layer */ protected void addGpxLayer(GpxLayer layer) { - synchronized (layers) { + layerLock.writeLock().lock(); + try { if (layers.isEmpty()) { layers.add(layer); return; @@ -376,6 +392,8 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer } } layers.add(0, layer); + } finally { + layerLock.writeLock().unlock(); } } @@ -390,30 +408,36 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer Layer oldActiveLayer = activeLayer; OsmDataLayer oldEditLayer = editLayer; - synchronized (layers) { - if (layer instanceof MarkerLayer && playHeadMarker == null) { - playHeadMarker = PlayHeadMarker.create(); - } + layerLock.writeLock().lock(); + layerLock.readLock().lock(); + try { + try { + if (layer instanceof MarkerLayer && playHeadMarker == null) { + playHeadMarker = PlayHeadMarker.create(); + } - if (layer instanceof GpxLayer) { - addGpxLayer((GpxLayer) layer); - } else if (layers.isEmpty()) { - layers.add(layer); - } else if (layer.isBackgroundLayer()) { - int i = 0; - for (; i < layers.size(); i++) { - if (layers.get(i).isBackgroundLayer()) { - break; + if (layer instanceof GpxLayer) { + addGpxLayer((GpxLayer) layer); + } else if (layers.isEmpty()) { + layers.add(layer); + } else if (layer.isBackgroundLayer()) { + int i = 0; + for (; i < layers.size(); i++) { + if (layers.get(i).isBackgroundLayer()) { + break; + } } + layers.add(i, layer); + } else { + layers.add(0, layer); } - layers.add(i, layer); - } else { - layers.add(0, layer); - } - if (isOsmDataLayer || oldActiveLayer == null) { - // autoselect the new layer - listenersToFire.addAll(setActiveLayer(layer, true)); + if (isOsmDataLayer || oldActiveLayer == null) { + // autoselect the new layer + listenersToFire.addAll(setActiveLayer(layer, true)); + } + } finally { + layerLock.writeLock().unlock(); } fireLayerAdded(layer); @@ -424,6 +448,8 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer layer.addPropertyChangeListener(this); Main.addProjectionChangeListener(layer); AudioPlayer.reset(); + } finally { + layerLock.readLock().unlock(); } if (!listenersToFire.isEmpty()) { repaint(); @@ -432,11 +458,14 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer @Override protected DataSet getCurrentDataSet() { - synchronized (layers) { + layerLock.readLock().lock(); + try { if (editLayer != null) return editLayer.data; else return null; + } finally { + layerLock.readLock().unlock(); } } @@ -446,8 +475,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return true if the active data layer (edit layer) is drawable, false otherwise */ public boolean isActiveLayerDrawable() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return editLayer != null; + } finally { + layerLock.readLock().unlock(); } } @@ -457,8 +489,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return true if the active data layer (edit layer) is visible, false otherwise */ public boolean isActiveLayerVisible() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return isActiveLayerDrawable() && editLayer.isVisible(); + } finally { + layerLock.readLock().unlock(); } } @@ -499,30 +534,38 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer Layer oldActiveLayer = activeLayer; OsmDataLayer oldEditLayer = editLayer; - synchronized (layers) { - List layersList = new ArrayList<>(layers); + layerLock.writeLock().lock(); + layerLock.readLock().lock(); + try { + try { + List layersList = new ArrayList<>(layers); - if (!layersList.remove(layer)) - return; + if (!layersList.remove(layer)) + return; - listenersToFire = setEditLayer(layersList); + listenersToFire = setEditLayer(layersList); - if (layer == activeLayer) { - listenersToFire.addAll(setActiveLayer(determineNextActiveLayer(layersList), false)); - } + if (layer == activeLayer) { + listenersToFire.addAll(setActiveLayer(determineNextActiveLayer(layersList), false)); + } - if (layer instanceof OsmDataLayer) { - ((OsmDataLayer) layer).removeLayerPropertyChangeListener(this); - } + if (layer instanceof OsmDataLayer) { + ((OsmDataLayer) layer).removeLayerPropertyChangeListener(this); + } - layers.remove(layer); - Main.removeProjectionChangeListener(layer); + layers.remove(layer); + Main.removeProjectionChangeListener(layer); + } finally { + layerLock.writeLock().unlock(); + } onActiveEditLayerChanged(oldActiveLayer, oldEditLayer, listenersToFire); fireLayerRemoved(layer); layer.removePropertyChangeListener(this); layer.destroy(); AudioPlayer.reset(); + } finally { + layerLock.readLock().unlock(); } repaint(); } @@ -562,21 +605,29 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer Layer oldActiveLayer = activeLayer; OsmDataLayer oldEditLayer = editLayer; - synchronized (layers) { - int curLayerPos = layers.indexOf(layer); - if (curLayerPos == -1) - throw new IllegalArgumentException(tr("Layer not in list.")); - if (pos == curLayerPos) - return; // already in place. - layers.remove(curLayerPos); - if (pos >= layers.size()) { - layers.add(layer); - } else { - layers.add(pos, layer); + layerLock.writeLock().lock(); + layerLock.readLock().lock(); + try { + try { + int curLayerPos = layers.indexOf(layer); + if (curLayerPos == -1) + throw new IllegalArgumentException(tr("Layer not in list.")); + if (pos == curLayerPos) + return; // already in place. + layers.remove(curLayerPos); + if (pos >= layers.size()) { + layers.add(layer); + } else { + layers.add(pos, layer); + } + listenersToFire = setEditLayer(layers); + } finally { + layerLock.writeLock().unlock(); } - listenersToFire = setEditLayer(layers); onActiveEditLayerChanged(oldActiveLayer, oldEditLayer, listenersToFire); AudioPlayer.reset(); + } finally { + layerLock.readLock().unlock(); } repaint(); } @@ -589,8 +640,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer */ public int getLayerPos(Layer layer) { int curLayerPos; - synchronized (layers) { + layerLock.readLock().lock(); + try { curLayerPos = layers.indexOf(layer); + } finally { + layerLock.readLock().unlock(); } if (curLayerPos == -1) throw new IllegalArgumentException(tr("Layer not in list.")); @@ -607,7 +661,8 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * first, layer with the highest Z-Order last. */ public List getVisibleLayersInZOrder() { - synchronized (layers) { + layerLock.readLock().lock(); + try { List ret = new ArrayList<>(); // This is set while we delay the addition of the active layer. boolean activeLayerDelayed = false; @@ -632,6 +687,8 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer ret.add(activeLayer); } return ret; + } finally { + layerLock.readLock().unlock(); } } @@ -820,8 +877,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return An unmodifiable collection of all layers */ public Collection getAllLayers() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return Collections.unmodifiableCollection(new ArrayList<>(layers)); + } finally { + layerLock.readLock().unlock(); } } @@ -829,8 +889,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return An unmodifiable ordered list of all layers */ public List getAllLayersAsList() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return Collections.unmodifiableList(new ArrayList<>(layers)); + } finally { + layerLock.readLock().unlock(); } } @@ -855,8 +918,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return the number of layers managed by this map view */ public int getNumLayers() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return layers.size(); + } finally { + layerLock.readLock().unlock(); } } @@ -920,13 +986,20 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @throws IllegalArgumentException if layer is not in the lis of layers */ public void setActiveLayer(Layer layer) { + layerLock.writeLock().lock(); + layerLock.readLock().lock(); EnumSet listenersToFire; - - synchronized (layers) { - Layer oldActiveLayer = activeLayer; - OsmDataLayer oldEditLayer = editLayer; - listenersToFire = setActiveLayer(layer, true); + Layer oldActiveLayer = activeLayer; + OsmDataLayer oldEditLayer = editLayer; + try { + try { + listenersToFire = setActiveLayer(layer, true); + } finally { + layerLock.writeLock().unlock(); + } onActiveEditLayerChanged(oldActiveLayer, oldEditLayer, listenersToFire); + } finally { + layerLock.readLock().unlock(); } repaint(); } @@ -959,8 +1032,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return the currently active layer (may be null) */ public Layer getActiveLayer() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return activeLayer; + } finally { + layerLock.readLock().unlock(); } } @@ -1015,8 +1091,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return the current edit layer. May be null. */ public OsmDataLayer getEditLayer() { - synchronized (layers) { + layerLock.readLock().lock(); + try { return editLayer; + } finally { + layerLock.readLock().unlock(); } } @@ -1027,8 +1106,11 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer * @return true if the list of layers managed by this map view contain layer */ public boolean hasLayer(Layer layer) { - synchronized (layers) { + layerLock.readLock().lock(); + try { return layers.contains(layer); + } finally { + layerLock.readLock().unlock(); } } @@ -1093,11 +1175,14 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer */ protected void refreshTitle() { if (Main.parent != null) { - synchronized (layers) { + layerLock.readLock().lock(); + try { boolean dirty = editLayer != null && (editLayer.requiresSaveToFile() || (editLayer.requiresUploadToServer() && !editLayer.isUploadDiscouraged())); ((JFrame) Main.parent).setTitle((dirty ? "* " : "") + tr("Java OpenStreetMap Editor")); ((JFrame) Main.parent).getRootPane().putClientProperty("Window.documentModified", dirty); + } finally { + layerLock.readLock().unlock(); } } } @@ -1123,13 +1208,16 @@ implements PropertyChangeListener, PreferenceChangedListener, OsmDataLayer.Layer if (mapMover != null) { mapMover.destroy(); } - synchronized (layers) { + layerLock.writeLock().lock(); + try { activeLayer = null; changedLayer = null; editLayer = null; layers.clear(); - nonChangedLayers.clear(); + } finally { + layerLock.writeLock().unlock(); } + nonChangedLayers.clear(); synchronized (temporaryLayers) { temporaryLayers.clear(); } -- 1.9.1