Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Mon, 29 Jun 2020 11:33:24 GMT, Johan Vos 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
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
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 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
Re: RFR: 8248381: Create a daemon thread for MonocleTimer [v2]
> Fixes [JDK-8248381](https://bugs.openjdk.java.net/browse/JDK-8248381). John Neffenger has updated the pull request incrementally with one additional commit since the last revision: Remove POOL_SIZE constant - Changes: - all: https://git.openjdk.java.net/jfx/pull/256/files - new: https://git.openjdk.java.net/jfx/pull/256/files/c771c242..4ca2bc03 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/256/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/256/webrev.00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/256.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/256/head:pull/256 PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
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 Sat, 27 Jun 2020 19:40:53 GMT, John Neffenger wrote: >> OK, that seems fine then. I'll take a closer look and then finish my review. > >> 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`. - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Sat, 27 Jun 2020 19:37:01 GMT, Kevin Rushforth 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! - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Sat, 27 Jun 2020 16:25:24 GMT, John Neffenger
wrote:
>> On second thought, this seems more like a workaround than a fix. Maybe it
>> would be better to shutdown the timer to shut
>> it down in an orderly fashion with the FX runtime is terminated. The
>> `QuantumToolkit::exit` method already calls
>> `PulseTimer::stop` so that seems a likely place to stop the timer.
>
>> On second thought, this seems more like a workaround than a fix.
>
> This change just restores the thread daemon status to the way it was before
> in JavaFX 14, where `true` is passed to the
> `Timer` constructor for "isDaemon - true if the associated thread should run
> as a daemon."
> if (timer == null) {
> timer = new java.util.Timer(true);
> }
>
> I tried setting `task.cancel(true)` instead of `false` in the `_stop` method,
> but that didn't stop the non-daemon
> thread.
OK, that seems fine then. I'll take a closer look and then finish my review.
-
PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
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 Sat, 27 Jun 2020 16:14:36 GMT, Kevin Rushforth wrote:
> On second thought, this seems more like a workaround than a fix.
This change just restores the thread daemon status to the way it was before in
JavaFX 14, where `true` is passed to the
`Timer` constructor for "isDaemon - true if the associated thread should run as
a daemon."
if (timer == null) {
timer = new java.util.Timer(true);
}
I tried setting `task.cancel(true)` instead of `false` in the `_stop` method,
but that didn't stop the non-daemon
thread.
-
PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Sat, 27 Jun 2020 14:34:09 GMT, Johan Vos wrote: >> Fixes [JDK-8248381](https://bugs.openjdk.java.net/browse/JDK-8248381). > > modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MonocleTimer.java > line 38: > >> 37: private static final String THREAD_NAME = "Monocle Timer"; >> 38: private static final int POOL_SIZE = 1; >> 39: > > I don't think there is a usecase where POOL_SIZE would be different from 1, > so this could be skipped? Thanks, Johan. I'll remove it. By the way, I added the thread name constant instead of calling `getClass().getSimpleName()` just so the name would appear similar to the other threads in the system, shown below.  - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Sat, 27 Jun 2020 14:10:21 GMT, Kevin Rushforth wrote: >> Note that this is a regression error that is not present in JavaFX 14, so it >> would be good to fix it before JavaFX 15 >> is released. > > The regression was caused by the fix for > [JDK-8176499](https://bugs.openjdk.java.net/browse/JDK-8176499). See PR #117. On second thought, this seems more like a workaround than a fix. Maybe it would be better to shutdown the timer to shut it down in an orderly fashion with the FX runtime is terminated. The `QuantumToolkit::exit` method already calls `PulseTimer::stop` so that seems a likely place to stop the timer. - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Fri, 26 Jun 2020 03:50:02 GMT, John Neffenger wrote: > Fixes [JDK-8248381](https://bugs.openjdk.java.net/browse/JDK-8248381). modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/MonocleTimer.java line 38: > 37: private static final String THREAD_NAME = "Monocle Timer"; > 38: private static final int POOL_SIZE = 1; > 39: I don't think there is a usecase where POOL_SIZE would be different from 1, so this could be skipped? - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Fri, 26 Jun 2020 23:53:21 GMT, John Neffenger wrote: >> It looks simple enough for a single reviewer. Since it is in Monocle, >> @johanvos will probably want to review it. > > Note that this is a regression error that is not present in JavaFX 14, so it > would be good to fix it before JavaFX 15 > is released. The regression was caused by the fix for [JDK-8176499](https://bugs.openjdk.java.net/browse/JDK-8176499). See PR #117. - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Fri, 26 Jun 2020 23:23:43 GMT, Kevin Rushforth wrote: >> Fixes [JDK-8248381](https://bugs.openjdk.java.net/browse/JDK-8248381). > > It looks simple enough for a single reviewer. Since it is in Monocle, > @johanvos will probably want to review it. Note that this is a regression error that is not present in JavaFX 14, so it would be good to fix it before JavaFX 15 is released. - PR: https://git.openjdk.java.net/jfx/pull/256
Re: RFR: 8248381: Create a daemon thread for MonocleTimer
On Fri, 26 Jun 2020 03:50:02 GMT, John Neffenger wrote: > Fixes [JDK-8248381](https://bugs.openjdk.java.net/browse/JDK-8248381). It looks simple enough for a single reviewer. Since it is in Monocle, @johanvos will probably want to review it. - PR: https://git.openjdk.java.net/jfx/pull/256
