java: executors + tasks + locks

Question

Suppose I have an ExecutorService (which can be a thread pool, so there's concurrency involved) which executes a task at various times, either periodically or in response to some other condition. The task to be executed is the following:

• if this task is already in progress, do nothing (and let the previously-running task finish).
• if this task is not already in progress, run Algorithm X, which can take a long time.

I'm trying to think of a way to implement this. It should be something like:

Runnable task = new Runnable() {
   final SomeObj inProgress = new SomeObj();
   @Override public void run() {
       if (inProgress.acquire())
       {
          try
          {
             algorithmX();
          }
          finally
          {
             inProgress.release();
          }
       }
   }
}

// re-use this task object whenever scheduling the task with the executor

where SomeObj is either a ReentrantLock (acquire = tryLock() and release = unlock()) or an AtomicBoolean or something, but I'm not sure which. Do I need a ReentrantLock here? (Maybe I want a non-reentrant lock in case algorithmX() causes this task to be run recursively!) Or would an AtomicBoolean be enough?

---

edit: for a non-reentrant lock, is this appropriate?

Runnable task = new Runnable() {
   boolean inProgress = false;
   final private Object lock = new Object();
   /** try to acquire lock: set inProgress to true, 
    *  return whether it was previously false
    */ 
   private boolean acquire() {
      synchronized(this.lock)
      {
         boolean result = !this.inProgress;
         this.inProgress = true;
         return result;
      }
   }
   /** release lock */
   private void release() {
      synchronized(this.lock)
      {
         this.inProgress = false;
      }
   }
   @Override public void run() {
       if (acquire())
       {
          // nobody else is running! let's do algorithmX()
          try
          {
             algorithmX();
          }
          finally
          {
             release();
          }
       }
       /* otherwise, we are already in the process of 
        * running algorithmX(), in this thread or in another,
        * so don't do anything, just return control to the caller.
        */
   }
}

Answer 1

The lock implementation you suggest is weak in the sense that it would be quite easy for someone to use it improperly.

Below is a much more efficient implementation with the same improper use weaknesses as your implementation:

   AtomicBoolean inProgress = new AtomicBoolean(false)
   /* Returns true if we acquired the lock */
   private boolean acquire() {
       return inProgress.compareAndSet(false, true);
   }
   /** Always release lock without determining if we in fact hold it */
   private void release() {
       inProgress.set(false);
   }

Answer 2

Your first bit of code looks pretty good, but if you're worried about algorithmX recursively invoking the task, I would suggest you use a java.util.concurrent.Semaphore as the synchronization object, rather than a ReentrantLock. For example:

Runnable task = new Runnable() {
   final Semaphore lock = new Semaphore( 1 );
   @Override public void run() {
       if (lock.tryAcquire())
       {
          try
          {
             algorithmX();
          }
          finally
          {
             lock.release();
          }
       }
   }
}

Note in particular the use of tryacquire. If acquiring the lock fails, algorithmX is not run.