Use a ScheduledExecutorService to handle periodic tasks by johanvos · Pull Request #433 · gluonhq/rich-text-area

johanvos · 2026-03-11T20:39:25Z

Copilot

Pull request overview

This PR refactors the internal SmartTimer utility to replace java.util.Timer/TimerTask with a ScheduledExecutorService-based implementation for periodic execution.

Changes:

Replace Timer/TimerTask with ScheduledExecutorService + ScheduledFuture.
Implement pause() via task cancellation and executor shutdown.
Implement start() via scheduleAtFixedRate(...) on a single-thread scheduled executor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T20:43:28Z

+        if (scheduler == null || scheduler.isShutdown()) {
+            scheduler = Executors.newSingleThreadScheduledExecutor(r -> {
+                Thread t = new Thread(r);
+                t.setDaemon(true);
+                return t;
+            });
+            scheduledTask = scheduler.scheduleAtFixedRate(task, delay, period, TimeUnit.MILLISECONDS);


start() schedules the provided task to run on the executor’s background thread. In this project, the only current use (paragraphListView::evictUnusedObjects) traverses JavaFX scene-graph nodes and mutates caches from RichTextAreaSkin, which must occur on the JavaFX Application Thread; running it off-thread can lead to IllegalStateException/undefined behavior. Consider wrapping the runnable so it executes via Platform.runLater(...) (or switching this utility to a JavaFX Timeline), so SmartTimer callbacks are always executed on the FX thread.

We are not going to make assumptions on how clients of this code are working. There is no guarantee that all users of this code require running on the JavaFX appthread. Moreover, it's bad practice to use the JavaFX appthread until the very last moment where the scene graph is manipulated.

Can there be a scenario where scheduler is null but scheduledTask is not? It doesn't harm to add another if statement before re-initializing scheduledTask ?

That is very unlikely, but theoretically possible if one thread invokes pause and another invokes start. In that case, it doesn't help adding an if-statement, as the system is unstable already.
In order to prevent the potential race condition, I made the start and pause synchronized. There is no scenario now where scheduler is null while scheduledTask is not.

Copilot · 2026-03-11T20:43:28Z

+            scheduledTask.cancel(true);
+            scheduledTask = null;
+        }
+        if (scheduler != null) {
+            scheduler.shutdownNow();


pause() uses scheduledTask.cancel(true) and shutdownNow(), which will interrupt the timer thread if the task is currently running. This is a behavior change from the previous Timer.cancel() (which does not interrupt a running task) and can leave work half-done if the runnable performs multi-step updates. If interruption is not explicitly required, prefer non-interrupting cancellation (cancel(false)) and a graceful shutdown (shutdown()), or otherwise document/guarantee that task is interruption-safe.

Suggested change

scheduledTask.cancel(true);

scheduledTask = null;

}

if (scheduler != null) {

scheduler.shutdownNow();

scheduledTask.cancel(false);

scheduledTask = null;

}

if (scheduler != null) {

scheduler.shutdown();

eugener · 2026-03-12T23:44:08Z

I think there is a pre-existing, FX thread violation here.

evictUnusedObjects reads scene graph nodes from the executor's background thread. The scene graph is not thread-safe. The old Timer code had the same bug. The task body needs Platform.runLater() wrapping, or SmartTimer should use a JavaFX ScheduledService/Timeline instead.

I would consider whether a JavaFX Timeline (which fires on the FX thread and has built-in pause/play/stop lifecycle) would be a simpler, safer replacement that avoids all the threading issues at once.

eugener · 2026-03-12T23:47:06Z

+            scheduledTask = null;
+        }
+        if (scheduler != null) {
+            scheduler.shutdownNow();


No awaitTermination after shutdownNow().
shutdownNow() returns immediately. If evictUnusedObjects is mid-execution when dispose() calls pause(), the task continues running against potentially nulled-out fields. We have to add

scheduler.shutdownNow(); scheduler.awaitTermination(100, TimeUnit.MILLISECONDS);

eugener · 2026-03-12T23:50:23Z

-        if ( timer != null ) {
-            timer.cancel();
-            timer = null;
+    public synchronized void pause() {


Both start() and pause() are only ever called from the FX thread, so the synchronized adds cost without benefit in current usage

There is no guarantee that those will not be called from other threads, so the cost of synchronizing is minimal compared to the risk

eugener · 2026-03-12T23:51:01Z

+    public synchronized void start( ) {
+        if (scheduler == null || scheduler.isShutdown()) {
+            scheduler = Executors.newSingleThreadScheduledExecutor(r -> {
+                Thread t = new Thread(r);


should set a name like "rta-cache-eviction" for debuggability

that would limit the usage to the existing cache eviction (which TBH I'm not sure we need)

eugener · 2026-03-12T23:52:21Z

-            timer = null;
+    public synchronized void pause() {
+        if (scheduledTask != null) {
+            scheduledTask.cancel(true);


cancel(true) before shutdownNow() is redundant -- shutdownNow() already interrupts workers and drains the queue.

cancel(true) + shutdownNow() interrupts a running task, The original Timer.cancel() did not interrupt - it only prevented future executions. This is a behavior change that could leave evictUnusedObjects half-done (e.g. fonts evicted but images not). Should we use cancel(false) + shutdown()?

I thought about that. I'm more worried about jobs that keep running, so I'm personally in favor of interrupting.

eugener · 2026-03-12T23:53:37Z

+                t.setDaemon(true);
+                return t;
+            });
+            scheduledTask = scheduler.scheduleAtFixedRate(task, delay, period, TimeUnit.MILLISECONDS);


For cache eviction, scheduleWithFixedDelay is semantically more correct (wait N after completion, not wall-clock intervals), comparing to scheduleAtFixedRate

johanvos · 2026-03-13T07:51:57Z

I think there is a pre-existing, FX thread violation here.

evictUnusedObjects reads scene graph nodes from the executor's background thread. The scene graph is not thread-safe. The old Timer code had the same bug. The task body needs Platform.runLater() wrapping, or SmartTimer should use a JavaFX ScheduledService/Timeline instead.

I would consider whether a JavaFX Timeline (which fires on the FX thread and has built-in pause/play/stop lifecycle) would be a simpler, safer replacement that avoids all the threading issues at once.

True, but that is a different (and as you said pre-existing) bug.
I'd like to use as much standard JDK code as possible, instead of JavaFX code. There is nothing that forces this timer class to use JavaFX, so using JavaFX here would limit its usage.

johanvos · 2026-03-13T08:00:13Z

I filed #436 as a follow-up. Since this PR fixes a critical memory issue, and it does not contain regression, I will merge this.
The discussion about eviction and what parts should be done on the FX Thread can be done in #436

Use a ScheduledExecutorService to handle periodic tasks

f9513b9

Fix gluonhq#432

johanvos requested a review from Copilot March 11, 2026 20:39

Copilot started reviewing on behalf of johanvos March 11, 2026 20:40 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Effectively pause timer (and remove references) when skin is disposed.

7346279

johanvos requested review from abhinayagarwal and jperedadnr March 12, 2026 07:28

Use synchronized access to prevent race condition

8e0004b

eugener requested changes Mar 12, 2026

View reviewed changes

johanvos merged commit 433e8ed into gluonhq:main Mar 13, 2026
3 checks passed

johanvos deleted the 432-scheduler branch March 13, 2026 08:00

Conversation

johanvos commented Mar 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

eugener commented Mar 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugener Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugener Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eugener Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johanvos commented Mar 13, 2026

Uh oh!

johanvos commented Mar 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eugener Mar 12, 2026 •

edited

Loading

eugener Mar 12, 2026 •

edited

Loading

eugener Mar 12, 2026 •

edited

Loading