Java Timer

Question

I'm trying to use a timer to schedule a recurring event in an application. However, I want to be able to adjust the period at which the event fires in real time (according to the users input).

For example:

public class HelperTimer extends TimerTask
{
    private Timer timer;
    //Default of 15 second between updates
    private int secondsToDelay = 15;

    public void setPeriod(int seconds)
    {
        this.secondsToDelay = seconds;
        long delay = 1000; // 1 second
        long period = 1000*secondsToDelay; // seconds
        if (timer != null) 
        {
            timer.cancel();
        }
        System.out.println(timer);
        timer = new Timer();
        System.out.println(timer);
        timer.schedule(this, delay, period);
    }
    public int getPeriod()
    {
        return this.secondsToDelay;
    }
}

I then start a new instance of this class and call its set period function. However, when I do that, I get an Illegal state exception. You can see the System.out.println(timer); in there because I'm checking, and yep sure enough, they are two different timers... so why am I getting an IllegalStateException when I try to run a schedule call on a brand new Timer instance!?!?!?!

java.util.Timer@c55e36
java.util.Timer@9664a1
Exception in thread "AWT-EventQueue-0" java.lang.IllegalStateException: Task already scheduled or cancelled
    at java.util.Timer.sched(Unknown Source)
    at java.util.Timer.schedule(Unknown Source)
    at HelperTimer.setPeriod(HelperTimer.java:38)

Answer 1

You can't reuse a TimerTask as you're doing here.

Relevant porition of Timer:

private void sched(TimerTask task, long time, long period) {
    if (time < 0)
        throw new IllegalArgumentException("Illegal execution time.");

    synchronized(queue) {
        if (!thread.newTasksMayBeScheduled)
            throw new IllegalStateException("Timer already cancelled.");

        synchronized(task.lock) {
            //Right here's your problem.
            //  state is package-private, declared in TimerTask
            if (task.state != TimerTask.VIRGIN)
                throw new IllegalStateException(
                    "Task already scheduled or cancelled");
            task.nextExecutionTime = time;
            task.period = period;
            task.state = TimerTask.SCHEDULED;
        }

        queue.add(task);
        if (queue.getMin() == task)
            queue.notify();
    }
}

You'll need to refactor your code so that you create a new TimerTask, rather than re-using one.

Answer 2

It seems odd to me to have a TimerTask with its own Timer inside it. Bad design. I'd totally separate the two and have the TimerTask implementation be handed off to a Timer, and put all that logic about fiddling with the period inside another class that provides an interface for doing so. Let that class instantiate the Timer and TimerTask and send them off to do their work.