Changeset 19153 in josm for trunk

Timestamp:

2024-07-25T22:54:48+02:00 (4 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.

Location:

trunk

Files:

• src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (modified) (8 diffs)
• test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (modified) (7 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);
+        }
     }
 

trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java

@@ -5 +5 @@
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -13 +14 @@
 import java.nio.charset.StandardCharsets;
 import java.util.Collection;
+import java.util.concurrent.ForkJoinPool;
+import java.util.concurrent.ForkJoinTask;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import com.github.tomakehurst.wiremock.client.WireMock;
 import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
 import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
+import org.awaitility.Awaitility;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -24 +29 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
 import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
+import org.openstreetmap.josm.gui.util.GuiHelper;
 import org.openstreetmap.josm.io.remotecontrol.handler.RequestHandler.RequestHandlerBadRequestException;
 import org.openstreetmap.josm.testutils.annotations.BasicPreferences;
@@ -41 +48 @@
 class LoadAndZoomHandlerTest {
     private static final String DEFAULT_BBOX_URL = "https://localhost/load_and_zoom?left=0&bottom=0&right=0.001&top=0.001";
+    private static final String DEFAULT_BBOX_URL_2 = "https://localhost/load_and_zoom?left=0.00025&bottom=0.00025&right=0.00075&top=0.00125";
     private static LoadAndZoomHandler newHandler(String url) throws RequestHandlerBadRequestException {
         LoadAndZoomHandler req = new LoadAndZoomHandler();
@@ -65 +73 @@
                         " <node id=\"3\" " + common + " lat=\"0.0002\" lon=\"0.0002\"/>\n" +
                         "</osm>")));
+        // The scientific notation is ok server-side.
+        wireMockRuntimeInfo.getWireMock().register(WireMock.get("/api/0.6/map?bbox=2.5E-4,0.001,7.5E-4,0.00125")
+                .willReturn(WireMock.aResponse().withBody("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
+                        "<osm version=\"0.6\" generator=\"hand\" copyright=\"JOSM\" attribution=\"\" license=\"\">\n" +
+                        " <bounds minlat=\"0.001\" minlon=\"0.00025\" maxlat=\"0.00125\" maxlon=\"0.00075\"/>\n" +
+                        " <node id=\"4\" " + common + " lat=\"0.00111\" lon=\"0.00026\"/>\n" +
+                        " <node id=\"5\" " + common + " lat=\"0.0011\" lon=\"0.00025\"/>\n" +
+                        " <node id=\"6\" " + common + " lat=\"0.0012\" lon=\"0.000251\"/>\n" +
+                        "</osm>")));
     }
 
@@ -141 +158 @@
         assertDoesNotThrow(handler::handle);
         syncThreads();
-        // The scientific notation is ok server-side.
-        final String mapCall = "/api/0.6/map?bbox=2.5E-4,0.001,7.5E-4,0.00125";
-        final String commonNode = "visible=\"true\" version=\"1\" changeset=\"1\" timestamp=\"2000-01-01T00:00:00Z\" user=\"tsmock\" uid=\"1\"";
-        wireMockRuntimeInfo.getWireMock().register(WireMock.get(mapCall)
-                .willReturn(WireMock.aResponse().withBody("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
-                        "<osm version=\"0.6\" generator=\"hand\" copyright=\"JOSM\" attribution=\"\" license=\"\">\n" +
-                        " <bounds minlat=\"0.001\" minlon=\"0.00025\" maxlat=\"0.00125\" maxlon=\"0.00075\"/>\n" +
-                        " <node id=\"4\" " + commonNode + " lat=\"0.00111\" lon=\"0.00026\"/>\n" +
-                        " <node id=\"5\" " + commonNode + " lat=\"0.0011\" lon=\"0.00025\"/>\n" +
-                        " <node id=\"6\" " + commonNode + " lat=\"0.0012\" lon=\"0.000251\"/>\n" +
-                        "</osm>")));
-        String request = "https://localhost/load_and_zoom?left=0.00025&bottom=0.00025&right=0.00075&top=0.00125";
-        handler = newHandler(request);
-        assertDoesNotThrow(handler::handle);
-        syncThreads();
-        wireMockRuntimeInfo.getWireMock().verifyThat(1, RequestPatternBuilder.newRequestPattern().withUrl(mapCall));
+
+        handler = newHandler(DEFAULT_BBOX_URL_2);
+        assertDoesNotThrow(handler::handle);
+        syncThreads();
+        wireMockRuntimeInfo.getWireMock().verifyThat(1, RequestPatternBuilder.newRequestPattern()
+                .withUrl("/api/0.6/map?bbox=2.5E-4,0.001,7.5E-4,0.00125"));
         final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
         assertNotNull(ds);
@@ -227 +234 @@
         assertEquals("here", ds.getChangeSetTags().get("is"));
     }
+
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/23821">#23821</a>
+     * @throws RequestHandlerBadRequestException If there is an issue with the handler
+     */
+    @Test
+    void testNonRegression23821() throws RequestHandlerBadRequestException {
+        final AtomicBoolean block = new AtomicBoolean(false);
+        final Runnable runnable = () -> {
+            synchronized (block) {
+                while (!block.get()) {
+                    try {
+                        block.wait();
+                    } catch (InterruptedException e) {
+                        Thread.currentThread().interrupt();;
+                        throw new RuntimeException(e);
+                    }
+                }
+            }
+        };
+        final DataSet wrongDataset = new DataSet();
+        MainApplication.getLayerManager().addLayer(new OsmDataLayer(wrongDataset,
+                "LoadAndZoomHandlerTest#testNonRegression23821", null));
+        ForkJoinTask<?> task1;
+        ForkJoinTask<?> task2;
+        try {
+            GuiHelper.runInEDT(runnable);
+            MainApplication.worker.submit(runnable);
+            // The processor makes a new handler for each request
+            // It is single-threaded, so blocking on one handler would fix the problem with the other handler.
+            // But we might as well work on multi-threading, since it is easier to test. :)
+            final LoadAndZoomHandler handler1 = newHandler(DEFAULT_BBOX_URL + "&new_layer=true&layer_name=OSMData");
+            final LoadAndZoomHandler handler2 = newHandler(DEFAULT_BBOX_URL_2 + "&new_layer=false&layer_name=OSMData");
+            // Use a separate threads to avoid blocking on this thread
+            final ForkJoinPool pool = ForkJoinPool.commonPool();
+            task1 = pool.submit(() -> assertDoesNotThrow(handler1::handle));
+
+            // Make certain there is enough time for the first task to block
+            Awaitility.await().until(() -> true);
+            task2 = pool.submit(() -> assertDoesNotThrow(handler2::handle));
+        } finally {
+            // Unblock UI/worker threads
+            synchronized (block) {
+                block.set(true);
+                block.notifyAll();
+            }
+        }
+
+        task1.join();
+        task2.join();
+
+        syncThreads();
+        assertEquals(2, MainApplication.getLayerManager().getLayers().size());
+        final DataSet ds = MainApplication.getLayerManager().getEditDataSet();
+        assertNotEquals(wrongDataset, ds);
+        assertTrue(wrongDataset.isEmpty());
+        assertEquals(6, ds.allPrimitives().size());
+    }
 }