Index: trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java =================================================================== --- trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java (revision 18723) +++ trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java (revision 18821) @@ -5,6 +5,6 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.io.File; @@ -23,14 +23,19 @@ import java.util.Random; import java.util.TreeSet; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.logging.Logger; +import org.awaitility.Awaitility; +import org.awaitility.Durations; +import org.awaitility.core.ConditionTimeoutException; +import org.hamcrest.Matchers; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; -import org.junit.jupiter.api.extension.RegisterExtension; -import org.openstreetmap.josm.JOSMFixture; -import org.openstreetmap.josm.TestUtils; import org.openstreetmap.josm.data.coor.LatLon; import org.openstreetmap.josm.data.osm.Changeset; @@ -43,6 +48,7 @@ import org.openstreetmap.josm.data.osm.Way; import org.openstreetmap.josm.gui.progress.NullProgressMonitor; -import org.openstreetmap.josm.spi.preferences.Config; -import org.openstreetmap.josm.testutils.JOSMTestRules; +import org.openstreetmap.josm.gui.util.GuiHelper; +import org.openstreetmap.josm.testutils.annotations.TestUser; +import org.openstreetmap.josm.tools.Logging; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -53,13 +59,8 @@ @SuppressFBWarnings(value = "CRLF_INJECTION_LOGS") @Timeout(value = 1, unit = TimeUnit.MINUTES) +@org.openstreetmap.josm.testutils.annotations.OsmApi(org.openstreetmap.josm.testutils.annotations.OsmApi.APIType.DEV) +@TestUser class MultiFetchServerObjectReaderTest { private static final Logger logger = Logger.getLogger(MultiFetchServerObjectReader.class.getName()); - - /** - * Setup test. - */ - @RegisterExtension - @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD") - public JOSMTestRules test = new JOSMTestRules().preferences(); /** @@ -159,15 +160,5 @@ @BeforeAll public static void init() throws Exception { - if (!TestUtils.areCredentialsProvided()) { - logger.severe("OSM DEV API credentials not provided. Please define them with -Dosm.username and -Dosm.password"); - return; - } logger.info("initializing ..."); - JOSMFixture.createFunctionalTestFixture().init(); - - Config.getPref().put("osm-server.auth-method", "basic"); - - // don't use atomic upload, the test API server can't cope with large diff uploads - Config.getPref().putBoolean("osm-server.atomic-upload", false); File dataSetCacheOutputFile = new File(System.getProperty("java.io.tmpdir"), @@ -213,7 +204,4 @@ @BeforeEach public void setUp() throws IOException, IllegalDataException, FileNotFoundException { - if (!TestUtils.areCredentialsProvided()) { - return; - } File f = new File(System.getProperty("java.io.tmpdir"), MultiFetchServerObjectReaderTest.class.getName() + ".dataset"); logger.info(MessageFormat.format("reading cached dataset ''{0}''", f.toString())); @@ -230,5 +218,4 @@ @Test void testMultiGet10Nodes() throws OsmTransferException { - assumeTrue(TestUtils.areCredentialsProvided()); MultiFetchServerObjectReader reader = new MultiFetchServerObjectReader(); ArrayList nodes = new ArrayList<>(ds.getNodes()); @@ -252,5 +239,4 @@ @Test void testMultiGet10Ways() throws OsmTransferException { - assumeTrue(TestUtils.areCredentialsProvided()); MultiFetchServerObjectReader reader = new MultiFetchServerObjectReader(); ArrayList ways = new ArrayList<>(ds.getWays()); @@ -275,5 +261,4 @@ @Test void testMultiGet10Relations() throws OsmTransferException { - assumeTrue(TestUtils.areCredentialsProvided()); MultiFetchServerObjectReader reader = new MultiFetchServerObjectReader(); ArrayList relations = new ArrayList<>(ds.getRelations()); @@ -298,5 +283,4 @@ @Test void testMultiGet800Nodes() throws OsmTransferException { - assumeTrue(TestUtils.areCredentialsProvided()); MultiFetchServerObjectReader reader = new MultiFetchServerObjectReader(); ArrayList nodes = new ArrayList<>(ds.getNodes()); @@ -320,5 +304,4 @@ @Test void testMultiGetWithNonExistingNode() throws OsmTransferException { - assumeTrue(TestUtils.areCredentialsProvided()); MultiFetchServerObjectReader reader = new MultiFetchServerObjectReader(); ArrayList nodes = new ArrayList<>(ds.getNodes()); @@ -349,3 +332,58 @@ assertEquals("ways?ways=123,126,130", requestString); } + + /** + * This is a non-regression test for #23140: Cancelling `MultiFetchServerObjectReader` while it is adding jobs + * to the executor causes a {@link RejectedExecutionException}. + * This was caused by a race condition between {@link MultiFetchServerObjectReader#cancel()} and queuing download + * jobs. + */ + @Test + void testCancelDuringJobAdd() { + final AtomicBoolean parsedData = new AtomicBoolean(); + final AtomicBoolean continueAddition = new AtomicBoolean(); + final AtomicInteger callCounter = new AtomicInteger(); + final AtomicReference thrownFailure = new AtomicReference<>(); + // We have 5 + 10 maximum (5 previous calls, 10 calls when iterating through the nodes). + final int expectedCancelCalls = 5; + final MultiFetchServerObjectReader reader = new MultiFetchServerObjectReader() { + @Override + public boolean isCanceled() { + final boolean result = super.isCanceled(); + // There are some calls prior to the location where we are interested + if (callCounter.incrementAndGet() >= expectedCancelCalls) { + // This will throw a ConditionTimeoutException. + // By blocking here until cancel() is called, we block cancel (since we are interested in a loop). + Awaitility.await().timeout(Durations.FIVE_HUNDRED_MILLISECONDS).untilTrue(continueAddition); + } + return result; + } + }; + ArrayList nodes = new ArrayList<>(ds.getNodes()); + for (int i = 0; i < 10; i++) { + reader.append(nodes.get(i)); + } + GuiHelper.runInEDT(() -> { + try { + reader.parseOsm(NullProgressMonitor.INSTANCE); + } catch (ConditionTimeoutException timeoutException) { + // This is expected due to the synchronization, so we just swallow it. + Logging.trace(timeoutException); + } catch (Exception failure) { + thrownFailure.set(failure); + } finally { + parsedData.set(true); + } + }); + // cancel, then continue + Awaitility.await().untilAtomic(callCounter, Matchers.greaterThanOrEqualTo(expectedCancelCalls)); + reader.cancel(); + continueAddition.set(true); + Awaitility.await().untilTrue(parsedData); + if (thrownFailure.get() != null) { + Logging.error(thrownFailure.get()); + } + assertNull(thrownFailure.get()); + assertEquals(expectedCancelCalls, callCounter.get()); + } }