java: executors + tasks + locks

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.
        */
   }
}

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);
   }

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.

ReentrantLock seems fine to me. The only situation where I'd find interesting to manually create a lock using AtomicInteger will be if you have a really short algorithmX which is not your case.

I think the secret of choosing the right lock impl is this: * if this task is already in progress, do nothing (and let the previously-running task finish).

What does "do nothing" mean in this context? Thread should block and retry execution after running algorithmX is finished?. If this is the case semaphore.acquire instead of tryAcquire should be used and AtomicBoolean solution won't work as expected.