Changeset 18821 in josm for trunk/src/org/openstreetmap/josm/io

Timestamp:

2023-08-24T17:23:11+02:00 (10 months ago)

Author:

taylor.smock

Message:

Fix #23140: RejectedExecutionException when MultiFetchServerObjectReader is cancelled while creating download jobs

This was caused by a race condition.
User starts a download, and cancels it as jobs are submitted to the thread pool.
The next job to be submitted will cause a RejectedExecutionException. In order
to fix this, we ensure that we shutdown the thread pool inside a synchronized
block, and add jobs to the executor inside a synchronized block.

File:

• trunk/src/org/openstreetmap/josm/io/MultiFetchServerObjectReader.java (modified) (9 diffs)

trunk/src/org/openstreetmap/josm/io/MultiFetchServerObjectReader.java

@@ -48 +48 @@
  * Retrieves a set of {@link OsmPrimitive}s from an OSM server using the so called
  * Multi Fetch API.
- *
+ * <p>
  * Usage:
  * <pre>
@@ -128 +128 @@
      * Remembers an {@link OsmPrimitive}'s id. The id will
      * later be fetched as part of a Multi Get request.
-     *
+     * <p>
      * Ignore the id if it represents a new primitives.
      *
@@ -326 +326 @@
         CompletionService<FetchResult> ecs = new ExecutorCompletionService<>(exec);
         List<Future<FetchResult>> 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 +352 @@
                 }
             } catch (InterruptedException | ExecutionException e) {
+                if (e instanceof InterruptedException) {
+                    Thread.currentThread().interrupt();
+                }
                 Logging.error(e);
                 if (e.getCause() instanceof OsmTransferException)
@@ -369 +376 @@
      * the latest version of the primitive (if any), even if the primitive is not visible (i.e. if
      * visible==false).
-     *
+     * <p>
      * Invoke {@link #getMissingPrimitives()} to get a list of primitives which have not been
      * found on  the server (the server response code was 404)
@@ -589 +596 @@
                     }
                     if (pkg.size() == 1) {
-                        FetchResult res = new FetchResult(new DataSet(), new HashSet<PrimitiveId>());
+                        FetchResult res = new FetchResult(new DataSet(), new HashSet<>());
                         res.missingPrimitives.add(new SimplePrimitiveId(pkg.iterator().next(), type));
                         return res;
@@ -668 +675 @@
          * 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}.
-         *
+         * <p>
          * 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 +689 @@
         protected FetchResult singleGetIdPackage(OsmPrimitiveType type, Set<Long> pkg, ProgressMonitor progressMonitor)
                 throws OsmTransferException {
-            FetchResult result = new FetchResult(new DataSet(), new HashSet<PrimitiveId>());
+            FetchResult result = new FetchResult(new DataSet(), new HashSet<>());
             String baseUrl = OsmApi.getOsmApi().getBaseUrl();
             for (long id : pkg) {
@@ -713 +720 @@
     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();
+            }
+        }
     }
 }