JDK-8309407 : java/util/concurrent/ExecutorService/CloseTest.java failed with "InterruptedException: sleep interrupted"
  • Type: CSR
  • Component: core-libs
  • Sub-Component: java.util.concurrent
  • Priority: P3
  • Status: Finalized
  • Resolution: Unresolved
  • Fix Versions: 22
  • Submitted: 2023-06-03
  • Updated: 2023-10-24
Related Reports
CSR :  
Description
Summary
-------

ForkJoinPool.invokeAll had incompatible Interruptiblity signature and behavior with its ExecutorService specs. This fixes the problem, while also adding an Uninterruptible version of the method for those who still need it.

Problem
-------

Usages of ForkJoinPool.invokeAll, when accessed as an ExecutorService (e.g., via Executors.newWorkStealingPool) had behavior incompatible with specs, causing Interrupts to unexpectedly be ignored in nearly all usages.

Solution
--------

Correct the signature and behavior of invokeAll, but also introduce method  invokeAllUninterruptibly for use by any users who specifically want or need the prior version. Also add two overloads of ForkJoinTask.adaptInterruptible for uniformity.

Specification
-------------

The update invokeAll specs just inherit those of ExecutorService, so are left implicit.

ForkJoinPool.invokeAllUninterruptibly:
   }

    /**
     * Uninterrupible version of {@code InvokeAll}. Executes the given
     * tasks, returning a list of Futures holding their status and
     * results when all complete, ignoring interrupts.  {@link
     * Future#isDone} is {@code true} for each element of the returned
     * list.  Note that a <em>completed</em> task could have
     * terminated either normally or by throwing an exception.  The
     * results of this method are undefined if the given collection is
     * modified while this operation is in progress.
     *
     * @apiNote This method supports usages that previously relied on an
     * incompatible override of
     * {@link ExecutorService#invokeAll(java.util.Collection)}.
     *
     * @param tasks the collection of tasks
     * @param <T> the type of the values returned from the tasks
     * @return a list of Futures representing the tasks, in the same
     *         sequential order as produced by the iterator for the
     *         given task list, each of which has completed
     * @throws NullPointerException if tasks or any of its elements are {@code null}
     * @throws RejectedExecutionException if any task cannot be
     *         scheduled for execution
     * @since 22
     */
    public <T> List<Future<T>> invokeAllUninterruptibly(Collection<? extends Callable<T>> tasks)

The two overloads for ForkJoinTask.adaptInterruptible include the same wording as the existing Callable version:

    /**
     * Returns a new {@code ForkJoinTask} that performs the {@code run}
     * method of the given {@code Runnable} as its action, and returns
     * the given result upon {@link #join}, translating any checked exceptions
     * encountered into {@code RuntimeException}.  Additionally,
     * invocations of {@code cancel} with {@code mayInterruptIfRunning
     * true} will attempt to interrupt the thread performing the task.
     *
     * @param runnable the runnable action
     * @param result the result upon completion
     * @param <T> the type of the result
     * @return the task
     *
     * @since 22
     */
    public static <T> ForkJoinTask<T> adaptInterruptible(Runnable runnable, T result)

    /**
     * Returns a new {@code ForkJoinTask} that performs the {@code
     * run} method of the given {@code Runnable} as its action, and
     * returns null upon {@link #join}, translating any checked
     * exceptions encountered into {@code RuntimeException}.
     * Additionally, invocations of {@code cancel} with {@code
     * mayInterruptIfRunning true} will attempt to interrupt the
     * thread performing the task.
     *
     * @param runnable the runnable action
     * @return the task
     *
     * @since 22
     */
    public static ForkJoinTask<?> adaptInterruptible(Runnable runnable)

Comments
[~darcy] I assume you are asking about the updates to the "Implementation Overview" comment. This isn't visible in the generated javadoc, instead it documents the implementation in the source code. There are 3 new methods proposed in this CSR, they should be clear. The rest of the CSR is about methods specified by ExecuorService (ES) where the implementation in ForkJoinPool (FJP) isn't aligned with the contract or where it differs to other implementations. Specifically: 1. ES..invokeAll is declared to throw InterruptedException if interrupted while waiting. Unfortunately, the override of this method in FJP did not declare the exception, FJP.invokeAll is not interruptible. The changes fixes this but it means a small source incompatible change. If there is code compiled directly to use ForkJoinPool.invokeAll then it will no longer compile. A corpus search found <5 usages, of which one or two may no longer compile. So the impact is low. The proposed invokeAllUninterruptibly is introduced for code that doesn't want to deal with interrupt. 2. ES specifies submit(Callable) and submit(Runnable). If the task fails, Future::get throws ExecutionException. The FJP implementation has historically wrapped the exception thrown by the task in a RuntimeException so Future::get throws ExecutionException with a RuntimeException as the cause. The proposed change aligns FJP with other ES implementations so the cause is the exception that the task failed with. 3. ES specifies submit(Callable) and submit(Runnable), returning a Future representing pending completion of the task. Future.cancel(true) is specified to cancel the task and interrupt the thread if the task is running. FJP tasks don't use interrupt for cancellation so invoking cancel(true) did not interrupt a thread executing a task. The proposed change is that invoking cancel(true) on a Future returned by an ES-specified method will interrupt.
24-10-2023

Moving to Provisional, not Approved. [~alanb] and [~dl], I'm going to need a bit of help decoding this CSR. Before asking this question, I've looked over the PR https://github.com/openjdk/jdk/pull/14301#issuecomment-1576848724 There appear to be large number of javadoc changes there not present in the CSR. Are they all to non-public types? Also, please bit more explicit in terms of what exactly is changing, "X.Y used to do $FOO, now X.Y does $BAR", etc.
24-10-2023

Thanks very much!!
13-10-2023

I sent it back to "Draft" and then "Finalized" it again. Appears to have gotten it back into the same state as before.
13-10-2023

Oh, sorry I think I closed this issue by mistake while checking status. Can someone tell me how to undo this?!
13-10-2023

Unless I'm missing something, I think this is now ready to approve?
12-10-2023

[~alanb], please review this request.
09-10-2023

Hi [~dl], I assume your intention is for this CSR to be Finalized for consideration for JDK 22?
09-10-2023

Can this be approved now?
09-10-2023

Yes, 22. We've been sitting on this for months, and decided that doing it near end of 21 ramp-down would be too disruptive, So the @versions have been or will be updated.
10-08-2023

Moving to Provisional, not Approved. [~dl] and [~alanb], I assume this change is intended for JDK 22 given where we are in the JDK 21 schedule.
10-08-2023

The addition of ForkJoinPool.invokeAllUninterruptibly and 2*ForkJoinTask.adaptInterruptible looks good. The source (and behaviour) incompatible change to the untimed ForkJoinTask.invokeAll to throw InterruptedException is unfortunate but the right thing to do. I agree the impact is very small and the few cases we found can be easily changed to use ForkJoinPool.invokeAllUninterruptibly if needed. The other behavioural changes are: - cancel(true) on a Future to a task submitted with the ES-specified submit(Runnable) and submit(Callable) will interrupt the task. This change means that FJP with be consistent with other ES implementations. - If a task submitted with the ES-specified submit(Runnable) and submit(Callable) fails then Future::get will throw ExecutionException will the exception as the cause. Previous behavior was to wrap the exception so the ExecutionException cause was a RuntimeException .This change means that FJP with be consistent with other ES implementations.
16-06-2023