On Tue, 15 Apr 2025 06:36:20 GMT, Alan Bateman <al...@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
