RFR: 8248381: Create a daemon thread for MonocleTimer

[Kevin Rushforth]

On Mon, 29 Jun 2020 11:33:24 GMT, Johan Vos <jvos at 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 at b1fe89
>>   [Not completed, task = java.util.concurrent.Executors$RunnableAdapter at 1f85c96
>>   [Wrapped task = com.sun.javafx.tk.quantum.QuantumToolkit$$Lambda$111/0x34563828 at 141859b]]
>>   rejected from java.util.concurrent.ScheduledThreadPoolExecutor at 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