Skip to content

Use a ScheduledExecutorService to handle periodic tasks #433

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.

Sign up for GitHub

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

Merged
johanvos merged 3 commits into from
Mar 13, 2026
+24 −17
Merged

Use a ScheduledExecutorService to handle periodic tasks #433

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f9513b9
Use a ScheduledExecutorService to handle periodic tasks
johanvos Mar 11, 2026
7346279
Effectively pause timer (and remove references) when skin is disposed.
johanvos Mar 11, 2026
8e0004b
Use synchronized access to prevent race condition
johanvos Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 20 additions & 15 deletions rta/src/main/java/com/gluonhq/richtextarea/SmartTimer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,15 @@
package com.gluonhq.richtextarea;

import java.util.Objects;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

class SmartTimer {

private Timer timer;
private ScheduledExecutorService scheduler;
private ScheduledFuture<?> scheduledTask;
private final Runnable task;
private final long delay;
private final long period;
Expand All @@ -45,22 +48,24 @@ public SmartTimer( Runnable task, long delay, long period) {
}

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

@eugener eugener Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

@johanvos johanvos Mar 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

scheduledTask = null;
}
if (scheduler != null) {
scheduler.shutdownNow();
Comment on lines +52 to +56

Copilot AI Mar 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copilot uses AI. Check for mistakes.

@eugener eugener Mar 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

scheduler = null;
}
}

public void start( ) {
if ( timer == null ) {
timer = new Timer(true);
TimerTask timerTask = new TimerTask() {
@Override
public void run() {
task.run();
}
};
timer.scheduleAtFixedRate( timerTask, delay, period);
if (scheduler == null || scheduler.isShutdown()) {
scheduler = Executors.newSingleThreadScheduledExecutor(r -> {
Thread t = new Thread(r);

@eugener eugener Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@johanvos johanvos Mar 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

t.setDaemon(true);
return t;
});
scheduledTask = scheduler.scheduleAtFixedRate(task, delay, period, TimeUnit.MILLISECONDS);
Comment on lines +62 to +68

Copilot AI Mar 11, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

@johanvos johanvos Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@abhinayagarwal abhinayagarwal Mar 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@johanvos johanvos Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eugener eugener Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

Expand Down
Loading