Changeset 19153 in josm for trunk/src

Timestamp:

2024-07-25T22:54:48+02:00 (10 months ago)

Author:

taylor.smock

Message:

Fix #23821: Ensure that a new layer is loaded prior to loading additional data to that layer

This occurred due to a race condition, whereby the /load_and_zoom call would
return immediately prior to the download finishing for the new layer and the
next /load_and_zoom call merging onto a pre-existing layer.

This could be fixed in one of two different ways:

1. Block the RemoteControl thread
2. Have some method for ensuring that a new layer is loaded first

While we are effectively doing (1), it was easier to do (2) as well for testing
purposes. This means the RemoteControl thread could spin off a thread for each
request to /load_and_zoom and this particular issue should not reappear.

This does not control for cases where a user calls /load_and_zoom like so:

1. new_layer=true + layer_name=first
2. new_layer=true + layer_name=second
3. new_layer=false + layer_name=first
4. new_layer=false + layer_name=second

Both (1) and (2) will complete before (3) and (4) are run. However, both (3) and
(4) will be loaded into the last layer loaded.

File:

• trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (modified) (8 diffs)

trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java ¶

@@ -8 +8 @@
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -16 +17 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -73 +76 @@
     private static final String SEARCH = "search";
 
+    private static final Map<String, ReadWriteLock> layerLockMap = new HashMap<>();
+
     // Mandatory arguments
     private double minlat;
@@ -161 +166 @@
     private void download() throws RequestHandlerErrorException {
         DownloadOsmTask osmTask = new DownloadOsmTask();
+        ReadWriteLock lock = null;
+        DownloadParams settings = null;
         try {
-            DownloadParams settings = getDownloadParams();
+            settings = getDownloadParams();
 
             if (command.equals(myCommand)) {
@@ -168 +175 @@
                     Logging.info("RemoteControl: download forbidden by preferences");
                 } else {
+                    // The lock ensures that a download that creates a new layer will finish before
+                    // downloads that do not create a new layer. This should be thread-safe.
+                    lock = obtainLock(settings);
+                    // We need to ensure that we only try to download new areas
                     Area toDownload = null;
                     if (!settings.isNewLayer()) {
@@ -179 +190 @@
                 }
             }
+        } catch (InterruptedException ex) {
+            Thread.currentThread().interrupt();
+            throw new RequestHandlerErrorException(ex);
         } catch (RuntimeException ex) { // NOPMD
             Logging.warn("RemoteControl: Error parsing load_and_zoom remote control request:");
             Logging.error(ex);
             throw new RequestHandlerErrorException(ex);
+        } finally {
+            releaseLock(settings, lock);
+        }
+    }
+
+    /**
+     * Obtain a lock to ensure that a new layer is created before downloading non-new layers
+     * @param settings The settings with the appropriate layer name; if no layer name is given, we assume that
+     *                 the caller doesn't care where the data goes.
+     * @return The lock to pass to {@link #releaseLock(DownloadParams, ReadWriteLock)} or {@code null} if no lock is needed.
+     * @throws InterruptedException If the lock could not be obtained.
+     */
+    private static ReadWriteLock obtainLock(DownloadParams settings) throws InterruptedException {
+        final ReadWriteLock lock;
+        if (settings.isNewLayer() && !Utils.isEmpty(settings.getLayerName())) {
+            synchronized (layerLockMap) {
+                lock = layerLockMap.computeIfAbsent(settings.getLayerName(), k -> new ReentrantReadWriteLock());
+                lock.writeLock().lock();
+            }
+        } else {
+            synchronized (layerLockMap) {
+                lock = layerLockMap.get(settings.getLayerName());
+            }
+            if (lock != null) {
+                lock.readLock().lockInterruptibly();
+            }
+        }
+        return lock;
+    }
+
+    /**
+     * Release the lock preventing data from being downloaded into an old layer
+     * @param settings The settings with information on the new layer status
+     * @param lock The lock to unlock
+     */
+    private static void releaseLock(DownloadParams settings, ReadWriteLock lock) {
+        if (lock != null) {
+            if (settings != null && settings.isNewLayer()) {
+                lock.writeLock().unlock();
+                synchronized (layerLockMap) {
+                    layerLockMap.remove(settings.getLayerName());
+                }
+            } else {
+                lock.readLock().unlock();
+            }
         }
     }
@@ -213 +272 @@
     }
 
-    private void performDownload(DownloadOsmTask osmTask, DownloadParams settings) {
+    /**
+     * Perform the actual download; this is synchronized to ensure that we only have one download going on at a time
+     * @param osmTask The task that will show a dialog
+     * @param settings The download settings
+     * @throws RequestHandlerErrorException If there is an issue getting data
+     */
+    private void performDownload(DownloadOsmTask osmTask, DownloadParams settings) throws RequestHandlerErrorException {
         Future<?> future = MainApplication.worker.submit(
                 new PostDownloadHandler(osmTask, osmTask.download(settings, new Bounds(minlat, minlon, maxlat, maxlon),
@@ -245 +310 @@
             }
         });
+        // Don't block forever, but do wait some period of time.
+        try {
+            future.get(OSM_DOWNLOAD_TIMEOUT.get(), TimeUnit.SECONDS);
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
+            throw new RequestHandlerErrorException(e);
+        } catch (TimeoutException | ExecutionException e) {
+            throw new RequestHandlerErrorException(e);
+        }
     }