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