Fix #23821: Ensure that a new layer is loaded ''prior to'' loading ad…

…ditional 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''.

git-svn-id: https://josm.openstreetmap.de/svn/trunk@19153 0c6e7542-c601-0410-84e7-c038aed88b3b

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

@@ -7,6 +7,7 @@
 import java.awt.geom.Rectangle2D;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -15,6 +16,8 @@
 import java.util.concurrent.Future;
 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;
 
@@ -72,6 +75,8 @@ public class LoadAndZoomHandler extends RequestHandler {
     private static final String CHANGESET_TAGS = "changeset_tags";
     private static final String SEARCH = "search";
 
+    private static final Map<String, ReadWriteLock> layerLockMap = new HashMap<>();
+
     // Mandatory arguments
     private double minlat;
     private double maxlat;
@@ -160,13 +165,19 @@ protected void handleRequest() throws RequestHandlerErrorException {
 
     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)) {
                 if (!PermissionPrefWithDefault.LOAD_DATA.isAllowed()) {
                     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()) {
                         toDownload = removeAlreadyDownloadedArea();
@@ -178,10 +189,58 @@ private void download() throws RequestHandlerErrorException {
                     }
                 }
             }
+        } 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();
+            }
         }
     }
 
@@ -212,7 +271,13 @@ private Area removeAlreadyDownloadedArea() {
         return toDownload;
     }
 
-    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),
                         null /* let the task manage the progress monitor */)));
@@ -244,6 +309,15 @@ private void performDownload(DownloadOsmTask osmTask, DownloadParams settings) {
                 ExceptionDialogUtil.explainException(ex);
             }
         });
+        // 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);
+        }
     }
 
     private Collection<OsmPrimitive> performSearchZoom() throws RequestHandlerErrorException {

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

@@ -4,6 +4,7 @@
 import static org.junit.jupiter.api.Assertions.assertAll;
 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;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -12,17 +13,23 @@
 import java.net.URLEncoder;
 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;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 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;
 
@@ -40,6 +47,7 @@
 @ExtendWith(BasicWiremock.OsmApiExtension.class)
 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();
         req.myCommand = LoadAndZoomHandler.command;
@@ -64,6 +72,15 @@ void setup(WireMockRuntimeInfo wireMockRuntimeInfo) {
                         " <node id=\"2\" " + common + " lat=\"0.0001\" lon=\"0.0001\"/>\n" +
                         " <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>")));
     }
 
     /**
@@ -140,22 +157,12 @@ void testOverlappingArea(WireMockRuntimeInfo wireMockRuntimeInfo) throws Request
         LoadAndZoomHandler handler = newHandler(DEFAULT_BBOX_URL);
         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);
+
+        handler = newHandler(DEFAULT_BBOX_URL_2);
         assertDoesNotThrow(handler::handle);
         syncThreads();
-        wireMockRuntimeInfo.getWireMock().verifyThat(1, RequestPatternBuilder.newRequestPattern().withUrl(mapCall));
+        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);
         assertAll(() -> assertNotNull(ds.getPrimitiveById(1, OsmPrimitiveType.NODE)),
@@ -226,4 +233,62 @@ void testChangesetTags() throws RequestHandlerBadRequestException {
         assertEquals("tag", ds.getChangeSetTags().get("custom"));
         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());
+    }
 }