Index: /trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java =================================================================== --- /trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (revision 19152) +++ /trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (revision 19153) @@ -8,4 +8,5 @@ import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; @@ -16,4 +17,6 @@ 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,4 +76,6 @@ private static final String SEARCH = "search"; + private static final Map layerLockMap = new HashMap<>(); + // Mandatory arguments private double minlat; @@ -161,6 +166,8 @@ 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,4 +175,8 @@ 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,8 +190,56 @@ } } + } 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,5 +272,11 @@ } - 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,4 +310,13 @@ } }); + // 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); + } } Index: /trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (revision 19152) +++ /trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandlerTest.java (revision 19153) @@ -5,4 +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,8 +14,12 @@ 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,4 +29,6 @@ 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,4 +48,5 @@ 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,4 +73,13 @@ " \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" + + ""))); } @@ -141,20 +158,10 @@ 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); - 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,3 +234,61 @@ 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()); + } }