[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8248381: Create a daemon thread for MonocleTimer
From:       Johan Vos <jvos () openjdk ! java ! net>
Date:       2020-06-29 11:35:41
Message-ID: 0P3rBw0Lkt5oFlgcSWPBaw8JQ_545NxYEinvuPMJAYI=.37ab6b83-9938-418d-a0f6-459fc5a7771b () github ! com
[Download RAW message or body]

The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.

----------------------------------------------------------------------
On Sun, 28 Jun 2020 01:56:26 GMT, John Neffenger \
<github.com+1413266+jgneff@openjdk.org> wrote:

> > > OK, that seems fine then. I'll take a closer look and then finish my review.
> > 
> > Actually, I think you may be right, though. Sorry for replying before looking \
> > into it. I now think the `ScheduledThreadPoolExecutor` should be shut down, but \
> > let me look into it a bit more this afternoon before your final review. Thanks! \
> > The new `ScheduledThreadPoolExecutor` is ~complicated~ flexible! 😄
> 
> 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.

-------------

PR: https://git.openjdk.java.net/jfx/pull/256


[prev in list] [next in list] [prev in thread] [next in thread]