Ticket #23203: 23203.2.patch

File 23203.2.patch, 9.7 KB (added by taylor.smock, 3 years ago)

Handle other threads changing selection better

  • src/org/openstreetmap/josm/actions/SimplifyWayAction.java 

    Subject: [PATCH] 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.
---
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
diff --git a/src/org/openstreetmap/josm/actions/SimplifyWayAction.java b/src/org/openstreetmap/josm/actions/SimplifyWayAction.java
    a b  
    1717import java.util.List;
    1818import java.util.Objects;
    1919import java.util.Set;
     20import java.util.function.Consumer;
     21import java.util.function.Supplier;
    2022import java.util.stream.Collectors;
    2123
    2224import javax.swing.BorderFactory;
    … …  
    3739import org.openstreetmap.josm.data.SystemOfMeasurement;
    3840import org.openstreetmap.josm.data.UndoRedoHandler;
    3941import org.openstreetmap.josm.data.coor.EastNorth;
     42import org.openstreetmap.josm.data.osm.DataSelectionListener;
    4043import org.openstreetmap.josm.data.osm.DataSet;
    4144import org.openstreetmap.josm.data.osm.Node;
    4245import org.openstreetmap.josm.data.osm.OsmPrimitive;
    … …  
    4750import org.openstreetmap.josm.gui.HelpAwareOptionPane.ButtonSpec;
    4851import org.openstreetmap.josm.gui.MainApplication;
    4952import org.openstreetmap.josm.gui.Notification;
     53import org.openstreetmap.josm.gui.util.GuiHelper;
    5054import org.openstreetmap.josm.spi.preferences.Config;
    5155import org.openstreetmap.josm.spi.preferences.IPreferences;
    5256import org.openstreetmap.josm.tools.GBC;
    … …  
    129133     * @since 16566
    130134     */
    131135    public static double askSimplifyWays(List<Way> ways, String text, boolean auto) {
    132         IPreferences s = Config.getPref();
    133         String key = "simplify-way." + (auto ? "auto." : "");
    134         String keyRemember = key + "remember";
    135         String keyError = key + "max-error";
     136        return askSimplifyWays(ways, () -> text, null, auto);
     137    }
    136138
    137         String r = s.get(keyRemember, "ask");
     139    /**
     140     * Asks the user for max-err value used to simplify ways, if not remembered before
     141     * @param ways the ways that are being simplified (to show estimated number of nodes to be removed)
     142     * @param textSupplier the text being shown (called when the DataSet selection changes)
     143     * @param auto whether it's called automatically (conversion) or by the user
     144     * @param listener The dataset selection update listener
     145     * @return the max-err value or -1 if canceled
     146     */
     147    private static double askSimplifyWays(List<Way> ways, Supplier<String> textSupplier, SimplifyWayDataSelectionListener listener,
     148                                          boolean auto) {
     149        final IPreferences s = Config.getPref();
     150        final String key = "simplify-way." + (auto ? "auto." : "");
     151        final String keyRemember = key + "remember";
     152        final String keyError = key + "max-error";
     153
     154        final String r = s.get(keyRemember, "ask");
    138155        if (auto && "no".equals(r)) {
    139156            return -1;
    140157        } else if ("yes".equals(r)) {
    141158            return s.getDouble(keyError, 3.0);
    142159        }
    143160
    144         JPanel p = new JPanel(new GridBagLayout());
    145         p.add(new JLabel("<html><body style=\"width: 375px;\">" + text + "<br><br>" +
     161        final JPanel p = new JPanel(new GridBagLayout());
     162        final Supplier<String> actualTextSupplier = () -> "<html><body style=\"width: 375px;\">" + textSupplier.get() + "<br><br>" +
    146163                tr("This reduces unnecessary nodes along the way and is especially recommended if GPS tracks were recorded by time "
    147                  + "(e.g. one point per second) or when the accuracy was low (reduces \"zigzag\" tracks).")
    148                 + "</body></html>"), GBC.eol());
     164                        + "(e.g. one point per second) or when the accuracy was low (reduces \"zigzag\" tracks).")
     165                + "</body></html>";
     166        final JLabel textLabel = new JLabel(actualTextSupplier.get());
     167        p.add(textLabel, GBC.eol());
    149168        p.setBorder(BorderFactory.createEmptyBorder(5, 10, 10, 5));
    150169        JPanel q = new JPanel(new GridBagLayout());
    151170        q.add(new JLabel(tr("Maximum error (meters): ")));
    … …  
    157176
    158177        JLabel nodesToRemove = new JLabel();
    159178        SimplifyChangeListener l = new SimplifyChangeListener(nodesToRemove, errorModel, ways);
     179        final Runnable changeCleanup = () -> {
     180            if (l.lastCommand != null && l.lastCommand.equals(UndoRedoHandler.getInstance().getLastCommand())) {
     181                UndoRedoHandler.getInstance().undo();
     182                l.lastCommand = null;
     183            }
     184        };
     185        if (listener != null) {
     186            listener.addConsumer(ignored -> {
     187                textLabel.setText(actualTextSupplier.get());
     188                changeCleanup.run();
     189                l.stateChanged(null);
     190            });
     191        }
    160192        if (!ways.isEmpty()) {
    161193            errorModel.addChangeListener(l);
    162194            l.stateChanged(null);
    … …  
    182214
    183215        int ret = ed.showDialog().getValue();
    184216        double val = (double) n.getValue();
    185         if (l.lastCommand != null && l.lastCommand.equals(UndoRedoHandler.getInstance().getLastCommand())) {
    186             UndoRedoHandler.getInstance().undo();
    187             l.lastCommand = null;
    188         }
     217        changeCleanup.run();
    189218        if (ret == 1) {
    190219            s.putDouble(keyError, val);
    191220            if (c.isSelected()) {
    … …  
    202231
    203232    @Override
    204233    public void actionPerformed(ActionEvent e) {
    205         DataSet ds = getLayerManager().getEditDataSet();
    206         ds.update(() -> {
    207             List<Way> ways = ds.getSelectedWays().stream()
    208                     .filter(p -> !p.isIncomplete())
    209                     .collect(Collectors.toList());
     234        final DataSet ds = getLayerManager().getEditDataSet();
     235        final List<Way> ways = new ArrayList<>();
     236        final SimplifyWayDataSelectionListener listener = new SimplifyWayDataSelectionListener(ways);
     237        ds.addSelectionListener(listener);
     238        try {
     239            listener.updateWayList(ds);
    210240            if (ways.isEmpty()) {
    211241                alertSelectAtLeastOneWay();
    212242                return;
    … …  
    214244                return;
    215245            }
    216246
    217             String lengthstr = SystemOfMeasurement.getSystemOfMeasurement().getDistText(
     247            final Supplier<String> lengthStr = () -> SystemOfMeasurement.getSystemOfMeasurement().getDistText(
    218248                    ways.stream().mapToDouble(Way::getLength).sum());
    219249
    220             double err = askSimplifyWays(ways, trn(
    221                     "You are about to simplify {0} way with a total length of {1}.",
    222                     "You are about to simplify {0} ways with a total length of {1}.",
    223                     ways.size(), ways.size(), lengthstr), false);
     250            final double err = askSimplifyWays(ways, () -> trn(
     251                                "You are about to simplify {0} way with a total length of {1}.",
     252                                "You are about to simplify {0} ways with a total length of {1}.",
     253                                ways.size(), ways.size(), lengthStr.get()), listener, false);
    224254
    225255            if (err > 0) {
    226256                simplifyWays(ways, err);
    227257            }
    228         });
     258        } finally {
     259            ds.removeSelectionListener(listener);
     260        }
    229261    }
    230262
    231263    /**
    … …  
    248280            isRequired = frequency > 1;
    249281        }
    250282        if (!isRequired) {
    251             List<OsmPrimitive> parents = new LinkedList<>();
    252             parents.addAll(node.getReferrers());
     283            List<OsmPrimitive> parents = new LinkedList<>(node.getReferrers());
    253284            parents.remove(way);
    254285            isRequired = !parents.isEmpty();
    255286        }
    … …  
    518549            }
    519550        }
    520551    }
     552
     553    private static final class SimplifyWayDataSelectionListener implements DataSelectionListener {
     554        private final List<Way> wayList;
     555        private Consumer<List<Way>> consumer;
     556
     557        /**
     558         * Create a new selection listener for {@link SimplifyWayAction}
     559         * @param wayList The <i>modifiable</i> list to update on selection changes
     560         */
     561        SimplifyWayDataSelectionListener(List<Way> wayList) {
     562            this.wayList = wayList;
     563        }
     564
     565        @Override
     566        public void selectionChanged(SelectionChangeEvent event) {
     567            updateWayList(event.getSource());
     568        }
     569
     570        void updateWayList(DataSet dataSet) {
     571            final List<Way> newWays = dataSet.getSelectedWays().stream()
     572                    .filter(p -> !p.isIncomplete())
     573                    .collect(Collectors.toList());
     574            this.wayList.clear();
     575            this.wayList.addAll(newWays);
     576            if (this.consumer != null) {
     577                GuiHelper.runInEDT(() -> this.consumer.accept(this.wayList));
     578            }
     579        }
     580
     581        void addConsumer(Consumer<List<Way>> wayConsumer) {
     582            this.consumer = wayConsumer;
     583        }
     584    }
    521585}