On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman <al...@openjdk.org> wrote:
> This is a test only change to the unit test for the ExecutorService returned > by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll > assume the threads started to execute the tasks do actually execute the task > code. The refresh in JEP 444 changed the implementation to use FutureTask, > and FutureTask checks the interrupt status before it executes the task code. > So some intermittent timeouts of the tests for interrupting invokeAll as > those tests were waiting for the task to complete. > > The main change is that the tests for interrupting invokeAll are changed to > interrupt when the main thread blocks in invokeAll. They are also changed to > check if the task started or not. The tests for interrupting invokeAny > already did this, but these are changed to use the same infrastructure to > avoid having two styles of tests in the same source file. Looks reasonable. Just some question about testing invokeAll/invokeAny when there is only one task. test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java line 578: > 576: try { > 577: > scheduleInterruptAt("java.util.concurrent.ThreadPerTaskExecutor.invokeAny"); > 578: executor.invokeAny(Set.of(task)); Seems strange to test invokeAny when there is only one task? test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java line 835: > 833: try { > 834: > scheduleInterruptAt("java.util.concurrent.ThreadPerTaskExecutor.invokeAll"); > 835: executor.invokeAll(Set.of(task)); same remark for invokeAll? ------------- PR Review: https://git.openjdk.org/jdk/pull/14072#pullrequestreview-1436244152 PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200271736 PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200272685