Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to have a ReentrantLock with another way to determinate which thread will have the lock in the waiting queue. ReentrantLock implementation manages a Fair/Unfair policy, that's not what I want.

I would like to give a priority when I call the .lock() method, and this priority will determinate the order of current thread in the waiting threads queue.

I wrote a class, based on a PriorityBlockingQueue, and I'd like your opinion.

import java.util.Date;
import java.util.concurrent.PriorityBlockingQueue;

/**
 * Class to lock a resource. 
 * When multiple threads are waiting for the resource to be unlocked, the one with the greatest priority will have the resource.
 * When two threads have the same priority, the first added thread will lock the resource.
 *
 */
public class PrioritizedLock {

  private PriorityBlockingQueue<PrioritizedThread> waitingQueue = new PriorityBlockingQueue<PrioritizedThread>();

  private volatile PrioritizedThread activeThread;

  private volatile Object syncStartNext = new Object();

  private volatile boolean shutdown = false;

  public PrioritizedLock() {
    startQueueWatcher();
  }

  /**
   * Starts a queue watcher, which takes PrioritizedThread from the PriorityBlockingQueue and notify them to wake them up.
   */
  private void startQueueWatcher() {
    new Thread(new Runnable() {

      @Override
      public void run() {
        while (!shutdown) {

          /*
           * Wait until a PrioritizedThread is added to the PriorityBlockingQueue
           */
          while (activeThread == null) {
            try {
              activeThread = waitingQueue.take();
            } catch (InterruptedException e1) {
              //
            }
          }

          /*
           * Notify the thread to wake up (now it has locked the resource) 
           */
          synchronized (activeThread) {
            activeThread.notify();
          }

          /*
           * Wait until the resource has been released from the active thread
           */
          while (activeThread != null) {
            synchronized (syncStartNext) {
              try {
                syncStartNext.wait();
              } catch (InterruptedException e) {
                //
              }
            }
          }

        }
      }
    }).start();
  }

  /**
   * Waits until the thread is woken up by the Queue Watcher thread
   * @param thread
   */
  private void wait(PrioritizedThread thread) {
    synchronized (thread) {

      while (activeThread != thread) {
        try {
          thread.wait();
        } catch (InterruptedException e) {
          //
        }

      }
    }

  }

  static class PrioritizedThread implements Comparable<PrioritizedThread> {
    Thread thread;

    int priority;

    Date date = new Date();

    public PrioritizedThread(Thread aThread, int aPriority) {
      thread = aThread;
      priority = aPriority;
    }

    @Override
    public int compareTo(PrioritizedThread other) {
      int diffPriorities = this.priority - other.priority;

      if (diffPriorities == 0) {
        return date.compareTo(other.date);
      }
      return diffPriorities;
    }
  }

  /**
   * Waits until the resource is locked by the current thread
   * @param priority Priority of the lock, the less has the greatest priority
   */
  public void lock(int priority) {
    PrioritizedThread prioritizedThread = new PrioritizedThread(Thread.currentThread(), priority);

    waitingQueue.add(prioritizedThread);
    wait(prioritizedThread);
  }

  /**
   * Unlock the resource
   */
  public void unlock() {
    activeThread = null;

    synchronized (syncStartNext) {
      syncStartNext.notify();
    }
  }

  /**
   * Trigger a shutdown of the lock,
   */
  public void shutdown() {
    shutdown = true;
  }

}
share|improve this question

migrated from stackoverflow.com May 11 '12 at 12:54

This question came from our site for professional and enthusiast programmers.

1 Answer 1

After calling shutdown(), the shutdown boolean may never get checked. The Thread may be stuck in waitingQueue.take().

It's probably not a good idea to catch and swallow InterruptedException.

Considering those two points together, it makes sense to remove the Thread and use Executors.newSingleThreadExecutor() instead, and delegate to its shutdown() method.

Finally, it's fairly low-level and error prone to use wait() and notifyAll(). Using notify() is even riskier than notifyAll(). I'd recommend using a Condition instead.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.