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 <[email protected]>
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: [email protected]
For additional commands, e-mail: [email protected]
