Executing a task at a particular time in the morning using ScheduledExecutorService

Question

I have a task which I need to run every 6 in the morning. I have the below code which does the job and uses ScheduledExecutorService.

ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);
Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);

int hour = calendar.get(Calendar.HOUR_OF_DAY);
int delayInHour = hour < 6 ? 6 - hour : 24 - (hour - 6);

System.out.println("Current Hour: "+hour);
System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);

scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

I am opting for code review to see whether it can be improved or not.

Answer 1

According to the javadoc, a Calendar instance is initialized with the current date and time. So instead of this:

Date date = new Date();
Calendar calendar = Calendar.getInstance();
calendar.setTime(date);
int hour = calendar.get(Calendar.HOUR_OF_DAY);

You don't need to create a separate Date object, so you can simplify like this for the same effect:

Calendar calendar = Calendar.getInstance();
int hour = calendar.get(Calendar.HOUR_OF_DAY);

---

It would be good to split this code into multiple reusable and testable functions. For example you could have a function that returns the hours until some target hour:

private int getHoursUntilTarget(int targetHour) {
    Calendar calendar = Calendar.getInstance();
    int hour = calendar.get(Calendar.HOUR_OF_DAY);
    return hour < targetHour ? targetHour - hour : targetHour - hour + 24;
}

Now the target hour is a parameter, this is reusable and flexible, and eliminates the magic number 6 that was duplicated at multiple places. Also note that I changed the ternary expression a bit, I think targetHour - hour + 24 is more natural, and easier to understand due it's symmetry with targetHour - hour.

You can move the scheduling to another method, which can now have a single responsibility (scheduling) and not worry about calculating hours.

---

Here's a unit test to verify that getHoursUntilTarget works well:

@Test
public void testGetHoursUntilTarget() {
    Calendar calendar = Calendar.getInstance();
    int hour = calendar.get(Calendar.HOUR_OF_DAY);
    assertEquals(24, getHoursUntilTarget(hour));
    assertEquals(1, getHoursUntilTarget(hour + 1));
    assertEquals(23, getHoursUntilTarget(hour - 1));
}

Note that at 5:59 AM, the hours until target will be 1, even though 6 AM is only one minute away. This is how it was in your original code, I hope that's ok with you.

---

There is some noise in your post:

System.out.println("Comuted Delay for next 5 AM: "+intDelayInHour);
scheduler.scheduleAtFixedRate(new MyTask(), intDelayInHour, 24, TimeUnit.HOURS);

"next 5 AM" is not consistent with the rest of your code (you used 6 everywhere else), and you don't have a variable intDelayInHour but you have delayInHour. Review more carefully before you post.

Answer 2

You have an issue if you live/deploy in an area that has daylight saving times. The way you set it up is to happen every 24 hours, rather than at a particular time each day. When a time shift happens, your event will happen at 5am, or 7am depending.

In the past I have set up a time-of-day schedule for jobs, and I found the best way to do that was to set up a self-repeating wrapper for a task. When the task runs, it reschedules itself for the next iteration.

So, the way I did it was to create a Runnable that self-replicates each time it runs. The concept ends up being somewhat simple (somewhat...). So, consider the following code that will reschedule itself after running:

import java.util.Calendar;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;


public class ReschedulingDailyTask implements Runnable {

    private final ScheduledExecutorService service;

    private final Runnable task;
    private final int hour;
    private final int min;
    private final int sec;

    private final AtomicBoolean active = new AtomicBoolean(false);
    private final AtomicBoolean scheduled = new AtomicBoolean(false);

    public ReschedulingDailyTask(final ScheduledExecutorService service,
            final Runnable task, final int hour) {
        this (service, task, hour, 0, 0);
    }

    public ReschedulingDailyTask(final ScheduledExecutorService service,
            final Runnable task, final int hour, final int min, final int sec) {
        this.service = service;
        this.task = task;
        this.hour = hour;
        this.min = min;
        this.sec = sec;

    }

    public void enable() {
        if (!active.getAndSet(true)) {
            // was not enabled:
            reSchedule();
        }
    }

    public void disable() {
        active.getAndSet(false);
    }

    private void reSchedule() {
        if (!scheduled.getAndSet(true)) {
            Calendar calendar = Calendar.getInstance();
            long now = calendar.getTimeInMillis();
            calendar.set(Calendar.HOUR, hour);
            calendar.set(Calendar.MINUTE, min);
            calendar.set(Calendar.SECOND, sec);
            calendar.set(Calendar.MILLISECOND, 0);
            while (calendar.getTimeInMillis() < now) {
                // scheduled in the past, go forward one day....
                calendar.add(Calendar.DAY_OF_MONTH, 1);
            }

            // for debug, if needed....
            //System.out.println("Reschedule for " + calendar.getTimeInMillis() 
            //        + " (in " + (calendar.getTimeInMillis() - now) + "ms)" );

            // set ourselves up to run at a given time.
            service.schedule(this, calendar.getTimeInMillis() - now, TimeUnit.MILLISECONDS);
        }

    }

    @Override
    public void run() {
        // since we are running, we are no longer scheduled...
        scheduled.set(false);

        // we may have been disabled after we were enabled ...
        // you can't cancel the schedule, but you can ignore the task...
        if (!active.get()) {
            return;
        }

        // we were active, and we run the task, and force the reschedule.
        try {
            task.run();
        } finally {
            reSchedule();
        }
    }

}

I tested it with the following code:

    private static final class DaemonThreadFactory implements ThreadFactory {

        @Override
        public Thread newThread(Runnable runnable) {
            Thread thread = new Thread(runnable, "Schedule Thread");
            thread.setDaemon(true);
            return thread;
        }

    }

    public static void main(String[] args) throws InterruptedException {
        ScheduledExecutorService service = Executors.newScheduledThreadPool(1, new DaemonThreadFactory());
        Calendar cal = Calendar.getInstance();
        System.out.println("Time now: " + cal.getTimeInMillis());
        cal.add(Calendar.SECOND, 5);
        cal.set(Calendar.MILLISECOND, 0);
        System.out.println("Execute at: " + cal.getTimeInMillis());
        int hour = cal.get(Calendar.HOUR);
        int min = cal.get(Calendar.MINUTE);
        int sec = cal.get(Calendar.SECOND);
        final Object lock = new Object();
        Runnable task = new Runnable() {

            @Override
            public void run() {
                System.out.println("Time is " + Calendar.getInstance().getTimeInMillis());
                synchronized(lock) {
                    lock.notifyAll();
                }
            }

        };

        ReschedulingDailyTask daily = new ReschedulingDailyTask(service, task, hour, min, sec);
        daily.enable();

        System.out.println("Waiting");
        synchronized (lock) {
            lock.wait();
        }
        System.out.println("Done");
    }

Note that I create a Daemon thread factory for the Scheduler, so that, if the main thread closes, the application terminates. The above method will schedule the task for about 5 seconds in the future....