This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 5b5cf8c8b7 Follow up to "Capture and send the final session delta..."
5b5cf8c8b7 is described below
commit 5b5cf8c8b7d83cd94bccb2ad34e62b4aa9432e6a
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 | 19 +++
2 files changed, 124 insertions(+), 58 deletions(-)
diff --git a/java/org/apache/catalina/ha/session/DeltaSession.java
b/java/org/apache/catalina/ha/session/DeltaSession.java
index fdd6f96eb0..b4bf203eb7 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;
@@ -188,17 +193,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();
}
}
@@ -669,32 +683,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
@@ -749,20 +785,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();
}
}
@@ -1006,21 +1052,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 ee64bcd30e..68a8d08bd8 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -223,6 +223,25 @@
<code>notifyLifecycleListenerOnFailure</code> is enabled. The event
was previously declared but never fired. (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
+ in <code>ReplicatedContext</code>, so that they are replicated to the
+ other nodes like attributes set later on rather than remaining local to
+ the node that started the application. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
