From 55c8b6e4feb7368ff2044336210356ac25aad8a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mitchell=20B=C3=B6secke?= <mitchell.bosecke@forcorp.com>
Date: Tue, 12 Mar 2024 15:20:36 -0600
Subject: [PATCH] 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.
+ * <p>
+ * 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.
+ * <p>
+ * One objective of this class is to <a href="https://github.com/GeoWebCache/geowebcache/issues/1226">support
+ * nested locking scenarios</a>. 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:
+ * <pre>
+ *  lock(metatile);
+ *  try {
+ *      for(tile : metatile.getTiles()){
+ *          lock(tile);
+ *          try{
+ *              ... do work
+ *           } finally {
+ *               release(tile);
+ *          }
+ *      }
+ *  } finally {
+ *      release(metatile);
+ *  }
+ * </pre>
  *
  * @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<String, LockAndCounter> 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);
     }
 }