tags:

views:

240

answers:

4

I have implemented a token system which assigns a fixed number of tokens. Each token on assignment starts up a Timer which expires after a few number of minutes and clears that token slot for reuse. If the user validates the token before the timer expires the Timer is supposed to be cancelled and reset with another token validity period. I seem to be unable to cancel the timer from outside the timer thread, is this behaviour expected. Snippets follow:

/**
 * Fills one of the available slots with a new session key
 * @param sessionKey
 * @return true on slot fill success - false on fail
 */
public boolean fillSlot(String sessionKey)
{
 if(count<MAXCOUNT)
 {
  //Add key to slot
  slots.add(sessionKey);
  //Up the key count
  upCount();
  //Set up expiry timer
  Timer timer = new Timer();
  timer.schedule(new ExpiringTokentask(timer,sessionKey), EXPIRY_TIME);
  timers.put(sessionKey, timer);
  return true;
 }
 return false;
}

    /**
 * Check if a given key is stored in the slots
 * reset timer every time key is checked
 * @param sessionKey
 * @return true on key found false on not found
 */
public boolean checkSlot(String sessionKey)
{
 //TODO: More efficient key search and storage for larger user sets
 //TODO: Upgrade from memory array to h2 embedded DB
 for(int i=0;i<slots.size();i++)
 {
  if(sessionKey.equals(slots.get(i)))
  {
   //Reset timer
   Timer timer = timers.get(sessionKey);
   //Can't seem to do this
   // timer.cancel();
   timer.schedule(new ExpiringTokentask(timer,sessionKey), EXPIRY_TIME);
   //Return token validation
   return true;
  }
 }

 return false;
}


private class ExpiringTokentask extends TimerTask
{
 private Timer timer;
 private String expireToken;

 public ExpiringTokentask(Timer timer, String sessionKey)
 {
  this.timer = timer;
  this.expireToken = sessionKey;
  System.out.println(sessionKey);
 }

 public void run() {
        System.out.format("Time's up!%n");
        clearSlot(expireToken);
        timer.cancel(); //Terminate the timer thread
    }
}
+1  A: 

If you timer is not null, you should be able to call cancel() from any thread. A better way of doing this is to use a ScheduledExecutorService, and get a Future for each task submitted. With the future, you can cancel it, or check the result.

brianegge
A: 

After the line where you try and cancel the timer, you should create a new one before calling the schedule method.

if(sessionKey.equals(slots.get(i)))
{
  //Reset timer
  Timer timer = timers.get(sessionKey);
  //Can't seem to do this
  timer.cancel();
  timer = new Timer();
  timer.schedule(new ExpiringTokentask(timer,sessionKey), EXPIRY_TIME);
  //Return token validation
  return true;
}
Aaron Chambers
+2  A: 

I believe that you can use one single timer object and create as many TimerTasks rather than creating many timers. Timers are costly, so using one or two timers for the entire application should suffice. Also, you are cancelling the timer and not the TimerTask try cancelling the TimerTask

Ram
A: 

As was said, you can cancel the TimerTask that was submitted to the timer instead of canceling the timer, this way you don't ever have to new up more timers.

What you are doing:

timer.cancel();
timer.schedule(...);

will throw IllegalStateExceptions since you CAN NOT schedule new tasks on a cancelled timer.

So instead of doing: timer.cancel() have your map be a mapping from session keys to TimerTasks and cancel the TimerTask instead of the Timer. This way you don't have to new up new timers and your timer will work as expected after canceling one or more of its tasks. You will also be able to use one timer to handle many sessions. Right now you are making one Timer and thus one thread per session.

On another note, you should NOT use java.util.Timer. What happens if any of your TimerTasks throws an exception? Your Timer will be killed and never run again! What if one of your TimerTasks is slow or blocks for an indefinite period? Any other TimerTasks on that timer will not be able to execute. Looking into using a SechduledThreadPoolExecutor instead. I'm sure that java.util.Timer will be deprecated by the next Java release.

mlaw
All of the above can happen but a peek at the timertask will tell you that it is not a large blocking operation. It simply removes an element from a list and exits. I might have trouble with lists locking up etc.
whatnick