5

From my readings, it seems that ScheduledExecutorService is the right way to start and stop timers in Java.

I need to port some code that starts and stops a timer. This is not a periodic timer. This code, stops the timer before starting it. So, effectively every start is really a restart(). I am looking for the right way to do this using the ScheduledExecutorService. Here is what I came up with. Looking for comments and insight on things I am missing:

ScheduledExecutorService _Timer = Executors.newScheduledThreadPool(1);
ScheduledFuture<?> _TimerFuture = null;

private boolean startTimer() {
    try {
        if (_TimerFuture != null) {
            //cancel execution of the future task (TimerPopTask())
            //If task is already running, do not interrupt it.
            _TimerFuture.cancel(false);
        }

        _TimerFuture = _Timer.schedule(new TimerPopTask(), 
                                       TIMER_IN_SECONDS, 
                                       TimeUnit.SECONDS);
        return true;
    } catch (Exception e) {
        return false;
    }
}

private boolean stopTimer() {
    try {
        if (_TimerFuture != null) {
            //cancel execution of the future task (TimerPopTask())
            //If task is already running, interrupt it here.
            _TimerFuture.cancel(true);
        }

        return true;
    } catch (Exception e) {
        return false;
    }
}

private class TimerPopTask implements Runnable  {  
    public void run ()   {  
        TimerPopped();
    }  
}

public void TimerPopped () {
    //Do Something
}

tia, rouble

rouble
  • 16,364
  • 16
  • 107
  • 102
  • Code as written won't compile: _batchTimer in startTimer() isn't declared anywhere. It'd be helpful if you could go into a bit more detail on the expected behaviors: what are the return values from startTimer and stopTimer? Why do you want start/stopTimer to be potentially blocking calls? – Sbodd Jun 23 '10 at 18:02
  • @Sbodd, when I go to start a timer, if there is already a timer running, I want to stop that, so that I don't have two timers popping at the same time. I think instead of the get() there, I just need to call cancel() on the future instance. – rouble Jun 23 '10 at 18:09

1 Answers1

3

This looks like a problem:

private boolean startTimer() {
    // ......
        if (_TimerFuture != null) {
            _TimerFuture.cancel(false);
        }

        _TimerFuture = _Timer.schedule(new TimerPopTask(), 
                                       TIMER_IN_SECONDS, 
                                       TimeUnit.SECONDS);
    // ......
}

Since you're passing a false to cancel, the old _TimerFuture may not get cancelled if the task is already running. A new one gets created anyway (but it won't run concurrently because your ExecutorService has a fixed thread pool size of 1). In any case, that doesn't sound like your desired behavior of restarting a timer when startTimer() is called.

I would rearchitect a bit. I would make the TimerPopTask instance be the thing you "cancel", and I would leave the ScheduledFutures alone once they are created:

private class TimerPopTask implements Runnable  {
    //volatile for thread-safety
    private volatile boolean isActive = true;  
    public void run ()   {  
        if (isActive){
            TimerPopped();
        }
    }  
    public void deactivate(){
        isActive = false;
    }
}

then I would retain the instance of TimerPopTask rather than the instance of ScheduledFuture and rearrange startTimer method thusly:

private TimerPopTask timerPopTask;

private boolean startTimer() {
    try {
        if (timerPopTask != null) {
            timerPopTask.deactivate();
        }

        timerPopTask = new TimerPopTask();
        _Timer.schedule(timerPopTask, 
                        TIMER_IN_SECONDS, 
                        TimeUnit.SECONDS);
        return true;
    } catch (Exception e) {
        return false;
    }
}

(Similar modification to stopTimer() method.)

You may want to crank up the number of threads if you truly anticipate needing to 'restart' the timer before the current timer expires:

private ScheduledExecutorService _Timer = Executors.newScheduledThreadPool(5);

You may want to go with a hybrid approach, keeping references to both the current TimerPopTask as I described and also to the current ScheduledFuture and make the best effort to cancel it and free up the thread if possible, understanding that it's not guaranteed to cancel.

(Note: this all assumes startTimer() and stopTimer() method calls are confined to a single main thread, and only the TimerPopTask instances are shared between threads. Otherwise you'll need additional safeguards.)

Scott Bale
  • 10,649
  • 5
  • 33
  • 36
  • good point. Let me ask you this, say a task was running. A new task is created, and scheduled to run TIMER_IN_SECONDS seconds later. When does this time (the TIMER_IN_SECONDS seconds) begin? Does it begin right then, when schedule() is called OR does it begin when there is a free thread in the thread pool? – rouble Jun 24 '10 at 03:56
  • The TIMER_IN_SECONDS delay starts 'now' right when schedule() is called. The task is guaranteed to start no sooner than the delay, however, it may start later (depending in part on thread availability). From the javadoc for ScheduledThreadPoolExecutor: "Delayed tasks execute no sooner than they are enabled, but without any real-time guarantees about when, after they are enabled, they will commence." – Scott Bale Jun 24 '10 at 14:42