Index: trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java =================================================================== --- trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java (revision 18819) +++ 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()); + } } Index: trunk/test/unit/org/openstreetmap/josm/testutils/annotations/OsmApi.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/testutils/annotations/OsmApi.java (revision 18821) +++ trunk/test/unit/org/openstreetmap/josm/testutils/annotations/OsmApi.java (revision 18821) @@ -0,0 +1,80 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.testutils.annotations; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.util.Map; + +import org.junit.jupiter.api.extension.AfterEachCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.BeforeEachCallback; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.platform.commons.support.ReflectionSupport; +import org.junit.runners.model.InitializationError; +import org.openstreetmap.josm.io.OsmApiInitializationException; +import org.openstreetmap.josm.io.OsmTransferCanceledException; +import org.openstreetmap.josm.spi.preferences.Config; +import org.openstreetmap.josm.testutils.FakeOsmApi; + +/** + * Used for setting the desired OsmApi type + */ +@BasicPreferences +@ExtendWith(OsmApi.OsmApiExtension.class) +@HTTP +@LayerManager +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface OsmApi { + APIType value() default APIType.NONE; + enum APIType { + /** Don't use any API */ + NONE, + /** Use the {@link org.openstreetmap.josm.testutils.FakeOsmApi} for testing. */ + FAKE, + /** Enable the dev.openstreetmap.org API for this test. */ + DEV + } + + class OsmApiExtension implements BeforeAllCallback, BeforeEachCallback, AfterEachCallback { + @Override + public void afterEach(ExtensionContext context) throws Exception { + Config.getPref().put("osm-server.url", "http://invalid"); + ((Map) ReflectionSupport.tryToReadFieldValue( + org.openstreetmap.josm.io.OsmApi.class.getDeclaredField("instances"), null) + .get()).clear(); + } + + @Override + public void beforeAll(ExtensionContext context) throws Exception { + this.beforeEach(context); + } + + @Override + public void beforeEach(ExtensionContext context) throws Exception { + final APIType useAPI = AnnotationUtils.findFirstParentAnnotation(context, OsmApi.class) + .map(OsmApi::value).orElse(APIType.NONE); + // Set API + if (useAPI == APIType.DEV) { + Config.getPref().put("osm-server.url", "https://api06.dev.openstreetmap.org/api"); + } else if (useAPI == APIType.FAKE) { + FakeOsmApi api = FakeOsmApi.getInstance(); + Config.getPref().put("osm-server.url", api.getServerUrl()); + } else { + Config.getPref().put("osm-server.url", "http://invalid"); + } + + // Initialize API + if (useAPI != APIType.NONE) { + try { + org.openstreetmap.josm.io.OsmApi.getOsmApi().initialize(null); + } catch (OsmTransferCanceledException | OsmApiInitializationException e) { + throw new InitializationError(e); + } + } + } + } +} Index: trunk/test/unit/org/openstreetmap/josm/testutils/annotations/TestUser.java =================================================================== --- trunk/test/unit/org/openstreetmap/josm/testutils/annotations/TestUser.java (revision 18821) +++ trunk/test/unit/org/openstreetmap/josm/testutils/annotations/TestUser.java (revision 18821) @@ -0,0 +1,63 @@ +// License: GPL. For details, see LICENSE file. +package org.openstreetmap.josm.testutils.annotations; + + +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.net.Authenticator; +import java.net.PasswordAuthentication; + +import org.junit.jupiter.api.extension.AfterEachCallback; +import org.junit.jupiter.api.extension.BeforeAllCallback; +import org.junit.jupiter.api.extension.BeforeEachCallback; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.ExtensionContext; +import org.openstreetmap.josm.TestUtils; +import org.openstreetmap.josm.data.UserIdentityManager; +import org.openstreetmap.josm.io.auth.CredentialsManager; +import org.openstreetmap.josm.spi.preferences.Config; +import org.openstreetmap.josm.tools.Utils; + +/** + * Used for tests that require a test user to be defined via -Dosm.username and -Dosm.password. + * This uses the {@link OsmApi.APIType#DEV} server. + */ +@BasicPreferences +@OsmApi(OsmApi.APIType.DEV) +@ExtendWith(TestUser.TestUserExtension.class) +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface TestUser { + class TestUserExtension implements BeforeAllCallback, BeforeEachCallback, AfterEachCallback { + @Override + public void afterEach(ExtensionContext context) throws Exception { + UserIdentityManager.getInstance().setAnonymous(); + CredentialsManager.getInstance().purgeCredentialsCache(Authenticator.RequestorType.SERVER); + } + + @Override + public void beforeAll(ExtensionContext context) throws Exception { + this.beforeEach(context); + } + + @Override + public void beforeEach(ExtensionContext context) throws Exception { + assumeTrue(TestUtils.areCredentialsProvided(), + "OSM DEV API credentials not provided. Please define them with -Dosm.username and -Dosm.password"); + final String username = Utils.getSystemProperty("osm.username"); + final String password = Utils.getSystemProperty("osm.password"); + assumeTrue(username != null && !username.isEmpty(), "Please add -Dosm.username for the OSM DEV API"); + assumeTrue(password != null && !password.isEmpty(), "Please add -Dosm.password for the OSM DEV API"); + 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); + CredentialsManager.getInstance().store(Authenticator.RequestorType.SERVER, org.openstreetmap.josm.io.OsmApi.getOsmApi().getHost(), + new PasswordAuthentication(username, password.toCharArray())); + } + } +}