From 19b1dd9fb3db7138739bcf6314705808ff8d22f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mitchell=20B=C3=B6secke?= Date: Thu, 7 Mar 2024 15:16:32 -0700 Subject: [PATCH 1/2] Changed MemoryLockProvider to be unbounded and support nested locking scenarios. Solves #1226 --- .../geowebcache/locks/MemoryLockProvider.java | 76 ++++++++++++------- 1 file changed, 47 insertions(+), 29 deletions(-) diff --git a/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java b/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java index 87f07660c..e97cf3132 100644 --- a/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java +++ b/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java @@ -14,12 +14,12 @@ */ package org.geowebcache.locks; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Level; import java.util.logging.Logger; -import org.apache.commons.codec.digest.DigestUtils; import org.geotools.util.logging.Logging; -import org.geowebcache.GeoWebCacheException; /** * An in memory lock provider based on a striped lock @@ -30,48 +30,66 @@ public class MemoryLockProvider implements LockProvider { private static Logger LOGGER = Logging.getLogger(MemoryLockProvider.class.getName()); - java.util.concurrent.locks.Lock[] locks; - - public MemoryLockProvider() { - this(1024); - } - - public MemoryLockProvider(int concurrency) { - locks = new java.util.concurrent.locks.Lock[concurrency]; - for (int i = 0; i < locks.length; i++) { - locks[i] = new ReentrantLock(); - } - } + ConcurrentHashMap lockAndCounters = new ConcurrentHashMap<>(); @Override public Lock getLock(String lockKey) { - final int idx = getIndex(lockKey); - if (LOGGER.isLoggable(Level.FINE)) - LOGGER.fine("Mapped lock key " + lockKey + " to index " + idx + ". Acquiring lock."); - locks[idx].lock(); - if (LOGGER.isLoggable(Level.FINE)) - LOGGER.fine("Mapped lock key " + lockKey + " to index " + idx + ". Lock acquired"); + if (LOGGER.isLoggable(Level.FINE)) LOGGER.fine("Acquiring lock key " + lockKey); + + LockAndCounter lockAndCounter = + lockAndCounters.compute( + lockKey, + (key, existingLockAndCounter) -> { + if (existingLockAndCounter == null) { + existingLockAndCounter = new LockAndCounter(); + } + existingLockAndCounter.counter.incrementAndGet(); + return existingLockAndCounter; + }); + + lockAndCounter.lock.lock(); + + if (LOGGER.isLoggable(Level.FINE)) LOGGER.fine("Acquired lock key " + lockKey); return new Lock() { boolean released = false; @Override - public void release() throws GeoWebCacheException { + public void release() { if (!released) { released = true; - locks[idx].unlock(); - if (LOGGER.isLoggable(Level.FINE)) - LOGGER.fine("Released lock key " + lockKey + " mapped to index " + idx); + + LockAndCounter lockAndCounter = lockAndCounters.get(lockKey); + lockAndCounter.lock.unlock(); + + // Attempt to remove lock if no other thread is waiting for it + if (lockAndCounter.counter.decrementAndGet() == 0) { + + lockAndCounters.compute( + lockKey, + (key, existingLockAndCounter) -> { + if (existingLockAndCounter == null + || existingLockAndCounter.counter.get() == 0) { + return null; + } + return existingLockAndCounter; + }); + } + + if (LOGGER.isLoggable(Level.FINE)) LOGGER.fine("Released lock key " + lockKey); } } }; } - private int getIndex(String lockKey) { - // Simply hashing the lock key generated a significant number of collisions, - // doing the SHA1 digest of it provides a much better distribution - int idx = Math.abs(DigestUtils.sha1Hex(lockKey).hashCode() % locks.length); - return idx; + private static class LockAndCounter { + private final java.util.concurrent.locks.Lock lock = new ReentrantLock(); + + /** + * Track how many threads are waiting on this lock so we know if it's safe to remove it + * during a release. + */ + private final AtomicInteger counter = new AtomicInteger(0); } } From a60bcb2db3d5ee1906433cfe855bcf0b4d373367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mitchell=20B=C3=B6secke?= Date: Tue, 12 Mar 2024 15:20:36 -0600 Subject: [PATCH 2/2] Updated javadoc and comments on MemoryLockProvider --- .../geowebcache/locks/MemoryLockProvider.java | 54 ++++++++++++++----- 1 file changed, 42 insertions(+), 12 deletions(-) diff --git a/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java b/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java index e97cf3132..55a814c2a 100644 --- a/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java +++ b/geowebcache/core/src/main/java/org/geowebcache/locks/MemoryLockProvider.java @@ -14,21 +14,47 @@ */ package org.geowebcache.locks; +import org.geotools.util.logging.Logging; + import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.locks.ReentrantLock; import java.util.logging.Level; import java.util.logging.Logger; -import org.geotools.util.logging.Logging; /** - * An in memory lock provider based on a striped lock + * An in memory lock provider. + *

+ * This provider does not constrain the number of locks that can be held at any given time. + * Because any one thread can hold multiple locks at a time, a more appropriate approach + * to constraining resource usage would be to limit the number of concurrent threads instead. + *

+ * One objective of this class is to support + * nested locking scenarios. This class used to use a striped lock algorithm which + * would cause deadlocks for nested locking because of the non-predictable manner in + * which any lock can be arbitrarily locked by another unrelated lock. An example use case of + * nested locks, in pseudocode, would be: + * 

+ *  lock(metatile);
+ *  try {
+ *      for(tile : metatile.getTiles()){
+ *          lock(tile);
+ *          try{
+ *              ... do work
+ *           } finally {
+ *               release(tile);
+ *          }
+ *      }
+ *  } finally {
+ *      release(metatile);
+ *  }
+ *
* * @author Andrea Aime - GeoSolutions */ public class MemoryLockProvider implements LockProvider { - private static Logger LOGGER = Logging.getLogger(MemoryLockProvider.class.getName()); + private final static Logger LOGGER = Logging.getLogger(MemoryLockProvider.class.getName()); ConcurrentHashMap lockAndCounters = new ConcurrentHashMap<>(); @@ -36,15 +62,16 @@ public class MemoryLockProvider implements LockProvider { public Lock getLock(String lockKey) { if (LOGGER.isLoggable(Level.FINE)) LOGGER.fine("Acquiring lock key " + lockKey); + // Atomically create a new LockAndCounter, or increment the existing one LockAndCounter lockAndCounter = lockAndCounters.compute( lockKey, - (key, existingLockAndCounter) -> { - if (existingLockAndCounter == null) { - existingLockAndCounter = new LockAndCounter(); + (key, internalLockAndCounter) -> { + if (internalLockAndCounter == null) { + internalLockAndCounter = new LockAndCounter(); } - existingLockAndCounter.counter.incrementAndGet(); - return existingLockAndCounter; + internalLockAndCounter.counter.incrementAndGet(); + return internalLockAndCounter; }); lockAndCounter.lock.lock(); @@ -66,6 +93,8 @@ public void release() { // Attempt to remove lock if no other thread is waiting for it if (lockAndCounter.counter.decrementAndGet() == 0) { + // Try to remove the lock, but we have to check the count AGAIN inside of "compute" + // so that we know it hasn't been incremented since the if-statement above was evaluated lockAndCounters.compute( lockKey, (key, existingLockAndCounter) -> { @@ -83,13 +112,14 @@ public void release() { }; } + /** + * A ReentrantLock with a counter to track how many threads are waiting on this lock + * so we know if it's safe to remove it during a release. + */ private static class LockAndCounter { private final java.util.concurrent.locks.Lock lock = new ReentrantLock(); - /** - * Track how many threads are waiting on this lock so we know if it's safe to remove it - * during a release. - */ + // The count of threads holding or waiting for this lock private final AtomicInteger counter = new AtomicInteger(0); } }