Simple Java task scheduler

Question

The task is pretty simple: implement a timer that executes tasks when the next event takes place. I created a task scheduler based on a priority queue.

1. A new thread is created for TaskTimerRunnable and sleeps during run() until the next task.
2. When a new task is added, it checks if the time for the new task is sooner than the soonest time in the priority queue and interrupt the timer thread to start the loop again.
3. Each task is executed in a separate thread from a thread pool.

Is there any way to improve my code?

import java.util.concurrent.BlockingQueue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.PriorityBlockingQueue;

public class TaskScheduler {

    interface Task extends Runnable {
    }

    private static class TaskTime implements Comparable<TaskTime> {
        public long timestampMilis;
        public Task task;

        TaskTime(long timestamp, Task task) {
            this.task = task;
            this.timestampMilis = timestamp;
        }

        @Override
        public int compareTo(TaskTime task) {
            long diff = timestampMilis - task.timestampMilis;

            if (diff > 0) return 1;

            return diff == 0 ? 0 : -1;
        }
    }

    public static class TimerQueue {
        private BlockingQueue<TaskTime> queue = new PriorityBlockingQueue<>();

        public void addTask(long timestampMillis, Task task) {
            queue.add(new TaskTime(timestampMillis, task));
        }

        public long getSonnestTimestampMillis() {
            TaskTime taskTime = queue.peek();
            if (taskTime == null) return 0;
            return taskTime.timestampMilis;
        }

        private Task popSoonestTask() {
            TaskTime taskTime = queue.poll();
            if (taskTime == null) return null;
            return taskTime.task;
        }
    }

    public static class TaskTimerRunnable implements Runnable {
        private static final int MAX_THREADS = 100;
        private TimerQueue queue;
        private ExecutorService pool = Executors.newFixedThreadPool(MAX_THREADS);
        private Thread timerThread;

        public TaskTimerRunnable() {
            this.queue = new TimerQueue();
        }

        @Override
        public void run() {
            timerThread = Thread.currentThread();
            while (true) {
                long soonestTimestampMillis = queue.getSonnestTimestampMillis();
                long currentTimestampMillis = System.currentTimeMillis();
                if (soonestTimestampMillis <= currentTimestampMillis) {
                    pool.submit(queue.popSoonestTask());
                } else {
                    sleep(soonestTimestampMillis - currentTimestampMillis);
                }
            }
        }

        private void sleep(long millis) {
            try {
                if (millis <= 0) {
                    Thread.sleep(Long.MAX_VALUE);
                } else {
                    Thread.sleep(millis);
                }
            } catch (InterruptedException e) {
                return;
            }
        }

        public void addTask(long whenTimestampMillis, Task task) {
            queue.addTask(whenTimestampMillis, task);
            long soonestTimestampMillis = queue.getSonnestTimestampMillis();
            if (whenTimestampMillis >= soonestTimestampMillis) {
                // interrupt timer thread
                if (timerThread != null) {
                    timerThread.interrupt();
                }
            }
        }
    }

    public static class TestTask implements Task {
        private long timestampMillis;

        public TestTask(long timestampMillis) {
            this.timestampMillis = timestampMillis;
        }

        @Override
        public void run() {
            System.out.println("Timestamp: " + timestampMillis);
        }
    }

    public static void main(String[] args) {
        TaskTimerRunnable timer = new TaskTimerRunnable();
        Thread timerThread = new Thread(timer);

        long currentTimestamp = System.currentTimeMillis();
        for (int i = 0; i < 20; i++) {
            long whenMillis = currentTimestamp + 5000 + i * 1000;
            timer.addTask(whenMillis, new TestTask(whenMillis));
        }

        timerThread.start();

        try {
            timerThread.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

Answer 1

Typos

private static class TaskTime implements Comparable<TaskTime> {
    public long timestampMilis;
    // ...
}

timestampMilis should be timestampMillis, and it's missing the final modifier if you want it be publicly accessible in an immutable way.

public long getSonnestTimestampMillis() {
    TaskTime taskTime = queue.peek();
    if (taskTime == null) return 0;
    return taskTime.timestampMilis;
}

The method name should be getSoonestTimestampMillis(). Actually, if you think 'soonest' is a mouthful, you can also consider 'next'.

compareTo()

In addition to @TheCoffeeCup's answer, there's Long.compare(long, long) if you really prefer to return the range [-1, 1].

No more work

// inside TimerQueue
private Task popSoonestTask() {
    TaskTime taskTime = queue.poll();
    if (taskTime == null) return null;
    return taskTime.task;
}

// inside TaskTimerRunnable
if (soonestTimestampMillis <= currentTimestampMillis) {
    pool.submit(queue.popSoonestTask());
} else {
    // ...
}

The problem here is that the caller simply passes the potential null result to the pool, which will result in a NullPointerException. You should check for this first.

Rip Van Winkle

private void sleep(long millis) {
    try {
        if (millis <= 0) {
            Thread.sleep(Long.MAX_VALUE);
        } else {
            Thread.sleep(millis);
        }
    } catch (InterruptedException e) {
        return;
    }
}

I'm not sure if you really want your program to sleep() for \$2^{63} - 1\$ milliseconds if it receives a <= 0 value. What if your program is told to launch just one task one second ago?

Scheduling an alternative

Instead of using a regular fixed-size thread-pool-backed ExecutorService, why not consider ScheduledExecutorService? Even though its APIs deal with relative timings and you are trying to use absolute timings, its Javadoc describes a possible simple transformation with some caveats:

All schedule methods accept relative delays and periods as arguments, not absolute times or dates. It is a simple matter to transform an absolute time represented as a Date to the required form. For example, to schedule at a certain future date, you can use: schedule(task, date.getTime() - System.currentTimeMillis(), TimeUnit.MILLISECONDS). Beware however that expiration of a relative delay need not coincide with the current Date at which the task is enabled due to network time synchronization protocols, clock drift, or other factors.

Answer 2

In my opinion, you seriously don't need this:

interface Task extends Runnable {
}

The only point I can see for that is to change the name. Stick to Runnable.

---

This compareTo() method:

    @Override
    public int compareTo(TaskTime task) {
        long diff = timestampMilis - task.timestampMilis;

        if (diff > 0) return 1;

        return diff == 0 ? 0 : -1;
    }

can be actually simply written as:

    @Override
    public int compareTo(TaskTime task) {
        return timestampMilis - task.timestampMilis;
    }

Because compareTo() does not need to return -1, 0, or 1. It returns <0, 0, >0.

If you want to retain your way of result, use Math.signum():

    @Override
    public int compareTo(TaskTime task) {
        // Math.signum returns a float, so cast is necessary
        return (int) Math.signum(timestampMilis - task.timestampMilis);
    }

---

I'm not a fan of how you organize your classes. Don't do:

public class ContainerClass {

    public static class Class1 {
        // ...
    }

    public static class Class2 {
        // ...
    }

    // etc.
}

Do:

public class Class1 { // This is the most important class
    // ...
}

class Class2 {
    // ...
}

Answer 3

if statement always true

In this code block:

        queue.addTask(whenTimestampMillis, task);
        long soonestTimestampMillis = queue.getSonnestTimestampMillis();
        if (whenTimestampMillis >= soonestTimestampMillis) {
            // interrupt timer thread
            if (timerThread != null) {
                timerThread.interrupt();
            }
        }

whenTimestampMillis should always be greater than or equal to soonestTimestampMillis. Therefore you will always interrupt the timer thread. I think what you meant to do was interrupt the timer thread if the task you just added was the earliest task. If that is what you intended, then you should change the >= to ==.