RFR: 8342486: Implement JEP 505: Structured Concurrency (Fifth Preview) [v8]
Viktor Klang
vklang at openjdk.org
Tue Apr 15 21:51:53 UTC 2025
On Tue, 15 Apr 2025 06:36:20 GMT, Alan Bateman <alanb at openjdk.org> wrote:
>> Changes for [JEP 505: Structured Concurrency (Fifth Preview)](https://openjdk.org/jeps/8340343). The proposal is to re-preview the API with some changes, specifically:
>>
>> - A [StructuredTaskScope](https://download.java.net/java/early_access/loom/docs/api/java.base/java/util/concurrent/StructuredTaskScope.html) is now opened with a static factory method instead of a constructor. Once opened, the API usage is unchanged: fork subtasks individually, join them as a unit, process outcome, and close.
>> - In conjunction with moving to using a static open method, policy and desired outcome is now selected by specifying a Joiner to the open method rather than extending STS. A Joiner handles subtask completion and produces the result for join to return. Joiner.onComplete is the equivalent of overriding handleComplete previously. This change means that the subclasses ShutdownOnFailure and ShutdownOnSuccess are removed, replaced by factory methods on Joiner to get an equivalent Joiner.
>> - The join method is changed to return the result or throw STS.FailedException, replacing the need for an API in subclasses to obtain the outcome. This removes the hazard that was forgetting to call throwIfFailed to propagate exceptions.
>> - Configuration that was provided with parameters for the constructor is changed so that can be provided by a configuration function.
>> - joinUntil is replaced by allowing a timeout be configured by the configuration function. This allows the timeout to apply the scope rather than the join method.
>>
>> The underlying implementation is unchanged except that ThreadFlock.shutdown and wakeup methods are no longer confined. The STS API implementation moves to non-public StructuedTaskScopeImpl because STS is now an interface. A non-public Joiners class is added with the built-in Joiner implementations.
>
> Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
>
> - Add JEP number, update copyright headers
> - Merge branch 'master' into JDK-8342486
> - Sync up from loom repo
> - Merge branch 'master' into JDK-8342486
> - Sync up from loom repo
> - Merge branch 'master' into JDK-8342486
> - Merge branch 'master' into JDK-8342486
> - Fix link
> - Merge branch 'master' into JDK-8342486
> - Sync up impl/tests form loom repo
> - ... and 5 more: https://git.openjdk.org/jdk/compare/3090e218...418bc3d3
Looks great, Alan. I made a couple of suggestions, primarily stylistic, but this PR is good to go as-is.
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 496:
> 494: * processed. It should be clear what the {@code Joiner} does vs. the application
> 495: * code at the use-site. In general, the {@code Joiner} implementation is not the
> 496: * place to code "business logic". A {@code Joiner} should be designed to be as
Suggestion:
* place for "business logic". A {@code Joiner} should be designed to be as
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 563:
> 561: * <p> In normal usage, this method will be called at most once by the {@code join}
> 562: * method to produce the result (or exception). The behavior of this method when
> 563: * invoked directly, and invoked more than once, is not specified. Where possible,
Suggestion:
* invoked directly, and invoked more than once, is undefined. Where possible,
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 778:
> 776: /**
> 777: * {@return a new {@code Configuration} object with the given timeout}
> 778: * The other components are the same as this object.
Suggestion:
* The other other configuration options are preserved.
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 806:
> 804: * @param cause the cause, can be {@code null}
> 805: */
> 806: public FailedException(Throwable cause) {
Should this be public?
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 826:
> 824: * Constructs a {@code TimeoutException} with no detail message.
> 825: */
> 826: public TimeoutException() { }
Should this be public?
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 859:
> 857: *
> 858: * @param joiner the joiner
> 859: * @param configFunction a function to produce the configuration
Suggestion:
* @param configFunction a function to adapt the configuration
src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 948:
> 946: * <p> This method returns the {@link Subtask Subtask} object. In some usages, this
> 947: * object may be used to get its result. In other cases it may be used for correlation
> 948: * or just discarded. To ensure correct usage, the {@link Subtask#get() Subtask.get()}
Suggestion:
* or be discarded. To ensure correct usage, the {@link Subtask#get() Subtask.get()}
src/java.base/share/classes/java/util/concurrent/StructuredTaskScopeImpl.java line 2:
> 1: /*
> 2: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
No changes in 2025?
src/java.base/share/classes/java/util/concurrent/StructuredTaskScopeImpl.java line 424:
> 422: * Used to schedule a task to cancel the scope when a timeout expires.
> 423: */
> 424: private static class TimerSupport {
Might make sense to use the commonPool():s DelayScheduler?
test/jdk/java/util/concurrent/StructuredTaskScope/StructuredTaskScopeTest.java line 977:
> 975: scope.fork(() -> "foo");
> 976: while (!scope.isCancelled()) {
> 977: Thread.sleep(10);
Might make sense to create a utility method to "awaitAndAssertCancel" since this location has a 10ms sleep but others have a 20ms sleep. (To ensure that it is handled uniformly)
test/jdk/java/util/concurrent/StructuredTaskScope/StructuredTaskScopeTest.java line 1394:
> 1392: assertEquals(2, subtasks.size());
> 1393: assertTrue(subtasks.get(0) == subtask1);
> 1394: assertTrue(subtasks.get(1) == subtask2);
Stylistic, there's an "assertSame" to assert that two references refer to the same instance.
-------------
Marked as reviewed by vklang (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21934#pullrequestreview-2768269128
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2044538364
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2044548316
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045538271
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045540091
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045540261
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045541888
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045546116
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045559901
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045572893
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045585721
PR Review Comment: https://git.openjdk.org/jdk/pull/21934#discussion_r2045587907
More information about the core-libs-dev
mailing list