Question

I've got something like this:

private Striped<ReadWriteLock> stripes = Striped.lazyWeakReadWriteLock(STRIPES_AMOUNT);

private final Cache<Long, BlockingDeque<Peer>> peers = CacheBuilder.newBuilder()
    .expireAfterWrite(PEER_ACCESS_TIMEOUT_MIN, TimeUnit.MINUTES)
    .build();

Each time I proceed operation on cache, I lock it with help of stripes.

example#1 [write operation]

public void removePeers(long sessionId) {
    Lock lock = stripes.get(sessionId).writeLock();
    lock.lock();
    try {
        peers.invalidate(sessionId);
    } finally {
        lock.unlock();
    }
}

example#2 [read operation]

public BlockingDeque<Peer> getPeers(long sessionId) {
    Lock lock = stripes.get(sessionId).readLock();
    lock.lock();
    try {
        return peers.getIfPresent(sessionId);
    } finally {
        lock.unlock();
    }
}

example#3 [write operation]

public boolean addPeer(Peer peer) {
    long key = peer.getSessionId();
    Lock lock = stripes.get(key).writeLock();
    lock.lock();
    try {
        BlockingDeque<Peer> userPeers = peers.getIfPresent(key);
        if (userPeers == null) {
            userPeers = new LinkedBlockingDeque<Peer>();
            peers.put(key, userPeers);
        }
        return userPeers.offer(peer);
    } finally {
        lock.unlock();
    }
}

Question: What is the most efficient way to lock the method below?

/**
 * I should get the whole peers in cache
 */
public BlockingDeque<Peer> getAllPeers() {
    BlockingDeque<Peer> result = new LinkedBlockingDeque<Peer>();
    for (BlockingDeque<Peer> deque : peers.asMap().values()) {
        result.addAll(deque);
    }
    return result;
}
Was it helpful?

Solution

The most efficient way is not to lock at all :)

See my updated answer in Code Review: you don't need a ReadWriteLock, hence you don't need to lock the reads.

When you read (in Thread 1 for example), you get a snapshot of what's in the cache at that moment. If you have concurrent modifications (in Thread 2), even if you use locks, the content of the cache might have changed before Thread 1 finishes its computation and the lock doesn't buy you anything:

    Thread 1             Thread 2
       |                    |
   getAllPeers              |
       |                 addPeer
Do something with           |
the Peers but not           |
  the added one             |
       |                    |

So your implementation of getAllPeers() is fine as it is.


As a side note, if you need to lock all the stripes in a Striped<Lock>, you could do it using getAt(), but the naive way could get you into trouble if one of the lock() calls throws an unchecked exception (which is permitted):

for (int i = 0, size = stripes.size(); i++; i < size) {
    stripes.getAt(i).lock();
}
try {
    // Do something
} finally {
    for (int i = 0, size = stripes.size(); i++; i < size) {
        stripes.getAt(i).unlock();
    }
}

An alternative way would be to do it recursively, but it increases the length of the stack by the number of stripes, so you could get a StackOverflowException if you had a large number of stripes:

public void doSomethingWithLocks() {
     doSomethingWithLock(0);
}

private void doSomethingWithLock(int stripe) {
     if (stripe < stripes.size()) {
         Lock lock = stripes.getAt(stripe);
         lock.lock();
         try {
             doSomethingWithLock(stripe + 1);
         } finally {
             lock.unlock();
         }
     } else {
         doSomething();
     }
}

private void doSomething() {
    // Do something
}
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top