On Mon, 29 Jun 2020 11:33:24 GMT, Johan Vos <j...@openjdk.org> wrote:
>> I think the code in the `_stop` method is correct after all. >> >> The `MonocleTimer` class is written to allow for multiple calls to the pair >> of `_start` and `_stop` methods (even >> though I don't think that ever happens), and the static >> `ScheduledThreadPoolExecutor`, named `scheduler`, is created >> only once and reused on subsequent calls. Changing the `_stop` method to >> call `task.cancel(true)` still leaves the >> timer thread running, which prevents the JavaFX application from exiting >> when the timer thread is a user thread. >> Furthermore, whether it's a user or daemon thread, if the call to >> `task.cancel(true)` happens to run exactly when the >> periodic task is *in progress*, the `timerRunnable` lambda in >> `QuantumToolkit` prints the stack trace when it catches >> the `InterruptedException`. java.lang.InterruptedException: >> at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit >> .lambda$runToolkit$12(QuantumToolkit.java:345) >> at java.base/java.util.concurrent.Executors$RunnableAdapter >> .call(Executors.java:515) >> at java.base/java.util.concurrent.FutureTask >> .runAndReset(FutureTask.java:305) >> at >> java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask >> .run(ScheduledThreadPoolExecutor.java:305) >> at java.base/java.util.concurrent.ThreadPoolExecutor >> .runWorker(ThreadPoolExecutor.java:1130) >> at java.base/java.util.concurrent.ThreadPoolExecutor$Worker >> .run(ThreadPoolExecutor.java:630) >> at java.base/java.lang.Thread >> .run(Thread.java:832) >> >> So the call to `task.cancel(false)` is correct. >> >> Changing the `_stop` method to shut down the `scheduler` will terminate the >> associated thread, regardless of its daemon >> status, but a subsequent call to `_start` will throw a >> `RejectedExecutionException` when trying to schedule the timer >> task: java.util.concurrent.RejectedExecutionException: >> Task >> java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask@b1fe89 >> [Not completed, task = >> java.util.concurrent.Executors$RunnableAdapter@1f85c96 >> [Wrapped task = >> com.sun.javafx.tk.quantum.QuantumToolkit$$Lambda$111/0x34563828@141859b]] >> rejected from java.util.concurrent.ScheduledThreadPoolExecutor@55f462 >> [Terminated, pool size = 0, active threads = 0, queued tasks = 0, >> completed tasks = 0] >> at java.base/java.util.concurrent.ThreadPoolExecutor$AbortPolicy >> .rejectedExecution(ThreadPoolExecutor.java:2057) >> at java.base/java.util.concurrent.ThreadPoolExecutor >> .reject(ThreadPoolExecutor.java:827) >> at java.base/java.util.concurrent.ScheduledThreadPoolExecutor >> .delayedExecute(ScheduledThreadPoolExecutor.java:340) >> at java.base/java.util.concurrent.ScheduledThreadPoolExecutor >> .scheduleAtFixedRate(ScheduledThreadPoolExecutor.java:632) >> at javafx.graphics/com.sun.glass.ui.monocle.MonocleTimer >> ._start(MonocleTimer.java:64) >> at javafx.graphics/com.sun.glass.ui.Timer >> .start(Timer.java:96) >> at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit >> .runToolkit(QuantumToolkit.java:384) >> at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit >> .lambda$startup$10(QuantumToolkit.java:280) >> at javafx.graphics/com.sun.glass.ui.Application >> .lambda$run$1(Application.java:153) >> at javafx.graphics/com.sun.glass.ui.monocle.RunnableProcessor >> .runLoop(RunnableProcessor.java:92) >> at javafx.graphics/com.sun.glass.ui.monocle.RunnableProcessor >> .run(RunnableProcessor.java:51) >> at java.base/java.lang.Thread >> .run(Thread.java:832) >> >> So if we want `MonocleTimer` to reuse a single `ScheduledThreadPoolExecutor` >> object, I think the only way to make sure >> that its timer thread exits when the application exits is to set its daemon >> status to `true`. > > While the PR should indeed fix the original issue, I'm unsure about the > behavior of multiple invocations of start/stop > rather than using the (nop) pause method. However, it seems this behavior is > similar on other platforms, so I assume it > is by design. Given that this is a regression introduced in JavaFX 14, this fix seems like a good candidate for JavaFX 15, so I recommend to _not_ merge the master branch. Go ahead and retarget your PR to the `jfx15` branch (you should not need to merge anything), although we will need to satisfy ourselves that the risk of further regression is low. ------------- PR: https://git.openjdk.java.net/jfx/pull/256
