From 8c1d93774c14218bb446ca783a439bfbe37bedd6 Mon Sep 17 00:00:00 2001 From: "taylor.smock" Date: Thu, 25 Jul 2024 20:54:48 +0000 Subject: [PATCH] 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''. git-svn-id: https://josm.openstreetmap.de/svn/trunk@19153 0c6e7542-c601-0410-84e7-c038aed88b3b --- .../handler/LoadAndZoomHandler.java | 78 +++++++++++++++- .../handler/LoadAndZoomHandlerTest.java | 93 ++++++++++++++++--- 2 files changed, 155 insertions(+), 16 deletions(-) diff --git a/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java b/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java index c844b33ebf8..7d4a9f19643 100644 --- a/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java +++ b/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 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 performSearchZoom() throws RequestHandlerErrorException { diff --git a/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java b/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java index 8707da9b5f4..ec550bebae8 100644 --- a/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java +++ b/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,10 +13,14 @@ 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; @@ -23,6 +28,8 @@ 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) { " \n" + " \n" + ""))); + // 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("\n" + + "\n" + + " \n" + + " \n" + + " \n" + + " \n" + + ""))); } /** @@ -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("\n" + - "\n" + - " \n" + - " \n" + - " \n" + - " \n" + - ""))); - 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 #23821 + * @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()); + } }