This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/11.0.x by this push:
     new e796ac9dde Refactor WebResource locking to use the new 
KeyedReentrantReadWriteLock
e796ac9dde is described below

commit e796ac9dde647d7d7f07b03c888b0f073257bf44
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Aug 27 12:24:27 2025 +0100

    Refactor WebResource locking to use the new KeyedReentrantReadWriteLock
---
 java/org/apache/catalina/WebResourceLockSet.java   | 30 +++++++
 .../catalina/webresources/DirResourceSet.java      | 98 ++++++++--------------
 .../apache/catalina/webresources/FileResource.java | 11 +--
 webapps/docs/changelog.xml                         |  4 +
 4 files changed, 73 insertions(+), 70 deletions(-)

diff --git a/java/org/apache/catalina/WebResourceLockSet.java 
b/java/org/apache/catalina/WebResourceLockSet.java
index 142472c33c..e6620561c3 100644
--- a/java/org/apache/catalina/WebResourceLockSet.java
+++ b/java/org/apache/catalina/WebResourceLockSet.java
@@ -17,6 +17,7 @@
 package org.apache.catalina;
 
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReadWriteLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 /**
@@ -24,6 +25,17 @@ import java.util.concurrent.locks.ReentrantReadWriteLock;
  */
 public interface WebResourceLockSet {
 
+    /**
+     * Obtain a reentrant read/write lock for the resource at the provided 
path. The resource is not required to exist.
+     * Multiple calls to this method with the same path will return the same 
lock provided that at least one instance
+     * of the lock remains in use between the calls.
+     *
+     * @param path The path for which the lock should be obtained
+     *
+     * @return A reentrant read/write lock for the given resource.
+     */
+    ReadWriteLock getLock(String path);
+
     /**
      * Lock the resource at the provided path for reading. The resource is not 
required to exist. Read locks are not
      * exclusive.
@@ -31,7 +43,10 @@ public interface WebResourceLockSet {
      * @param path The path to the resource to be locked for reading
      *
      * @return The {@link ResourceLock} that must be passed to {@link 
#unlockForRead(ResourceLock)} to release the lock
+     *
+     * @deprecated Unused. Will be removed in Tomcat 12 onwards. Use {@code 
#getLock(String)} instead.
      */
+    @Deprecated
     ResourceLock lockForRead(String path);
 
     /**
@@ -39,7 +54,10 @@ public interface WebResourceLockSet {
      *
      * @param resourceLock The {@link ResourceLock} associated with the 
resource for which a read lock should be
      *                         released
+     *
+     * @deprecated Unused. Will be removed in Tomcat 12 onwards. Use {@code 
#getLock(String)} instead.
      */
+    @Deprecated
     void unlockForRead(ResourceLock resourceLock);
 
     /**
@@ -49,7 +67,10 @@ public interface WebResourceLockSet {
      * @param path The path to the resource to be locked for writing
      *
      * @return The {@link ResourceLock} that must be passed to {@link 
#unlockForWrite(ResourceLock)} to release the lock
+     *
+     * @deprecated Unused. Will be removed in Tomcat 12 onwards. Use {@code 
#getLock(String)} instead.
      */
+    @Deprecated
     ResourceLock lockForWrite(String path);
 
     /**
@@ -57,10 +78,19 @@ public interface WebResourceLockSet {
      *
      * @param resourceLock The {@link ResourceLock} associated with the 
resource for which the write lock should be
      *                         released
+     *
+     * @deprecated Unused. Will be removed in Tomcat 12 onwards. Use {@code 
#getLock(String)} instead.
      */
+    @Deprecated
     void unlockForWrite(ResourceLock resourceLock);
 
 
+    /**
+     * Represents a lock on a resource.
+     *
+     * @deprecated Unused. Will be removed in Tomcat 12 onwards.
+     */
+    @Deprecated
     class ResourceLock {
         public final AtomicInteger count = new AtomicInteger(0);
         public final ReentrantReadWriteLock reentrantLock = new 
ReentrantReadWriteLock();
diff --git a/java/org/apache/catalina/webresources/DirResourceSet.java 
b/java/org/apache/catalina/webresources/DirResourceSet.java
index c4f583dc39..83eacf7ad0 100644
--- a/java/org/apache/catalina/webresources/DirResourceSet.java
+++ b/java/org/apache/catalina/webresources/DirResourceSet.java
@@ -22,11 +22,11 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.StandardCopyOption;
-import java.util.HashMap;
 import java.util.Locale;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
 import java.util.jar.Manifest;
 
 import org.apache.catalina.LifecycleException;
@@ -38,6 +38,7 @@ import org.apache.catalina.util.ResourceSet;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 import org.apache.tomcat.util.compat.JreCompat;
+import org.apache.tomcat.util.concurrent.KeyedReentrantReadWriteLock;
 import org.apache.tomcat.util.http.RequestUtil;
 
 /**
@@ -47,8 +48,7 @@ public class DirResourceSet extends AbstractFileResourceSet 
implements WebResour
 
     private static final Log log = LogFactory.getLog(DirResourceSet.class);
 
-    private final Map<String,ResourceLock> resourceLocksByPath = new 
HashMap<>();
-    private final Object resourceLocksByPathLock = new Object();
+    private KeyedReentrantReadWriteLock resourceLocksByPath = new 
KeyedReentrantReadWriteLock();
 
 
     /**
@@ -96,7 +96,6 @@ public class DirResourceSet extends AbstractFileResourceSet 
implements WebResour
     }
 
 
-    @SuppressWarnings("null") // lock can never be null when lock.key is read
     @Override
     public WebResource getResource(String path) {
         checkPath(path);
@@ -109,7 +108,11 @@ public class DirResourceSet extends 
AbstractFileResourceSet implements WebResour
              * and writes (e.g. HTTP GET and PUT / DELETE) for the same path 
causing corruption of the FileResource
              * where some of the fields are set as if the file exists and some 
as set as if it does not.
              */
-            ResourceLock lock = readOnly ? null : lockForRead(path);
+            Lock readLock = null;
+            if (!readOnly) {
+                readLock = getLock(path).readLock();
+                readLock.lock();
+            }
             try {
                 File f = file(path.substring(webAppMount.length()), false);
                 if (f == null) {
@@ -121,10 +124,10 @@ public class DirResourceSet extends 
AbstractFileResourceSet implements WebResour
                 if (f.isDirectory() && path.charAt(path.length() - 1) != '/') {
                     path = path + '/';
                 }
-                return new FileResource(root, path, f, readOnly, 
getManifest(), this, readOnly ? null : lock.key);
+                return new FileResource(root, path, f, readOnly, 
getManifest(), this, readOnly ? null : path);
             } finally {
-                if (!readOnly) {
-                    unlockForRead(lock);
+                if (readLock != null) {
+                    readLock.unlock();
                 }
             }
         } else {
@@ -282,7 +285,8 @@ public class DirResourceSet extends AbstractFileResourceSet 
implements WebResour
          * HTTP GET and PUT / DELETE) for the same path causing corruption of 
the FileResource where some of the fields
          * are set as if the file exists and some as set as if it does not.
          */
-        ResourceLock lock = lockForWrite(path);
+        Lock writeLock = getLock(path).writeLock();
+        writeLock.lock();
         try {
             dest = file(path.substring(webAppMount.length()), false);
             if (dest == null) {
@@ -305,7 +309,7 @@ public class DirResourceSet extends AbstractFileResourceSet 
implements WebResour
 
             return true;
         } finally {
-            unlockForWrite(lock);
+            writeLock.unlock();
         }
     }
 
@@ -373,78 +377,42 @@ public class DirResourceSet extends 
AbstractFileResourceSet implements WebResour
     }
 
 
+
+    @Override
+    public ReadWriteLock getLock(String path) {
+        String key = getLockKey(path);
+        return resourceLocksByPath.getLock(key);
+    }
+
+
+    @SuppressWarnings("deprecation")
     @Override
     public ResourceLock lockForRead(String path) {
         String key = getLockKey(path);
-        ResourceLock resourceLock;
-        synchronized (resourceLocksByPathLock) {
-            /*
-             * Obtain the ResourceLock and increment the usage count inside 
the sync to ensure that that map always has
-             * a consistent view of the currently "in-use" ResourceLocks.
-             */
-            resourceLock = resourceLocksByPath.get(key);
-            if (resourceLock == null) {
-                resourceLock = new ResourceLock(key);
-                resourceLocksByPath.put(key, resourceLock);
-            }
-            resourceLock.count.incrementAndGet();
-        }
-        // Obtain the lock outside the sync as it will block if there is a 
current write lock.
-        resourceLock.reentrantLock.readLock().lock();
-        return resourceLock;
+        resourceLocksByPath.getLock(key).readLock().lock();
+        return new ResourceLock(key);
     }
 
 
+    @SuppressWarnings("deprecation")
     @Override
     public void unlockForRead(ResourceLock resourceLock) {
-        // Unlock outside the sync as there is no need to do it inside.
-        resourceLock.reentrantLock.readLock().unlock();
-        synchronized (resourceLocksByPathLock) {
-            /*
-             * Decrement the usage count and remove ResourceLocks no longer 
required inside the sync to ensure that that
-             * map always has a consistent view of the currently "in-use" 
ResourceLocks.
-             */
-            if (resourceLock.count.decrementAndGet() == 0) {
-                resourceLocksByPath.remove(resourceLock.key);
-            }
-        }
+        resourceLocksByPath.getLock(resourceLock.key).readLock().unlock();
     }
 
 
+    @SuppressWarnings("deprecation")
     @Override
     public ResourceLock lockForWrite(String path) {
         String key = getLockKey(path);
-        ResourceLock resourceLock;
-        synchronized (resourceLocksByPathLock) {
-            /*
-             * Obtain the ResourceLock and increment the usage count inside 
the sync to ensure that that map always has
-             * a consistent view of the currently "in-use" ResourceLocks.
-             */
-            resourceLock = resourceLocksByPath.get(key);
-            if (resourceLock == null) {
-                resourceLock = new ResourceLock(key);
-                resourceLocksByPath.put(key, resourceLock);
-            }
-            resourceLock.count.incrementAndGet();
-        }
-        // Obtain the lock outside the sync as it will block if there are any 
other current locks.
-        resourceLock.reentrantLock.writeLock().lock();
-        return resourceLock;
+        resourceLocksByPath.getLock(key).writeLock().lock();
+        return new ResourceLock(key);
     }
 
 
+    @SuppressWarnings("deprecation")
     @Override
     public void unlockForWrite(ResourceLock resourceLock) {
-        // Unlock outside the sync as there is no need to do it inside.
-        resourceLock.reentrantLock.writeLock().unlock();
-        synchronized (resourceLocksByPathLock) {
-            /*
-             * Decrement the usage count and remove ResourceLocks no longer 
required inside the sync to ensure that that
-             * map always has a consistent view of the currently "in-use" 
ResourceLocks.
-             */
-            if (resourceLock.count.decrementAndGet() == 0) {
-                resourceLocksByPath.remove(resourceLock.key);
-            }
-        }
+        resourceLocksByPath.getLock(resourceLock.key).writeLock().unlock();
     }
 }
diff --git a/java/org/apache/catalina/webresources/FileResource.java 
b/java/org/apache/catalina/webresources/FileResource.java
index faf6138744..e57c43a16a 100644
--- a/java/org/apache/catalina/webresources/FileResource.java
+++ b/java/org/apache/catalina/webresources/FileResource.java
@@ -29,10 +29,10 @@ import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.security.cert.Certificate;
+import java.util.concurrent.locks.Lock;
 import java.util.jar.Manifest;
 
 import org.apache.catalina.WebResourceLockSet;
-import org.apache.catalina.WebResourceLockSet.ResourceLock;
 import org.apache.catalina.WebResourceRoot;
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
@@ -134,15 +134,16 @@ public class FileResource extends AbstractResource {
          * HTTP GET and PUT / DELETE) for the same path causing corruption of 
the FileResource where some of the fields
          * are set as if the file exists and some as set as if it does not.
          */
-        ResourceLock lock = null;
+        Lock writeLock = null;
         if (lockSet != null) {
-            lock = lockSet.lockForWrite(lockKey);
+            writeLock = lockSet.getLock(lockKey).writeLock();
+            writeLock.lock();
         }
         try {
             return resource.delete();
         } finally {
-            if (lockSet != null) {
-                lockSet.unlockForWrite(lock);
+            if (writeLock != null) {
+                writeLock.unlock();
             }
         }
     }
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 939a1bbec0..6f6258e87b 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -117,6 +117,10 @@
         when the store was used with the <code>PersistentValve</code>. Based on
         pull request <pr>882</pr> by Aaron Ogburn. (markt)
       </fix>
+      <scode>
+        Refactor <code>WebResource</code> locking to use the new
+        <code>KeyedReentrantReadWriteLock</code>. (markt)
+      </scode>
     </changelog>
   </subsection>
   <subsection name="Coyote">


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Reply via email to