Resource manager with ReentrantLocks

Question

I am trying to implement a resource handler class that assigns the resources (strings, stored in an array) to multiple clients that can try to acquire the lock on a set of resources and unlock them by an ID given by the lock method.

I'm trying to use fair ReentrantReadWriteLock-s, one for each resource.

I only see the log of the client.

There are several problems, sometimes a thread won't stop requesting and acquiring resources, sometimes deadlock happens, and sometimes the releaseLock fails. Any tips appreciated.

public class ResHandler {

//ID-s of the granted resource lists
private static long lockNum = 0;

//Resources are identified by strings, each client has a list of demanded resources
//we store these when granted, along with an ID
private static ConcurrentHashMap<Long, Set<String>> usedResources 
    = new ConcurrentHashMap<Long, Set<String>>();

//We store a lock for each resource
private static ConcurrentHashMap<String, ReentrantReadWriteLock> resources 
    = new ConcurrentHashMap<String, ReentrantReadWriteLock>();

//Filling our resources map with the resources and their locks
static {
    for (int i = 0; i < SharedValues.RESOURCE_LIST.length; ++i) {
        String res = SharedValues.RESOURCE_LIST[i];
        //Fair reentrant lock
        ReentrantReadWriteLock lc = new ReentrantReadWriteLock(true);
        resources.put(res, lc);
    }
}

//We get a set of the required resources and the type of lock we have to use
public static long getLock(Set<String> mNeededRes, boolean mMethod) {
    //!!!
    if (mMethod == SharedValues.READ_METHOD) {

        //We try to get the required resources
        for (String mn : mNeededRes)
            resources.get(mn).readLock().lock();

        //After grandted, we put them in the usedResources map
        ++lockNum;
        usedResources.put(lockNum, mNeededRes);
        return lockNum;         
    }

    //Same thing, but with write locks
    else {

        for (String mn : mNeededRes)
            resources.get(mn).writeLock().lock();

        ++lockNum;
        usedResources.put(lockNum, mNeededRes);
        return lockNum;         
    }
}

//Releasing a set of locks by the set's ID
public static void releaseLock(long mLockID) {
    if (!usedResources.containsKey(mLockID)) {
        System.out.println("returned, no such key as: " + mLockID);
        return; 
    }

    Set<String> toBeReleased = usedResources.get(mLockID);

    //Unlocking every lock from this set
    for (String s : toBeReleased) {
        if (resources.get(s).isWriteLockedByCurrentThread())
            resources.get(s).writeLock().unlock();
        else 
            resources.get(s).readLock().unlock();
    }

    //Deleting from the map
    usedResources.remove(mLockID);
}   
}

Answer 1

There are several issues going on in your program that are the reason for the lock-ups and errors:

• In general: declare the global variables final. You do not want to accidentally mess with them. In addition this allows you to use them as synchronization objects.

• long is not guaranteed to be atomic, nor is the operator ++. A 32-bit JVM has to write this in 2 steps and can therefore - theoretically - cause a major hick-up in your system. Better to use AtomicLong.

• The getLock is not Thread-safe. Example:

Thread A calls getLock for resource 1, 3, 5
Thread B calls getLock at the same time for resource 2,5,3
Thread A is granted lock on 1, 3, then it is put on pause
Thread B is granted lock on 2, 5 then is put on pause
Thread A now waits on 5 from Thread B, and Thread B now waits on 3 from Thread A.
Deadlock.

Note that the release method does not need to synchronize as it cannot lock any other Thread.

• ++lockNum will cause two threads to mess up their lock value if called at the same time as this is a global variable.

Here is the code in a working state:

  private static final AtomicLong lockNum = new AtomicLong(0);
  private static final ConcurrentHashMap<Long, Set<String>> usedResources = new ConcurrentHashMap<Long, Set<String>>();
  private static final ConcurrentHashMap<String, ReentrantReadWriteLock> resources = new ConcurrentHashMap<String, ReentrantReadWriteLock>();

  static {
    for (int i = 0; i < SharedValues.RESOURCE_LIST.length; ++i) {
      String res = SharedValues.RESOURCE_LIST[i];
      ReentrantReadWriteLock lc = new ReentrantReadWriteLock(true);
      resources.put(res, lc);
    }
  }

  public static long getLock(Set<String> mNeededRes, boolean mMethod) {
    synchronized (resources) {
      if (mMethod == SharedValues.READ_METHOD) {
        for (String mn : mNeededRes) {
          resources.get(mn).readLock().lock();
        }
      } else {
        for (String mn : mNeededRes) {
          resources.get(mn).writeLock().lock();
        }
      }
    }
    final long lockNumber = lockNum.getAndIncrement();
    usedResources.put(lockNumber, mNeededRes);
    return lockNumber;
  }

  public static void releaseLock(final long mLockID) {
    if (!usedResources.containsKey(mLockID)) {
      System.out.println("returned, no such key as: " + mLockID);
      return;
    }

    final Set<String> toBeReleased = usedResources.remove(mLockID);

    for (String s : toBeReleased) {
      final ReentrantReadWriteLock lock = resources.get(s);
      if (lock.isWriteLockedByCurrentThread()) {
        lock.writeLock().unlock();
      } else {
        lock.readLock().unlock();
      }
    }
  }

Answer 2

Updated solution, give it a try:

public class ResHandler {

private static AtomicLong lockNum = new AtomicLong(0);
private static Map<Long, Set<String>> usedResources = new ConcurrentHashMap<Long, Set<String>>();
private static final Map<String, ReentrantReadWriteLock> resources = new ConcurrentHashMap<String, ReentrantReadWriteLock>();
// "priorityResources" to avoid deadlocks and starvation
private static final Map<String, PriorityBlockingQueue<Long>> priorityResources = new ConcurrentHashMap<String, PriorityBlockingQueue<Long>>();

static {
    for (int i = 0; i < SharedValues.RESOURCE_LIST.length; ++i) {
        String res = SharedValues.RESOURCE_LIST[i];
        ReentrantReadWriteLock lc = new ReentrantReadWriteLock(true);
        resources.put(res, lc);
        priorityResources.put(res, new PriorityBlockingQueue<Long>());
    }
}

public static long getLock(Set<String> mNeededRes, boolean mMethod) {
    long lockNumLocal = lockNum.addAndGet(1);
    for (String mn : mNeededRes) {
        priorityResources.get(mn).offer(lockNumLocal);
    }
    boolean tryLockResult;
    List<String> lockedList = new ArrayList<String>();
    boolean allLocked = false;
    while (!allLocked) {
        allLocked = true;
        for (String mn : mNeededRes) {
            if (lockedList.contains(mn) == true) {
                continue;//because we already have the lock
            }
            try {
                if (mMethod == SharedValues.READ_METHOD) {
                    tryLockResult = resources.get(mn).readLock().tryLock(1, TimeUnit.MILLISECONDS);
                } else {
                    tryLockResult = resources.get(mn).writeLock().tryLock(1, TimeUnit.MILLISECONDS);
                }
            } catch (InterruptedException ex) {
                Logger.getLogger(ResHandler.class.getName()).log(Level.SEVERE, null, ex);
                tryLockResult = false;
            }

            if (tryLockResult) {
                lockedList.add(mn);
            } else {
                allLocked = false;
                for (int i = lockedList.size() - 1; i >= 0; i--) {
                    //if the lock failed, all previous locked resources need to be released, but only if they will be used by higher priority lock operations
                    if (priorityResources.get(lockedList.get(i)).peek() != lockNumLocal) {
                        if (mMethod == SharedValues.READ_METHOD) {
                            resources.get(lockedList.get(i)).readLock().unlock();
                        } else {
                            resources.get(lockedList.get(i)).writeLock().unlock();
                        }
                        lockedList.remove(i);
                    }
                }
                break;
            }
        }
    }
    usedResources.put(lockNumLocal, mNeededRes);
    for (String mn : mNeededRes) {
        priorityResources.get(mn).remove(lockNumLocal);
    }
    return lockNumLocal;
}

public static void releaseLock(long mLockID) {
    if (!usedResources.containsKey(mLockID)) {
        System.out.println("returned, no such key as: " + mLockID);
        return;
    }

    Set<String> toBeReleased = usedResources.get(mLockID);

    //Unlocking every lock from this set
    for (String s : toBeReleased) {
        if (resources.get(s).isWriteLockedByCurrentThread()) {
            resources.get(s).writeLock().unlock();
        } else {
            resources.get(s).readLock().unlock();
        }
    }

    //Deleting from the map
    usedResources.remove(mLockID);
}

}

Answer 3

I'm assuming that different clients can call getLock from different threads. If so then the first problem is that access to lockNum is not synchronised. Two threads could be calling getLock at the same time so depending on the timing, they may both end being returned the same lock number. This would explain why release lock sometimes fails.

If you can that problem fixed, it should be easier to work out what else is going on.

Answer 4

To avoid deadlock your resources must be acquired in the same order, so you have to sort Set<String> mNeededRes before performing locks in cycle. Sorting method does not really matter.

This is greatly described in Chapter10. Avoiding Liveness Hazards from Java Concurrency In Practice Brian Göetz.

Id recoment you to remove getLock and releaseLock or make them private. And wrap all you logic into a Runnable. If you controll all locks there is no way to forget release them. Make something like this:

public void performMethod(List<String> mNeededRes, boolean mMethod, Runnable r){
    List sortd = Collections.sort(mNeededRes);
    try{
        getLock(mNeededRes, mMethod);
        r.run();
    }finally {
        releaseLock(mNeededRes);
    }
}