Changeset 8642 in josm


Ignore:
Timestamp:
2015-08-05T19:48:13+02:00 (9 years ago)
Author:
wiktorn
Message:

Temporary fix lockups on data layer removal when Validation layer is present

Replace ReentrantReadWriteLock with Java Monitors.

Addresses: #11689

File:
1 edited

Legend:

Unmodified
Added
Removed

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

    r8631 r8642  
    3232import java.util.Set;
    3333import java.util.concurrent.CopyOnWriteArrayList;
    34 import java.util.concurrent.locks.ReentrantReadWriteLock;
    3534
    3635import javax.swing.AbstractButton;
    … …  
    202201     */
    203202    protected void fireActiveLayerChanged(Layer oldLayer, Layer newLayer) {
    204         checkLayerLockNotHeld();
    205203        for (LayerChangeListener l : layerChangeListeners) {
    206204            l.activeLayerChange(oldLayer, newLayer);
    … …  
    209207
    210208    protected void fireLayerAdded(Layer newLayer) {
    211         checkLayerLockNotHeld();
    212209        for (MapView.LayerChangeListener l : MapView.layerChangeListeners) {
    213210            l.layerAdded(newLayer);
    … …  
    216213
    217214    protected void fireLayerRemoved(Layer layer) {
    218         checkLayerLockNotHeld();
    219215        for (MapView.LayerChangeListener l : MapView.layerChangeListeners) {
    220216            l.layerRemoved(layer);
    … …  
    223219
    224220    protected void fireEditLayerChanged(OsmDataLayer oldLayer, OsmDataLayer newLayer) {
    225         checkLayerLockNotHeld();
    226221        for (EditLayerChangeListener l : editLayerChangeListeners) {
    227222            l.editLayerChanged(oldLayer, newLayer);
    228         }
    229     }
    230 
    231     /**
    232      * This is a simple invariant check that tests if the {@link #layerLock} is not write locked.
    233      * This should be the case whenever a layer listener is invoked.
    234      */
    235     private void checkLayerLockNotHeld() {
    236         if (layerLock.isWriteLockedByCurrentThread()) {
    237             Main.warn("layerLock is write-held while a listener was called.");
    238223        }
    239224    }
    … …  
    250235     * The read lock is always held while those fields are read or while layer change listeners are fired.
    251236     */
    252     private final ReentrantReadWriteLock layerLock = new ReentrantReadWriteLock();
     237    //private final ReentrantReadWriteLock layerLock = new ReentrantReadWriteLock();
    253238
    254239    /**
    … …  
    376361     */
    377362    protected void addGpxLayer(GpxLayer layer) {
    378         layerLock.writeLock().lock();
    379         try {
     363        synchronized (layers) {
    380364            if (layers.isEmpty()) {
    381365                layers.add(layer);
    … …  
    393377            }
    394378            layers.add(0, layer);
    395         } finally {
    396             layerLock.writeLock().unlock();
    397379        }
    398380    }
    … …  
    409391        OsmDataLayer oldEditLayer = editLayer;
    410392
    411         layerLock.writeLock().lock();
    412         layerLock.readLock().lock();
    413         try {
    414             try {
    415                 if (layer instanceof MarkerLayer && playHeadMarker == null) {
    416                     playHeadMarker = PlayHeadMarker.create();
     393        synchronized (layers) {
     394            if (layer instanceof MarkerLayer && playHeadMarker == null) {
     395                playHeadMarker = PlayHeadMarker.create();
     396            }
     397
     398            if (layer instanceof GpxLayer) {
     399                addGpxLayer((GpxLayer) layer);
     400            } else if (layers.isEmpty()) {
     401                layers.add(layer);
     402            } else if (layer.isBackgroundLayer()) {
     403                int i = 0;
     404                for (; i < layers.size(); i++) {
     405                    if (layers.get(i).isBackgroundLayer()) {
     406                        break;
     407                    }
    417408                }
    418 
    419                 if (layer instanceof GpxLayer) {
    420                     addGpxLayer((GpxLayer) layer);
    421                 } else if (layers.isEmpty()) {
    422                     layers.add(layer);
    423                 } else if (layer.isBackgroundLayer()) {
    424                     int i = 0;
    425                     for (; i < layers.size(); i++) {
    426                         if (layers.get(i).isBackgroundLayer()) {
    427                             break;
    428                         }
    429                     }
    430                     layers.add(i, layer);
    431                 } else {
    432                     layers.add(0, layer);
    433                 }
    434 
    435                 if (isOsmDataLayer || oldActiveLayer == null) {
    436                     // autoselect the new layer
    437                     listenersToFire.addAll(setActiveLayer(layer, true));
    438                 }
    439             } finally {
    440                 layerLock.writeLock().unlock();
     409                layers.add(i, layer);
     410            } else {
     411                layers.add(0, layer);
     412            }
     413
     414            if (isOsmDataLayer || oldActiveLayer == null) {
     415                // autoselect the new layer
     416                listenersToFire.addAll(setActiveLayer(layer, true));
    441417            }
    442418
    … …  
    449425            Main.addProjectionChangeListener(layer);
    450426            AudioPlayer.reset();
    451         } finally {
    452             layerLock.readLock().unlock();
    453427        }
    454428        if (!listenersToFire.isEmpty()) {
    … …  
    459433    @Override
    460434    protected DataSet getCurrentDataSet() {
    461         layerLock.readLock().lock();
    462         try {
     435        synchronized (layers) {
    463436            if (editLayer != null)
    464437                return editLayer.data;
    465438            else
    466439                return null;
    467         } finally {
    468             layerLock.readLock().unlock();
    469440        }
    470441    }
    … …  
    476447     */
    477448    public boolean isActiveLayerDrawable() {
    478         layerLock.readLock().lock();
    479         try {
     449        synchronized (layers) {
    480450            return editLayer != null;
    481         } finally {
    482             layerLock.readLock().unlock();
    483451        }
    484452    }
    … …  
    490458     */
    491459    public boolean isActiveLayerVisible() {
    492         layerLock.readLock().lock();
    493         try {
     460        synchronized (layers) {
    494461            return isActiveLayerDrawable() && editLayer.isVisible();
    495         } finally {
    496             layerLock.readLock().unlock();
    497462        }
    498463    }
    … …  
    535500        OsmDataLayer oldEditLayer = editLayer;
    536501
    537         layerLock.writeLock().lock();
    538         layerLock.readLock().lock();
    539         try {
    540             try {
    541                 List<Layer> layersList = new ArrayList<>(layers);
    542 
    543                 if (!layersList.remove(layer))
    544                     return;
    545 
    546                 listenersToFire = setEditLayer(layersList);
    547 
    548                 if (layer == activeLayer) {
    549                     listenersToFire.addAll(setActiveLayer(determineNextActiveLayer(layersList), false));
    550                 }
    551 
    552                 if (layer instanceof OsmDataLayer) {
    553                     ((OsmDataLayer) layer).removeLayerPropertyChangeListener(this);
    554                 }
    555 
    556                 layers.remove(layer);
    557                 Main.removeProjectionChangeListener(layer);
    558 
    559             } finally {
    560                 layerLock.writeLock().unlock();
    561             }
     502        synchronized (layers) {
     503            List<Layer> layersList = new ArrayList<>(layers);
     504
     505            if (!layersList.remove(layer))
     506                return;
     507
     508            listenersToFire = setEditLayer(layersList);
     509
     510            if (layer == activeLayer) {
     511                listenersToFire.addAll(setActiveLayer(determineNextActiveLayer(layersList), false));
     512            }
     513
     514            if (layer instanceof OsmDataLayer) {
     515                ((OsmDataLayer) layer).removeLayerPropertyChangeListener(this);
     516            }
     517
     518            layers.remove(layer);
     519            Main.removeProjectionChangeListener(layer);
     520
    562521            onActiveEditLayerChanged(oldActiveLayer, oldEditLayer, listenersToFire);
    563522            fireLayerRemoved(layer);
    … …  
    565524            layer.destroy();
    566525            AudioPlayer.reset();
    567         } finally {
    568             layerLock.readLock().unlock();
    569526        }
    570527        repaint();
    … …  
    606563        OsmDataLayer oldEditLayer = editLayer;
    607564
    608         layerLock.writeLock().lock();
    609         layerLock.readLock().lock();
    610         try {
    611             try {
    612                 int curLayerPos = layers.indexOf(layer);
    613                 if (curLayerPos == -1)
    614                     throw new IllegalArgumentException(tr("Layer not in list."));
    615                 if (pos == curLayerPos)
    616                     return; // already in place.
    617                 layers.remove(curLayerPos);
    618                 if (pos >= layers.size()) {
    619                     layers.add(layer);
    620                 } else {
    621                     layers.add(pos, layer);
    622                 }
    623                 listenersToFire = setEditLayer(layers);
    624             } finally {
    625                 layerLock.writeLock().unlock();
    626             }
     565        synchronized (layers) {
     566            int curLayerPos = layers.indexOf(layer);
     567            if (curLayerPos == -1)
     568                throw new IllegalArgumentException(tr("Layer not in list."));
     569            if (pos == curLayerPos)
     570                return; // already in place.
     571            layers.remove(curLayerPos);
     572            if (pos >= layers.size()) {
     573                layers.add(layer);
     574            } else {
     575                layers.add(pos, layer);
     576            }
     577            listenersToFire = setEditLayer(layers);
    627578            onActiveEditLayerChanged(oldActiveLayer, oldEditLayer, listenersToFire);
    628579            AudioPlayer.reset();
    629         } finally {
    630             layerLock.readLock().unlock();
    631580        }
    632581        repaint();
    … …  
    641590    public int getLayerPos(Layer layer) {
    642591        int curLayerPos;
    643         layerLock.readLock().lock();
    644         try {
     592        synchronized (layers) {
    645593            curLayerPos = layers.indexOf(layer);
    646         } finally {
    647             layerLock.readLock().unlock();
    648594        }
    649595        if (curLayerPos == -1)
    … …  
    662608     */
    663609    public List<Layer> getVisibleLayersInZOrder() {
    664         layerLock.readLock().lock();
    665         try {
     610        synchronized (layers) {
    666611            List<Layer> ret = new ArrayList<>();
    667612            // This is set while we delay the addition of the active layer.
    … …  
    688633            }
    689634            return ret;
    690         } finally {
    691             layerLock.readLock().unlock();
    692635        }
    693636    }
    … …  
    878821     */
    879822    public Collection<Layer> getAllLayers() {
    880         layerLock.readLock().lock();
    881         try {
     823        synchronized (layers) {
    882824            return Collections.unmodifiableCollection(new ArrayList<>(layers));
    883         } finally {
    884             layerLock.readLock().unlock();
    885825        }
    886826    }
    … …  
    890830     */
    891831    public List<Layer> getAllLayersAsList() {
    892         layerLock.readLock().lock();
    893         try {
     832        synchronized (layers) {
    894833            return Collections.unmodifiableList(new ArrayList<>(layers));
    895         } finally {
    896             layerLock.readLock().unlock();
    897834        }
    898835    }
    … …  
    919856     */
    920857    public int getNumLayers() {
    921         layerLock.readLock().lock();
    922         try {
     858        synchronized (layers) {
    923859            return layers.size();
    924         } finally {
    925             layerLock.readLock().unlock();
    926860        }
    927861    }
    … …  
    987921     */
    988922    public void setActiveLayer(Layer layer) {
    989         layerLock.writeLock().lock();
    990         layerLock.readLock().lock();
    991923        EnumSet<LayerListenerType> listenersToFire;
    992         Layer oldActiveLayer = activeLayer;
    993         OsmDataLayer oldEditLayer = editLayer;
    994         try {
    995             try {
    996                 listenersToFire = setActiveLayer(layer, true);
    997             } finally {
    998                 layerLock.writeLock().unlock();
    999             }
     924
     925        synchronized (layers) {
     926            Layer oldActiveLayer = activeLayer;
     927            OsmDataLayer oldEditLayer = editLayer;
     928            listenersToFire = setActiveLayer(layer, true);
    1000929            onActiveEditLayerChanged(oldActiveLayer, oldEditLayer, listenersToFire);
    1001         } finally {
    1002             layerLock.readLock().unlock();
    1003930        }
    1004931        repaint();
    … …  
    1033960     */
    1034961    public Layer getActiveLayer() {
    1035         layerLock.readLock().lock();
    1036         try {
     962        synchronized (layers) {
    1037963            return activeLayer;
    1038         } finally {
    1039             layerLock.readLock().unlock();
    1040964        }
    1041965    }
    … …  
    10921016     */
    10931017    public OsmDataLayer getEditLayer() {
    1094         layerLock.readLock().lock();
    1095         try {
     1018        synchronized (layers) {
    10961019            return editLayer;
    1097         } finally {
    1098             layerLock.readLock().unlock();
    10991020        }
    11001021    }
    … …  
    11071028     */
    11081029    public boolean hasLayer(Layer layer) {
    1109         layerLock.readLock().lock();
    1110         try {
     1030        synchronized (layers) {
    11111031            return layers.contains(layer);
    1112         } finally {
    1113             layerLock.readLock().unlock();
    11141032        }
    11151033    }
    … …  
    11761094    protected void refreshTitle() {
    11771095        if (Main.parent != null) {
    1178             layerLock.readLock().lock();
    1179             try {
     1096            synchronized (layers) {
    11801097                boolean dirty = editLayer != null &&
    11811098                        (editLayer.requiresSaveToFile() || (editLayer.requiresUploadToServer() && !editLayer.isUploadDiscouraged()));
    11821099                ((JFrame) Main.parent).setTitle((dirty ? "* " : "") + tr("Java OpenStreetMap Editor"));
    11831100                ((JFrame) Main.parent).getRootPane().putClientProperty("Window.documentModified", dirty);
    1184             } finally {
    1185                 layerLock.readLock().unlock();
    11861101            }
    11871102        }
    … …  
    12091124            mapMover.destroy();
    12101125        }
    1211         layerLock.writeLock().lock();
    1212         try {
     1126        synchronized (layers) {
    12131127            activeLayer = null;
    12141128            changedLayer = null;
    12151129            editLayer = null;
    12161130            layers.clear();
    1217         } finally {
    1218             layerLock.writeLock().unlock();
    1219         }
    1220         nonChangedLayers.clear();
     1131            nonChangedLayers.clear();
     1132        }
    12211133        synchronized (temporaryLayers) {
    12221134            temporaryLayers.clear();
Note: See TracChangeset for help on using the changeset viewer.