Fix #23995: Avoid deadlock when changing street name of multiple ways

Signed-off-by: Taylor Smock <[email protected]>
JOSM · Nov 4, 2024 · 4f24114 · 4f24114
1 parent 51e028d
commit 4f24114
Show file tree

Hide file tree

Showing 8 changed files with 122 additions and 82 deletions.
diff --git a/.github/workflows/ant-release.yml b/.github/workflows/ant-release.yml
diff --git a/.github/workflows/ant.yml b/.github/workflows/ant.yml
@@ -16,7 +16,9 @@ jobs:
   call-workflow:
     strategy:
       matrix:
-        josm-revision: ["", "r15229"]
-    uses: JOSM/JOSMPluginAction/.github/workflows/ant.yml@v2
+        josm-revision: ["", "r19044"]
+    uses: JOSM/JOSMPluginAction/.github/workflows/ant.yml@v3
     with:
       josm-revision: ${{ matrix.josm-revision }}
+      perform-revision-tagging: ${{ matrix.josm-revision == 'r19044' && github.repository == 'JOSM/highwayNameModification' && github.ref_type == 'branch' && github.ref_name == 'master' && github.event_name != 'schedule' && github.event_name != 'pull_request' }}
+    secrets: inherit
diff --git a/gradle.properties b/gradle.properties
@@ -1,9 +1,9 @@
 # The minimum JOSM version this plugin is compatible with (can be any numeric version)
-plugin.main.version = 15229
+plugin.main.version = 19044
 # The JOSM version this plugin is currently compiled against
 # Please make sure this version is available at https://josm.openstreetmap.de/download
 # The special values "latest" and "tested" are also possible here, but not recommended.
-plugin.compile.version = 15229
+plugin.compile.version = 19044
 plugin.canloadatruntime = true
 plugin.author = Taylor Smock <incoming+gokaart/[email protected]>
 plugin.class = org.openstreetmap.josm.plugins.highwaynamemodification.HighwayNameModification

diff --git a/pom.xml b/pom.xml
@@ -0,0 +1,47 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <modelVersion>4.0.0</modelVersion>
+    <parent>
+        <groupId>org.openstreetmap.josm.plugins</groupId>
+        <artifactId>plugin-root</artifactId>
+        <version>SNAPSHOT</version>
+    </parent>
+    <artifactId>highwayNameModification</artifactId>
+
+    <url>${plugin.link}</url>
+    <developers>
+        <developer>
+            <id>taylor.smock</id>
+            <email>[email protected]</email>
+            <name>Taylor Smock</name>
+        </developer>
+    </developers>
+    <properties>
+        <plugin.src.dir>src/main/java</plugin.src.dir>
+        <plugin.resources.dir>src/main/resources</plugin.resources.dir>
+        <plugin.main.version>19044</plugin.main.version>
+        <plugin.author>Taylor Smock &lt;incoming+gokaart/[email protected]&gt;</plugin.author>
+        <plugin.class>org.openstreetmap.josm.plugins.highwaynamemodification.HighwayNameModification</plugin.class>
+        <plugin.description>Modify addr tags when a highway name is changed</plugin.description>
+        <plugin.icon>images/deltasignmod.svg</plugin.icon>
+        <plugin.link>https://gitlab.com/gokaart/JOSM_highwayNameModification</plugin.link>
+        <plugin.canloadatruntime>true</plugin.canloadatruntime>
+    </properties>
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-jar-plugin</artifactId>
+                <configuration>
+                    <archive>
+                        <manifestEntries>
+                            <Plugin-Link>${plugin.link}</Plugin-Link>
+                            <Plugin-Icon>${plugin.icon}</Plugin-Icon>
+                            <Plugin-Canloadatruntime>${plugin.canloadatruntime}</Plugin-Canloadatruntime>
+                        </manifestEntries>
+                    </archive>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
+</project>
diff --git a/...n/java/org/openstreetmap/josm/plugins/highwaynamemodification/DownloadAdditionalWays.java b/...n/java/org/openstreetmap/josm/plugins/highwaynamemodification/DownloadAdditionalWays.java
@@ -1,6 +1,9 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.highwaynamemodification;
 
+import static java.util.function.Predicate.not;
+
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
@@ -48,6 +51,8 @@ private DownloadAdditionalWays() {
      */
     public static <T extends OsmPrimitive> boolean checkIfDownloaded(Collection<T> ways) {
         boolean rValue = false;
+        final Collection<Layer> layers = new ArrayList<>(MainApplication.getLayerManager().getLayers());
+        downloadedLayerWays.keySet().removeIf(not(layers::contains));
         for (HashMap<String, HashSet<OsmPrimitive>> map : downloadedLayerWays.values()) {
             for (HashSet<OsmPrimitive> set : map.values()) {
                 if (set.containsAll(ways)) {

diff --git a/.../java/org/openstreetmap/josm/plugins/highwaynamemodification/HighwayNameChangeAction.java b/.../java/org/openstreetmap/josm/plugins/highwaynamemodification/HighwayNameChangeAction.java
@@ -42,8 +42,7 @@ public HighwayNameChangeAction(String name, String imageIcon, HighwayNameListene
     public void actionPerformed(ActionEvent e) {
         DataSet ds = MainApplication.getLayerManager().getActiveDataSet();
         Collection<OsmPrimitive> selection = ds.getAllSelected();
-        ModifyWays modifyWays = listener.getModifyWays();
-        modifyWays.setNameChangeInformation(selection, null, true);
+        ModifyWays modifyWays = new ModifyWays(selection, null, true);
         modifyWays.setDownloadTask(true);
         MainApplication.worker.execute(modifyWays);
     }

diff --git a/...main/java/org/openstreetmap/josm/plugins/highwaynamemodification/HighwayNameListener.java b/...main/java/org/openstreetmap/josm/plugins/highwaynamemodification/HighwayNameListener.java
@@ -1,7 +1,12 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.highwaynamemodification;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.event.AbstractDatasetChangedEvent;
@@ -22,7 +27,7 @@
  * @author Taylor Smock
  */
 public class HighwayNameListener implements DataSetListener {
-    private final ModifyWays modifyWays = ModifyWays.getInstance();
+    private static final String[] EMPTY_STRING_ARRAY = new String[0];
 
     @Override
     public void primitivesAdded(PrimitivesAddedEvent event) {
@@ -36,23 +41,16 @@ public void primitivesRemoved(PrimitivesRemovedEvent event) {
 
     @Override
     public void tagsChanged(TagsChangedEvent event) {
-        final OsmPrimitive osm = event.getPrimitive();
-        final Map<String, String> originalKeys = event.getOriginalKeys();
-        if (osm.hasKey("highway") && originalKeys.containsKey("name") && osm.hasKey("name")) {
-            String newName = osm.get("name");
-            String oldName = originalKeys.get("name");
+        final String[] oldNew = getOldNewName(event);
+        if (oldNew.length == 2) {
+            String newName = oldNew[0];
+            String oldName = oldNew[1];
             if (newName.equals(oldName))
                 return;
-            modifyWays.setNameChangeInformation(event.getPrimitives(), oldName);
-            modifyWays.setDownloadTask(true);
-            MainApplication.worker.execute(modifyWays);
+            performTagChanges(oldName, Collections.singleton(event));
         }
     }
 
-    public ModifyWays getModifyWays() {
-        return modifyWays;
-    }
-
     @Override
     public void nodeMoved(NodeMovedEvent event) {
         // Don't care
@@ -78,11 +76,33 @@ public void dataChanged(DataChangedEvent event) {
         // Validation fixes don't call tagsChanged, so we call it for them.
         if (event == null || event.getEvents() == null)
             return;
-        for (AbstractDatasetChangedEvent tEvent : event.getEvents()) {
-            if (DatasetEventType.TAGS_CHANGED == tEvent.getType()) {
-                tagsChanged((TagsChangedEvent) tEvent);
-            }
+        final Map<String, List<TagsChangedEvent>> groupedEvents = event.getEvents().stream()
+                .filter(tEvent -> DatasetEventType.TAGS_CHANGED == tEvent.getType())
+                .map(TagsChangedEvent.class::cast)
+                .collect(Collectors.groupingBy(tEvent -> String.join("----xxxx----", getOldNewName(tEvent))));
+        for (List<TagsChangedEvent> events : groupedEvents.values()) {
+            String oldName = getOldNewName(events.get(0))[0];
+            performTagChanges(oldName, events);
         }
     }
 
+    private void performTagChanges(String oldName, Collection<TagsChangedEvent> events) {
+        final Collection<OsmPrimitive> objects = events.stream().flatMap(event -> event.getPrimitives().stream()).collect(Collectors.toList());
+        final ModifyWays modifyWays = new ModifyWays(objects, oldName);
+        modifyWays.setDownloadTask(true);
+        MainApplication.worker.execute(modifyWays);
+    }
+
+    private String[] getOldNewName(TagsChangedEvent event) {
+        final OsmPrimitive osm = event.getPrimitive();
+        final Map<String, String> originalKeys = event.getOriginalKeys();
+        if (osm.hasKey("highway") && originalKeys.containsKey("name") && osm.hasKey("name")) {
+            String newName = osm.get("name");
+            String oldName = originalKeys.get("name");
+            if (!newName.equals(oldName)) {
+                return new String[] {oldName, newName};
+            }
+        }
+        return EMPTY_STRING_ARRAY;
+    }
 }
diff --git a/src/main/java/org/openstreetmap/josm/plugins/highwaynamemodification/ModifyWays.java b/src/main/java/org/openstreetmap/josm/plugins/highwaynamemodification/ModifyWays.java
@@ -42,45 +42,32 @@ final class ModifyWays implements Runnable {
     String originalName;
     boolean ignoreNewName;
 
-    private static class InstanceHolder {
-        static final ModifyWays INSTANCE = new ModifyWays();
-    }
-
-    private ModifyWays() {
-        // Do nothing
-    }
-
-    public static ModifyWays getInstance() {
-        return InstanceHolder.INSTANCE;
-    }
-
-    public synchronized void setDownloadTask(boolean b) {
-        this.downloadTask = b;
-    }
-
     /**
-     * Set the name change information
+     * Create a new {@link ModifyWays} object
      *
      * @param osmCollection The objects to change names for
      * @param originalName  The original name
      */
-    public void setNameChangeInformation(Collection<? extends OsmPrimitive> osmCollection, String originalName) {
-        setNameChangeInformation(osmCollection, originalName, false);
+    ModifyWays(Collection<? extends OsmPrimitive> osmCollection, String originalName) {
+        this(osmCollection, originalName, false);
     }
-
     /**
-     * Initialize a new ModifyWays method
+     * Create a new {@link ModifyWays} object
      *
      * @param osmCollection    The collection of ways that are changing names
      * @param originalName     The old name of the ways
      * @param ignoreNameChange If true, don't stop if the new name is the same as
      *                         the old name
      */
-    public synchronized void setNameChangeInformation(Collection<? extends OsmPrimitive> osmCollection,
-            String originalName, boolean ignoreNameChange) {
-        wayChangingName = osmCollection;
+    ModifyWays(Collection<? extends OsmPrimitive> osmCollection,
+               String originalName, boolean ignoreNameChange) {
+        this.wayChangingName = osmCollection;
         this.originalName = originalName;
-        ignoreNewName = ignoreNameChange;
+        this.ignoreNewName = ignoreNameChange;
+    }
+
+    public void setDownloadTask(boolean b) {
+        this.downloadTask = b;
     }
 
     private static class DownloadAdditionalAsk implements Runnable {
@@ -124,23 +111,21 @@ public boolean get() throws InterruptedException {
     @Override
     public void run() {
         try {
-            synchronized (this) {
-                if (originalName != null && downloadTask
-                        && !DownloadAdditionalWays.checkIfDownloaded(wayChangingName)) {
-                    DownloadAdditionalAsk ask = new DownloadAdditionalAsk();
-                    GuiHelper.runInEDTAndWait(ask);
-                    if (ask.get()) {
-                        DownloadAdditionalWays.getAdditionalWays(wayChangingName, originalName);
-                    }
+            if (originalName != null && downloadTask
+                    && !DownloadAdditionalWays.checkIfDownloaded(wayChangingName)) {
+                DownloadAdditionalAsk ask = new DownloadAdditionalAsk();
+                GuiHelper.runInEDTAndWait(ask);
+                if (ask.get()) {
+                    DownloadAdditionalWays.getAdditionalWays(wayChangingName, originalName);
                 }
-                for (OsmPrimitive osm : wayChangingName) {
-                    if (originalName != null) {
-                        doRealRun(osm, originalName);
-                    } else {
-                        for (String key : osm.keySet()) {
-                            if (key.contains("name") && !"name".equals(key)) {
-                                doRealRun(osm, osm.get(key));
-                            }
+            }
+            for (OsmPrimitive osm : wayChangingName) {
+                if (originalName != null) {
+                    doRealRun(osm, originalName);
+                } else {
+                    for (String key : osm.keySet()) {
+                        if (key.contains("name") && !"name".equals(key)) {
+                            doRealRun(osm, osm.get(key));
                         }
                     }
                 }