20

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)
duffymo
  • 305,152
  • 44
  • 369
  • 561
Zak
  • 24,947
  • 11
  • 38
  • 68

4 Answers4

21

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.

Eddie
  • 53,828
  • 22
  • 125
  • 145
Kevin Montrose
  • 22,191
  • 9
  • 88
  • 137
  • 1
    As Kevin said, you are calling "timer.schedule(this, delay, period)" with the same "this" each time. The TimerTask is not meant to be given to multiple different timers. Each TimerTask instance is meant to be scheduled exactly one time. – Eddie Jun 25 '09 at 01:07
  • 1
    Thanks, that totally does not pop out of the documentation, and as soon as I switched to creating new TimerTasks, everything went according to plan.. – Zak Jun 25 '09 at 01:28
  • 2
    The specification of schedule states: @throws IllegalStateException if task was already scheduled or cancelled, timer was cancelled, or timer thread terminated. The task was already scheduled in an earlier Timer, hence the exception being thrown. – notnoop Jun 25 '09 at 02:16
  • msaeed is right about the documentation, although its not as clear as it could be. Additionally, although I'm certainly not dogmatic about it, the OS nature of Java certainly helps in tracking down these "not quite clear" bugs. – Kevin Montrose Jun 25 '09 at 02:32
  • I suggest this as a better solution (yes, blowing my own trumpet, but I feel it should be said for the record :-P): http://stackoverflow.com/questions/32001/resettable-java-timer/32057#32057 – C. K. Young Jun 25 '09 at 03:33
4

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.

duffymo
  • 305,152
  • 44
  • 369
  • 561
0

In this exmaple, "Executed...." will be printed after 4 seconds of delay. After that, it will be printed continuously every 3 seconds:

import java.util.*;

class TimeSetting {
    public static void main(String[] args) {
        Timer t = new Timer();
        TimerTask time = new TimerTask() {
            public void run() {
                System.out.println("Executed......");
            }
        };
        t.scheduleAtFixedRate(time, 4000, 3000);
        /*
        * The task will be started after 4 secs and 
        * for every 3 seconds the task will be continuously 
        * executed.....
        */
    }
}
Cardinal System
  • 2,749
  • 3
  • 21
  • 42
deeban
  • 99
  • 1
  • 11
0

You can use ScheduledExecutorService, which allows you to schedule the same task multiple times without using scheduleAtFixedRate. Here's a quick example:

ScheduledExecutorService executorService = Executors.newScheduledThreadPool(1);
Runnable timerTask = new Runnable() {
    @Override
    public void run() {
        // Do something
        System.out.println("Task run!");
        // Schedule again
        executorService.schedule(this, 15, TimeUnit.SECONDS);
    }
};
// Schedule
executorService.schedule(timerTask, 15, TimeUnit.SECONDS);
Cardinal System
  • 2,749
  • 3
  • 21
  • 42