java - visibility in ScheduledThreadPoolExecutor threads

Question

I have a stop watch which runs for specified amount of time (duration parameter) using inner single threaded scheduled executor:

public class StopWatch {

    private final AtomicBoolean running = new AtomicBoolean();
    private final ScheduledThreadPoolExecutor executor = new ScheduledThreadPoolExecutor(1);

    private long lastTimeMillis;
    private long durationMillis;
    private long elapsedMillis;
    private ScheduledFuture<?> future;

    public void start(long duration, TimeUnit timeUnit) {
        if (running.compareAndSet(false, true)) {
            durationMillis = timeUnit.toMillis(duration);
            lastTimeMillis = System.currentTimeMillis();
            elapsedMillis = 0;
            future = executor.schedule(this::tick, 0, TimeUnit.MILLISECONDS);
        }
    }

    (...)

    private void tick() {
        long now = System.currentTimeMillis();
        elapsedMillis += now - lastTimeMillis;
        lastTimeMillis = now;

        // do some stuff

        if (elapsedMillis < durationMillis) {
            future = executor.schedule(this::tick, 100, TimeUnit.MILLISECONDS);
            return;
        }

        running.compareAndSet(true, false);
    }

    (...)

}

My question is: will I run into any visibility problems with this approach (i.e. after executing .start() on StopWatch again once first cycle is finished)?

elapsedMillis and lastTimeMillis are updated in two threads and durationMillis is updated in first thread and read in second one, but it's happening rather sequentially and scheduled task starts after first thread is done with updating fields. I'm still not certain if skipping volatility on those fields is safe though (probably not).

score 0 · Answer 1 · answered Nov 15 '20 at 16:37

What you are doing can be better accomplished via the ScheduledExecutorService API:

public class StopWatch {
    private final ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor();

    private ScheduledFuture<?> future;

    public void start(long duration, TimeUnit timeUnit) {
        if (future == null || future.isDone()) {
            future = executor.scheduleAtFixedRate(this::tick, 0L, 100L, TimeUnit.MILLISECONDS);
            executor.schedule((Runnable) () -> future.cancel(false), duration, timeUnit);
        }
    }

    private void tick() {

    }
}

score 0 · Accepted Answer · answered Nov 16 '20 at 05:37

Unless you really need to squeeze out the last bit of performance, I would not be worried about having a volatile more or less in the above code. So the simple answer to your question is: make the fields volatile.

And there is a happens before relation between the schedule and the task being executed. See the memory consistency effects here:

https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/util/concurrent/ExecutorService.html

So the task should be able to see the changes that were made before the task was placed on the executor. This is because the happens before relation is transitive.

java - visibility in ScheduledThreadPoolExecutor threads

2 Answers2