* Usage: *
@@ -128,5 +128,5 @@
* Remembers an {@link OsmPrimitive}'s id. The id will
* later be fetched as part of a Multi Get request.
- *
+ *
* Ignore the id if it represents a new primitives.
*
@@ -326,6 +326,10 @@
CompletionService ecs = new ExecutorCompletionService<>(exec);
List> jobs = new ArrayList<>();
- while (!toFetch.isEmpty() && !isCanceled()) {
- jobs.add(ecs.submit(new Fetcher(type, extractIdPackage(toFetch), progressMonitor)));
+ // There exists a race condition where this is cancelled after isCanceled is called, such that
+ // the exec ThreadPool has been shut down. This can cause a RejectedExecutionException.
+ synchronized (this) {
+ while (!toFetch.isEmpty() && !isCanceled()) {
+ jobs.add(ecs.submit(new Fetcher(type, extractIdPackage(toFetch), progressMonitor)));
+ }
}
// Run the fetchers
@@ -348,4 +352,7 @@
}
} catch (InterruptedException | ExecutionException e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
Logging.error(e);
if (e.getCause() instanceof OsmTransferException)
@@ -369,5 +376,5 @@
* the latest version of the primitive (if any), even if the primitive is not visible (i.e. if
* visible==false).
- *
+ *
* Invoke {@link #getMissingPrimitives()} to get a list of primitives which have not been
* found on the server (the server response code was 404)
@@ -589,5 +596,5 @@
}
if (pkg.size() == 1) {
- FetchResult res = new FetchResult(new DataSet(), new HashSet());
+ FetchResult res = new FetchResult(new DataSet(), new HashSet<>());
res.missingPrimitives.add(new SimplePrimitiveId(pkg.iterator().next(), type));
return res;
@@ -668,5 +675,5 @@
* invokes a sequence of Multi Gets for individual ids in a set of ids and a given {@link OsmPrimitiveType}.
* The retrieved primitives are merged to {@link #outputDataSet}.
- *
+ *
* This method is used if one of the ids in pkg doesn't exist (the server replies with return code 404).
* If the set is fetched with this method it is possible to find out which of the ids doesn't exist.
@@ -682,5 +689,5 @@
protected FetchResult singleGetIdPackage(OsmPrimitiveType type, Set pkg, ProgressMonitor progressMonitor)
throws OsmTransferException {
- FetchResult result = new FetchResult(new DataSet(), new HashSet());
+ FetchResult result = new FetchResult(new DataSet(), new HashSet<>());
String baseUrl = OsmApi.getOsmApi().getBaseUrl();
for (long id : pkg) {
@@ -713,6 +720,11 @@
public void cancel() {
super.cancel();
- if (exec != null)
- exec.shutdownNow();
+ // Synchronized to avoid a RejectedExecutionException in fetchPrimitives
+ // We don't want to synchronize on the super.cancel() call.
+ synchronized (this) {
+ if (exec != null) {
+ exec.shutdownNow();
+ }
+ }
}
}
Index: trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java
===================================================================
--- trunk/test/functional/org/openstreetmap/josm/io/MultiFetchServerObjectReaderTest.java (revision 18820)
+++ 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()));
+ }
+ }
+}