Ticket #11689: 0001-Revert-Temporary-fix-lockups-on-data-layer-removal-w.patch

src/org/openstreetmap/josm/gui/MapView.java

@@ -31 +31 @@
 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 +201 @@
      * @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<Layer> layers = new ArrayList<>();
@@ -234 +249 @@
      * <p>
      * 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 +375 @@
      * @param layer the GPX layer
      */
     protected void addGpxLayer(GpxLayer layer) {
-        synchronized (layers) {
+        layerLock.writeLock().lock();
+        try {
             if (layers.isEmpty()) {
                 layers.add(layer);
                 return;
@@ -376 +392 @@
                 }
             }
             layers.add(0, layer);
+        } finally {
+            layerLock.writeLock().unlock();
         }
     }
 
@@ -390 +408 @@
         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 +448 @@
             layer.addPropertyChangeListener(this);
             Main.addProjectionChangeListener(layer);
             AudioPlayer.reset();
+        } finally {
+            layerLock.readLock().unlock();
         }
         if (!listenersToFire.isEmpty()) {
             repaint();
@@ -432 +458 @@
 
     @Override
     protected DataSet getCurrentDataSet() {
-        synchronized (layers) {
+        layerLock.readLock().lock();
+        try {
             if (editLayer != null)
                 return editLayer.data;
             else
                 return null;
+        } finally {
+            layerLock.readLock().unlock();
         }
     }
 
@@ -446 +475 @@
      * @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 +489 @@
      * @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 +534 @@
         Layer oldActiveLayer = activeLayer;
         OsmDataLayer oldEditLayer = editLayer;
 
-        synchronized (layers) {
-            List<Layer> layersList = new ArrayList<>(layers);
+        layerLock.writeLock().lock();
+        layerLock.readLock().lock();
+        try {
+            try {
+                List<Layer> 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 +605 @@
         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 +640 @@
      */
     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 +661 @@
      * first, layer with the highest Z-Order last.
      */
     public List<Layer> getVisibleLayersInZOrder() {
-        synchronized (layers) {
+        layerLock.readLock().lock();
+        try {
             List<Layer> ret = new ArrayList<>();
             // This is set while we delay the addition of the active layer.
             boolean activeLayerDelayed = false;
@@ -632 +687 @@
                 ret.add(activeLayer);
             }
             return ret;
+        } finally {
+            layerLock.readLock().unlock();
         }
     }
 
@@ -820 +877 @@
      * @return An unmodifiable collection of all layers
      */
     public Collection<Layer> getAllLayers() {
-        synchronized (layers) {
+        layerLock.readLock().lock();
+        try {
             return Collections.unmodifiableCollection(new ArrayList<>(layers));
+        } finally {
+            layerLock.readLock().unlock();
         }
     }
 
@@ -829 +889 @@
      * @return An unmodifiable ordered list of all layers
      */
     public List<Layer> getAllLayersAsList() {
-        synchronized (layers) {
+        layerLock.readLock().lock();
+        try {
             return Collections.unmodifiableList(new ArrayList<>(layers));
+        } finally {
+            layerLock.readLock().unlock();
         }
     }
 
@@ -855 +918 @@
      * @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 +986 @@
      * @throws IllegalArgumentException if layer is not in the lis of layers
      */
     public void setActiveLayer(Layer layer) {
+        layerLock.writeLock().lock();
+        layerLock.readLock().lock();
         EnumSet<LayerListenerType> 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 +1032 @@
      * @return the currently active layer (may be null)
      */
     public Layer getActiveLayer() {
-        synchronized (layers) {
+        layerLock.readLock().lock();
+        try {
             return activeLayer;
+        } finally {
+            layerLock.readLock().unlock();
         }
     }
 
@@ -1015 +1091 @@
      * @return the current edit layer. May be null.
      */
     public OsmDataLayer getEditLayer() {
-        synchronized (layers) {
+        layerLock.readLock().lock();
+        try {
             return editLayer;
+        } finally {
+            layerLock.readLock().unlock();
         }
     }
 
@@ -1027 +1106 @@
      * @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 +1175 @@
      */
     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 +1208 @@
         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();
         }