This is an automated email from the ASF dual-hosted git repository.
markt-asf 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 3877d858f4 Follow up to "Capture and send the final session delta..."
3877d858f4 is described below
commit 3877d858f4776c0c7632f5fc84fa6008dd1906a3
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Oct 2 15:04:58 2026 +0100
Follow up to "Capture and send the final session delta..."
Fix some more potential deadlocks.
- Notify attribute listeners outside of diff lock
- Process received diffs with a different DeltaRequest and lock
Co-authored-by: Claude Sonnet 5.5
---
.../apache/catalina/ha/session/DeltaSession.java | 163 +++++++++++++--------
webapps/docs/changelog.xml | 12 ++
2 files changed, 117 insertions(+), 58 deletions(-)
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java
b/java/org/apache/catalina/ha/session/DeltaSession.java
index 814298a07c..97f3aa6aad 100644
--- a/java/org/apache/catalina/ha/session/DeltaSession.java
+++ b/java/org/apache/catalina/ha/session/DeltaSession.java
@@ -31,7 +31,7 @@ import java.util.List;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.concurrent.locks.ReentrantLock;
import org.apache.catalina.Manager;
import org.apache.catalina.SessionListener;
@@ -84,9 +84,14 @@ public class DeltaSession extends StandardSession implements
Externalizable, Clu
/**
- * Write lock used to protect delta operations.
+ * Write lock used to protect local delta operations.
*/
- protected final Lock diffLock = new ReentrantReadWriteLock().writeLock();
+ protected final Lock diffLock = new ReentrantLock();
+
+ /**
+ * Lock used to protect delta receive operations
+ */
+ private final Lock receiveLock = new ReentrantLock();
private long version;
@@ -187,17 +192,26 @@ public class DeltaSession extends StandardSession
implements Externalizable, Clu
public void applyDiff(byte[] diff, int offset, int length) throws
IOException, ClassNotFoundException {
Thread currentThread = Thread.currentThread();
ClassLoader contextLoader = currentThread.getContextClassLoader();
- lockInternal();
- try (ObjectInputStream stream = ((ClusterManager)
getManager()).getReplicationStream(diff, offset, length)) {
- ClassLoader[] loaders = getClassLoaders();
- if (loaders != null && loaders.length > 0) {
- currentThread.setContextClassLoader(loaders[0]);
+ /*
+ * Listeners may be called so a separate DeltaRequest and lock are
used to avoid a possible deadlock with the
+ * session lock and diff lock.
+ */
+ receiveLock.lock();
+ try {
+ DeltaRequest request = obtainRequest();
+ try (ObjectInputStream stream = ((ClusterManager)
getManager()).getReplicationStream(diff, offset, length)) {
+ ClassLoader[] loaders = getClassLoaders();
+ if (loaders != null && loaders.length > 0) {
+ currentThread.setContextClassLoader(loaders[0]);
+ }
+ request.readExternal(stream);
+ request.execute(this, ((ClusterManager)
getManager()).isNotifyListenersOnReplication());
+ } finally {
+ currentThread.setContextClassLoader(contextLoader);
+ releaseRequest(request);
}
- deltaRequest.readExternal(stream);
- deltaRequest.execute(this, ((ClusterManager)
getManager()).isNotifyListenersOnReplication());
} finally {
- unlockInternal();
- currentThread.setContextClassLoader(contextLoader);
+ receiveLock.unlock();
}
}
@@ -666,32 +680,54 @@ public class DeltaSession extends StandardSession
implements Externalizable, Clu
*/
protected void deserializeAndExecuteDeltaRequest(byte[] delta) throws
IOException, ClassNotFoundException {
if (manager instanceof ClusterManagerBase) {
- SynchronizedStack<DeltaRequest> deltaRequestPool =
((ClusterManagerBase) manager).getDeltaRequestPool();
-
- DeltaRequest newDeltaRequest = deltaRequestPool.pop();
- if (newDeltaRequest == null) {
- newDeltaRequest = createRequest(null, ((ClusterManagerBase)
manager).isRecordAllActions());
+ /*
+ * Listeners may be called so a separate DeltaRequest and lock are
used to avoid a possible deadlock with
+ * the session lock and diff lock.
+ */
+ receiveLock.lock();
+ try {
+ DeltaRequest request = obtainRequest();
+ try {
+ ReplicationStream ois = ((ClusterManagerBase)
manager).getReplicationStream(delta);
+ request.readExternal(ois);
+ ois.close();
+ // Listeners may be called so the diff lock must not be
held
+ request.execute(this, ((ClusterManagerBase)
manager).isNotifyListenersOnReplication());
+ setPrimarySession(false);
+ } finally {
+ releaseRequest(request);
+ }
+ } finally {
+ receiveLock.unlock();
}
+ }
+ }
- ReplicationStream ois = ((ClusterManagerBase)
manager).getReplicationStream(delta);
- newDeltaRequest.readExternal(ois);
- ois.close();
- DeltaRequest oldDeltaRequest = null;
- lockInternal();
- try {
- oldDeltaRequest = replaceDeltaRequest(newDeltaRequest);
- newDeltaRequest.execute(this, ((ClusterManagerBase)
manager).isNotifyListenersOnReplication());
- setPrimarySession(false);
- } finally {
- unlockInternal();
- if (oldDeltaRequest != null) {
- oldDeltaRequest.reset();
- deltaRequestPool.push(oldDeltaRequest);
- }
+ /**
+ * Obtain a request, not associated with this session, to read and execute
received changes with.
+ */
+ private DeltaRequest obtainRequest() {
+ DeltaRequest request = null;
+ if (manager instanceof ClusterManagerBase cmb) {
+ request = cmb.getDeltaRequestPool().pop();
+ if (request == null) {
+ request = createRequest(null, cmb.isRecordAllActions());
}
+ } else {
+ request = createRequest();
}
+ return request;
}
+
+
+ private void releaseRequest(DeltaRequest request) {
+ if (manager instanceof ClusterManagerBase cmb) {
+ request.reset();
+ cmb.getDeltaRequestPool().push(request);
+ }
+ }
+
// ------------------------------------------------- HttpSession Properties
// ----------------------------------------------HttpSession Public Methods
@@ -746,20 +782,30 @@ public class DeltaSession extends StandardSession
implements Externalizable, Clu
return;
}
- lockInternal();
- try {
- super.setAttribute(name, value, notify);
- /*
- * It is possible that the session expires concurrently with the
attribute being added. Depending on the
- * exact timing, one of two things will happen. Either an
IllegalStateException will be thrown or the
- * attribute will be added and then immediately removed from the
session. The exception will be re-thrown.
- * If the attribute is removed, don't update the deltaRequest.
- */
- if (getAttribute(name) != null && addDeltaRequest &&
!exclude(name, value)) {
- deltaRequest.setAttribute(name, value);
+ /*
+ * The diff lock must not be held while listeners are called. They run
arbitrary application code that may
+ * need the session monitor and the session monitor is held when
listeners are called from expire().
+ */
+ super.setAttribute(name, value, notify);
+ if (addDeltaRequest) {
+ lockInternal();
+ try {
+ /*
+ * It is possible that the session expires concurrently with
the attribute being added. Depending on
+ * the exact timing, one of two things will happen. Either an
IllegalStateException will be thrown or
+ * the attribute will be added and then immediately removed
from the session. The exception will be
+ * re-thrown. If the attribute is removed, don't update the
deltaRequest.
+ *
+ * Record the current value, not the value passed in, so
concurrent updates of the same attribute
+ * cannot leave the replica with an older value than the
primary.
+ */
+ Object current = getAttribute(name);
+ if (current != null && !exclude(name, current)) {
+ deltaRequest.setAttribute(name, current);
+ }
+ } finally {
+ unlockInternal();
}
- } finally {
- unlockInternal();
}
}
@@ -1003,21 +1049,22 @@ public class DeltaSession extends StandardSession
implements Externalizable, Clu
* @param addDeltaRequest Whether to add a delta request entry
*/
protected void removeAttributeInternal(String name, boolean notify,
boolean addDeltaRequest) {
- lockInternal();
- try {
- // Remove this attribute from our collection
- Object value = attributes.get(name);
- if (value == null) {
- return;
- }
+ // Remove this attribute from our collection. Listeners are called
without the diff lock held.
+ if (attributes.get(name) == null) {
+ return;
+ }
- super.removeAttributeInternal(name, notify);
- if (addDeltaRequest && !exclude(name, null)) {
- deltaRequest.removeAttribute(name);
+ super.removeAttributeInternal(name, notify);
+ if (addDeltaRequest && !exclude(name, null)) {
+ lockInternal();
+ try {
+ // Don't record a removal if the attribute has since been set
again
+ if (attributes.get(name) == null) {
+ deltaRequest.removeAttribute(name);
+ }
+ } finally {
+ unlockInternal();
}
-
- } finally {
- unlockInternal();
}
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 4d59163791..7419d6b952 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -215,6 +215,18 @@
session attribute listener notifications, may in turn wait for the
session monitor. (remm)
</fix>
+ <fix>
+ Fix a possible deadlock in <code>DeltaSession</code>: session attribute
+ listeners are no longer called while the delta lock is held when
+ attributes are set or removed. The delta lock is only held while the
+ change is recorded. (markt)
+ </fix>
+ <fix>
+ Fix a possible deadlock in <code>DeltaSession</code>: received session
+ deltas are now read into a separate <code>DeltaRequest</code> from the
+ one the session uses to record local changes. This avoids having to
+ hold the delta lock while processing received changes. (markt)
+ </fix>
<fix>
Move context attributes that were set while the web application was
still starting into the replicated attribute map when it is installed
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
