Changeset 18851 in josm for trunk/src


Ignore:
Timestamp:
2023-10-03T17:39:16+02:00 (17 months ago)
Author:
taylor.smock
Message:

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.

File:
1 edited

Legend:

Unmodified
Added
Removed

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

    r18316 r18851  
    1818import java.util.Objects;
    1919import java.util.Set;
     20import java.util.function.Consumer;
     21import java.util.function.Supplier;
    2022import java.util.stream.Collectors;
    2123
    … …  
    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;
    … …  
    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;
    … …  
    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 
    137         String r = s.get(keyRemember, "ask");
     136        return askSimplifyWays(ways, () -> text, null, auto);
     137    }
     138
     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;
    … …  
    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());
    … …  
    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);
    … …  
    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);
    … …  
    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();
    … …  
    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
    … …  
    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();
    … …  
    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}
Note: See TracChangeset for help on using the changeset viewer.