Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8248381: Create a daemon thread for MonocleTimer #256

Closed
wants to merge 2 commits into from

Conversation

@jgneff
Copy link
Contributor

jgneff commented Jun 26, 2020

Fixes JDK-8248381.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

  • Kevin Rushforth (kcr - Reviewer)
  • Johan Vos (jvos - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/256/head:pull/256
$ git checkout pull/256

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 26, 2020

👋 Welcome back jgneff! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jun 26, 2020
@mlbridge
Copy link

mlbridge bot commented Jun 26, 2020

Webrevs

@kevinrushforth kevinrushforth requested a review from johanvos Jun 26, 2020
@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 26, 2020

It looks simple enough for a single reviewer. Since it is in Monocle, @johanvos will probably want to review it.

@jgneff
Copy link
Contributor Author

jgneff commented Jun 26, 2020

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 27, 2020

The regression was caused by the fix for JDK-8176499. See PR #117.

@@ -34,6 +34,9 @@
* Monocle implementation class for Timer.
*/
final class MonocleTimer extends Timer {
private static final String THREAD_NAME = "Monocle Timer";
private static final int POOL_SIZE = 1;

This comment has been minimized.

@johanvos

johanvos Jun 27, 2020 Collaborator

I don't think there is a usecase where POOL_SIZE would be different from 1, so this could be skipped?

This comment has been minimized.

@jgneff

jgneff Jun 27, 2020 Author Contributor

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.

javafx-threads-2020-06-27

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 27, 2020

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 27, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jun 27, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth kevinrushforth self-requested a review Jun 27, 2020
@jgneff
Copy link
Contributor Author

jgneff commented Jun 27, 2020

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 27, 2020

OK, that seems fine then. I'll take a closer look and then finish my review.

@jgneff
Copy link
Contributor Author

jgneff commented Jun 27, 2020

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! 😄

@jgneff
Copy link
Contributor Author

jgneff commented Jun 28, 2020

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.

@johanvos
Copy link
Collaborator

johanvos commented Jun 29, 2020

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 2, 2020

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.

@jgneff
Copy link
Contributor Author

jgneff commented Jul 3, 2020

Given that this is a regression introduced in JavaFX 14, this fix seems like a good candidate for JavaFX 15 ...

Actually, this is a regression introduced in JavaFX 15, so we have the chance to fix it before it's ever released.

Go ahead and retarget your PR to the jfx15 branch (you should not need to merge anything) ...

Like this? I'll try that now.

@jgneff jgneff changed the base branch from master to jfx15 Jul 3, 2020
Copy link
Member

kevinrushforth left a comment

Looks good to me. I verified that it does, in fact, fix the regression. As long as @johanvos also approves, then this is fine to go in for jfx15

I think we can address any issues relating to Timer start / stop / pause / resume in a subsequent release. There was some work done to support pause / resume as part of JDK-8189926, but it was only implemented on macOS (with follow-up issues filed to implement it on Windows and Linux). It didn't address any issues with start / stop. In particular we have never supported restarting the JavaFX runtime, so unless there are other uses of stop besides shutting down the runtime, I don't know that it matters if the timer can be restarted.

Copy link
Collaborator

johanvos left a comment

I'm ok with this, as it fixes regression.
A longer-term requirement could be the addition of platform-independent start/stop and pause/resume methods, similar to http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/4e1e2f56c7af (https://bugs.openjdk.java.net/browse/JDK-8189926)

@openjdk
Copy link

openjdk bot commented Jul 20, 2020

@jgneff This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8248381: Create a daemon thread for MonocleTimer

Reviewed-by: kcr, jvos
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 26 commits pushed to the jfx15 branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge jfx15 into your branch, and then specify the current head hash when integrating, like this: /integrate 2f4666a71ecd3c921b669b5f2bd3f595dd5ef047.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jul 20, 2020
@jgneff
Copy link
Contributor Author

jgneff commented Jul 21, 2020

/integrate

@openjdk openjdk bot added the sponsor label Jul 21, 2020
@openjdk
Copy link

openjdk bot commented Jul 21, 2020

@jgneff
Your change (at version 4ca2bc0) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jul 21, 2020

/sponsor

@openjdk openjdk bot closed this Jul 21, 2020
@openjdk openjdk bot added integrated and removed sponsor ready rfr labels Jul 21, 2020
@openjdk
Copy link

openjdk bot commented Jul 21, 2020

@kevinrushforth @jgneff The following commits have been pushed to jfx15 since your change was applied:

  • 2f4666a: 8248490: [macOS] Undecorated stage does not minimize
  • 5de99be: Merge
  • e2d1c02: 8176270: Adding ChangeListener to TextField.selectedTextProperty causes StringOutOfBoundsException
  • d67c47f: 8201567: QuantumRenderer modifies buffer in use by JavaFX Application Thread
  • cd57bb5: Merge
  • 126637f: 8201570: Get two bytes for the Linux input event type, not four
  • f3a0446: 8247963: Update SQLite to version 3.32.3
  • 32584db: 8238954: Improve performance of tiled snapshot rendering
  • 869ea40: 8244212: Optionally download media and webkit libraries from latest openjfx EA build
  • 62f8cee: 8247947: Build DirectShow Samples (Base Classes) from source checked into repo
  • 527cc2b: 8248551: [TestBug] Ignore two failing FXML unit tests which use Nashorn script engine
  • 45c9854: 8238080: FXMLLoader: if script engines implement javax.script.Compilable compile scripts
  • 15f97ed: 8240264: iOS: Unnecessary logging on every pulse when GL context changes
  • 2ca509a: 8193800: TreeTableView selection changes on sorting
  • a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
  • f6ec5f1: Merge
  • 1663624: Merge
  • 13f42e1: Merge
  • 78bf7b7: 8245422: Better Pisces rasterizing
  • f8cc090: Merge
  • 4597437: Merge
  • f1ac39b: Merge
  • bb5ce2c: Merge
  • d299465: Merge
  • d2346b8: Merge
  • 397587d: 8241108: Glib improvements

Your commit was automatically rebased without conflicts.

Pushed as commit 5f60ea5.

@jgneff jgneff deleted the jgneff:monocle-timer branch Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.