8390870: ForkJoinTask.get(long, TimeUnit) stuck in busy-wait with another waiter - #32485
8390870: ForkJoinTask.get(long, TimeUnit) stuck in busy-wait with another waiter#32485fmeum wants to merge 3 commits into
Conversation
|
👋 Welcome back fmeum! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
a71fba8 to
7b9a0f1
Compare
| /* | ||
| * @test | ||
| * @bug 8390870 | ||
| * @summary ForkJoinTask.get must honor its timeout and interrupts even | ||
| * when another thread is waiting on the same task. | ||
| */ | ||
|
|
||
| import java.util.concurrent.ForkJoinTask; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.TimeoutException; | ||
| import java.time.Duration; | ||
|
|
||
| public class GetMultipleWaiters { | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| // get() with timeout must time out | ||
| test(false); | ||
| // get() must be interruptible | ||
| test(true); | ||
| } | ||
|
|
||
| static void test(boolean interrupt) throws Exception { | ||
| ForkJoinTask<?> task = ForkJoinTask.adapt(() -> {}); | ||
| Throwable[] thrown = {null}; | ||
|
|
||
| Thread a = new Thread(() -> { | ||
| try { | ||
| if (interrupt) | ||
| task.get(); | ||
| else | ||
| task.get(1, TimeUnit.SECONDS); | ||
| } catch (Throwable t) { | ||
| thrown[0] = t; | ||
| } | ||
| }, "waiter-A"); | ||
| Thread b = new Thread(() -> { | ||
| try { | ||
| task.get(); | ||
| } catch (Throwable ignore) {} | ||
| }, "waiter-B"); | ||
| // Do not rely on a timeout to terminate the test if it fails | ||
| a.setDaemon(true); | ||
| b.setDaemon(true); | ||
|
|
||
| a.start(); | ||
| while (a.getState() != (interrupt ? Thread.State.WAITING : Thread.State.TIMED_WAITING)) | ||
| Thread.sleep(10); | ||
| b.start(); | ||
| while (b.getState() != Thread.State.WAITING) | ||
| Thread.sleep(10); | ||
| if (interrupt) | ||
| a.interrupt(); | ||
|
|
||
| if (!a.join(Duration.ofSeconds(10))) { | ||
| System.out.println("waiter-A state=" + a.getState()); | ||
| for (StackTraceElement e : a.getStackTrace()) | ||
| System.out.println(" at " + e); | ||
| throw new RuntimeException("get() did not return (interrupt=" + interrupt + ")"); | ||
| } | ||
| Class<?> expected = interrupt ? InterruptedException.class : TimeoutException.class; | ||
| if (!expected.isInstance(thrown[0])) | ||
| throw new RuntimeException("expected " + expected.getSimpleName() + ", got " + thrown[0]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks for reporting and submitting a potential fix for this issue, @fmeum!
I'd prefer if this was a junit regression test, that the different permutations were separated (to make it easier to comprehend), and that the test cleans up after itself (rather than using daemonicity).
Is this something you'd like to take a stab at, or do you want me to propose how the regression test could look as described above?
There was a problem hiding this comment.
I made the three changes you requested, happy to change this further. I also shortened the sleep durations as I bet the state would usually progress faster.
Webrevs
|
| while (a.getState() != Thread.State.TIMED_WAITING) | ||
| Thread.sleep(1); |
There was a problem hiding this comment.
I'd create a static method for this, something like:
static void startAndAwaitState(Thread t, Thread.State state) throws Exception {
t.start();
while(t.getState() != state)
Thread.sleep(1);
}That would reduce the number of places where sleep adjustments would need to get made.
You could even go as far as:
static Thread startThreadAndAwaitState(Runnable r, String name, Thread.State state) throws Exception {
var t = new Thread(r, name);
t.start();
while(t.getState() != state)
Thread.sleep(1);
return t;
}There was a problem hiding this comment.
Done. Do you want me to move the Thread#start calls into the try-finally block or is that overly cautious?
|
|
||
| assertInstanceOf(InterruptedException.class, thrown[0]); | ||
| } finally { | ||
| task.cancel(false); |
There was a problem hiding this comment.
If we make the task complete instead of cancelling it, then task.get() should all return normally and the threads should cleanly exit (making .join() succeed).
| task.cancel(false); | |
| task.complete(null); |
| assertInstanceOf(TimeoutException.class, thrown[0]); | ||
| } finally { | ||
| task.cancel(false); | ||
| a.join(); |
There was a problem hiding this comment.
a.join() is likely not needed here—either it already succeeded, or it was Interrupted, in which case it might get interrupted again anyway.
| assertInstanceOf(InterruptedException.class, thrown[0]); | ||
| } finally { | ||
| task.cancel(false); | ||
| a.join(); |
There was a problem hiding this comment.
Not needed (see comment on line 105)
| } catch (Throwable t) { | ||
| thrown[0] = t; | ||
| } | ||
| }, "waiter-A", Thread.State.WAITING); |
There was a problem hiding this comment.
How about we give all the waiter-threads unique names, "timed-waiter-A" for the timed get, and "untimed-waiter-A". That would make it easier to locate any issues.
2 participants
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/32485/head:pull/32485$ git checkout pull/32485Update a local copy of the PR:
$ git checkout pull/32485$ git pull https://git.openjdk.org/jdk.git pull/32485/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 32485View PR using the GUI difftool:
$ git pr show -t 32485Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/32485.diff
Using Webrev
Link to Webrev Comment