Changeset 19153 in josm for trunk/test/unit

Timestamp:

2024-07-25T22:54:48+02:00 (6 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/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (modified) (7 diffs)

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());
+    }
 }