Checking inactivity with periodic tasks in Java

Question

I have created a simple HealthCheck service. I have two approaches:

1. Create a task canceling the previous one if the system is active, rescheduling the new task.
2. Create a periodic task and regularly check the last active time.

Which one is better?

Recommendations on how to improve my usage of synchronized, volatile, AtomicBoolean keywords in both approaches above are welcome.

public interface IHealthCheck {
    void start();
    void stop();
    void onActive();
    void onExpire();
}

public class Main {
    public static void main(String[] args) {
        IHealthCheck check = new HealthCheck2();

        new Thread(() -> checkTask(check, 5)).start();

        new Thread(() -> checkTask(check, 6)).start();

        new Thread(() -> checkTask(check, 7)).start();

        sleep(100);
        check.start();

    }

    private static void checkTask(IHealthCheck check, long sleep) {
        for (int i = 0; i < 1000; i++) {
            sleep(sleep);
            check.onActive();
        }
    }

    private static void sleep(long n) {
        try {
            Thread.sleep(n);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

package com.deneme;

import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

public class HealthCheck implements IHealthCheck {

    private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor();
    private ScheduledFuture<?> future;

    @Override
    public void start() {
        onActive();
    }

    @Override
    public synchronized void stop() {
        if (future != null) {
            future.cancel(true);
        }
    }

    @Override
    public synchronized void onActive() {
        if (future != null) {
            future.cancel(true);
        }
        System.out.println("Active: " + System.currentTimeMillis());
        future = scheduler.schedule(this::onExpire, 200, TimeUnit.MILLISECONDS);
    }

    @Override
    public void onExpire() {
        System.out.println("Expired: " + System.currentTimeMillis());
    }
}

package com.deneme;

import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

public class HealthCheck2 implements IHealthCheck {

    private final ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor();
    private ScheduledFuture<?> future;
    private Instant lastActiveTime;
    private boolean expired;


    @Override
    public void start() {
        onActive();
        future = scheduler.scheduleWithFixedDelay(this::check, 50, 50, TimeUnit.MILLISECONDS);
    }

    private synchronized void check() {
        long diff = ChronoUnit.MILLIS.between(lastActiveTime, Instant.now());
        if (diff > 100) {
            onExpire();
        }
    }


    @Override
    public synchronized void stop() {
        if (future != null) {
            future.cancel(true);
        }
    }

    @Override
    public synchronized void onActive() {
        expired = false;
        lastActiveTime = Instant.now();
        System.out.println("Active: " + lastActiveTime.toEpochMilli());
    }

    @Override
    public void onExpire() {
        if (!expired) {
            expired = true;
            System.out.println("Expired: " + System.currentTimeMillis());
        }
    }


}

Answer 1

It depends on your needs, but generally I would say that the second approach would be better.

Why? because it allows you to run multiple tasks and that gives flexibility to your system. Now the use of synchronized may be troublesome if there are many threads running, if you don't need to handle static or shared resources/variables it would be better to not use it.

Now, in the first place your classes has no static variables, and does not access to shared resources so synchronization is not needed. In the case of the class HealthCheck2, the following method will work fine without synchronized

    private void check() {
        //the variable diff isn't needed either
        if (ChronoUnit.MILLIS.between(lastActiveTime, Instant.now()) > 100)
            onExpire();
    }

Why? Because the check method is uniquely owned by each HealthCheck2 instance, even if they call the start method, the scheduler and the future variables are equally owned.

I think you could remember when you were learning about static variables and methods that these are accessible and modifiable by any class instance. Well, the oposite is true, non-static variables and methods are unique for each instance.

Hope it helped you.