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 d74cebb0c5 Improve handling of concurrent expiration and attribute addition
d74cebb0c5 is described below
commit d74cebb0c5cf0331e1479b3fea3da6e2c278c2b7
Author: Mark Thomas <[email protected]>
AuthorDate: Thu Aug 6 19:50:02 2026 +0100
Improve handling of concurrent expiration and attribute addition
---
.../apache/catalina/session/StandardSession.java | 56 ++++--
.../catalina/ha/session/TestDeltaSession.java | 108 +++++++++++
.../catalina/session/TestStandardSession.java | 215 ++++++++++++++++++++-
webapps/docs/changelog.xml | 8 +
4 files changed, 372 insertions(+), 15 deletions(-)
diff --git a/java/org/apache/catalina/session/StandardSession.java b/java/org/apache/catalina/session/StandardSession.java
index 601c5d7405..55c2bd6080 100644
--- a/java/org/apache/catalina/session/StandardSession.java
+++ b/java/org/apache/catalina/session/StandardSession.java
@@ -660,12 +660,17 @@ public class StandardSession implements HttpSession, Session, Serializable {
}
}
- // We have completed expire of this session
- setValid(false);
- expiring = false;
+ String[] keys;
+ synchronized (attributes) {
+ // We have completed expire of this session
+ setValid(false);
+ expiring = false;
+
+ // Snapshot the attributes before permitting any racing setAttribute() call to observe the invalid state
+ keys = keys();
+ }
// Unbind any objects associated with this session
- String[] keys = keys();
ClassLoader oldContextClassLoader = null;
try {
oldContextClassLoader = context.bind(null);
@@ -999,11 +1004,13 @@ public class StandardSession implements HttpSession, Session, Serializable {
HttpSessionBindingEvent event = null;
// Call the valueBound() method if necessary
+ boolean valueBoundHasBeenCalled = false;
if (notify && value instanceof HttpSessionBindingListener) {
// Don't call any notification if replacing with the same value
// unless configured to do so
Object oldValue = attributes.get(name);
if (value != oldValue || manager.getNotifyBindingListenerOnUnchangedValue()) {
+ valueBoundHasBeenCalled = true;
event = new HttpSessionBindingEvent(getSession(), name, value);
try {
((HttpSessionBindingListener) value).valueBound(event);
@@ -1014,20 +1021,35 @@ public class StandardSession implements HttpSession, Session, Serializable {
}
// Replace or add this attribute
- Object unbound = attributes.put(name, value);
+ Object unbound = null;
+ boolean valid;
+ synchronized (attributes) {
+ valid = isValidInternal();
+ if (valid) {
+ unbound = attributes.put(name, value);
+ }
+ }
+
+ if (!valid) {
+ if (notify && value instanceof HttpSessionBindingListener) {
+ /*
+ * The session has expired since setAttribute() started. Although the attribute never made it as far as
+ * being added to the session, call valueUnbound() if valueBound() was called.
+ */
+ if (valueBoundHasBeenCalled) {
+ notifyAttributeUnbound(name, value);
+ }
+ return;
+ }
+ throw new IllegalStateException(sm.getString("standardSession.setAttribute.ise", getIdInternal()));
+ }
// Call the valueUnbound() method if necessary
if (notify && unbound instanceof HttpSessionBindingListener) {
// Don't call any notification if replacing with the same value
// unless configured to do so
if (unbound != value || manager.getNotifyBindingListenerOnUnchangedValue()) {
- try {
- ((HttpSessionBindingListener) unbound)
- .valueUnbound(new HttpSessionBindingEvent(getSession(), name));
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
- manager.getContext().getLogger().error(sm.getString("standardSession.bindingEvent"), t);
- }
+ notifyAttributeUnbound(name, unbound);
}
}
@@ -1081,6 +1103,16 @@ public class StandardSession implements HttpSession, Session, Serializable {
}
+ private void notifyAttributeUnbound(String name, Object value) {
+ try {
+ ((HttpSessionBindingListener) value).valueUnbound(new HttpSessionBindingEvent(getSession(), name));
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ manager.getContext().getLogger().error(sm.getString("standardSession.bindingEvent"), t);
+ }
+ }
+
+
// ------------------------------------------ HttpSession Protected Methods
/**
diff --git a/test/org/apache/catalina/ha/session/TestDeltaSession.java b/test/org/apache/catalina/ha/session/TestDeltaSession.java
new file mode 100644
index 0000000000..f5e43d1e5d
--- /dev/null
+++ b/test/org/apache/catalina/ha/session/TestDeltaSession.java
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.ha.session;
+
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+
+import jakarta.servlet.http.HttpSessionBindingEvent;
+import jakarta.servlet.http.HttpSessionBindingListener;
+import jakarta.servlet.http.HttpSessionEvent;
+import jakarta.servlet.http.HttpSessionListener;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Manager;
+import org.apache.catalina.core.StandardContext;
+import org.apache.catalina.session.StandardManager;
+
+public class TestDeltaSession {
+
+ @Test
+ public void testDeltaSessionBindingListenerAddedDuringExpiration() throws Exception {
+ StandardContext context = new StandardContext();
+ Manager manager = new StandardManager();
+ manager.setContext(context);
+
+ DeltaSession session = new DeltaSession(manager);
+ session.setValid(true);
+
+ CountDownLatch valueBoundEntered = new CountDownLatch(1);
+ CountDownLatch continueValueBound = new CountDownLatch(1);
+ CountDownLatch sessionDestroyedEntered = new CountDownLatch(1);
+ AtomicReference<Throwable> setAttributeException = new AtomicReference<>();
+ AtomicReference<Throwable> expireException = new AtomicReference<>();
+
+ context.setApplicationLifecycleListeners(new Object[] { new HttpSessionListener() {
+
+ @Override
+ public void sessionDestroyed(HttpSessionEvent se) {
+ sessionDestroyedEntered.countDown();
+ session.setAttribute("fromSessionDestroyed", "value");
+ }
+ } });
+
+ HttpSessionBindingListener listener = new HttpSessionBindingListener() {
+
+ @Override
+ public void valueBound(HttpSessionBindingEvent event) {
+ valueBoundEntered.countDown();
+ try {
+ continueValueBound.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ };
+
+ Thread setAttributeThread = new Thread(() -> {
+ try {
+ session.setAttribute("listener", listener);
+ } catch (Throwable t) {
+ setAttributeException.set(t);
+ }
+ });
+ setAttributeThread.setDaemon(true);
+ setAttributeThread.start();
+
+ Thread expireThread = new Thread(() -> {
+ try {
+ session.expire();
+ } catch (Throwable t) {
+ expireException.set(t);
+ }
+ });
+ expireThread.setDaemon(true);
+
+ try {
+ Assert.assertTrue(valueBoundEntered.await(10, TimeUnit.SECONDS));
+ expireThread.start();
+ Assert.assertTrue(sessionDestroyedEntered.await(10, TimeUnit.SECONDS));
+ } finally {
+ continueValueBound.countDown();
+ }
+ setAttributeThread.join(10000);
+ expireThread.join(10000);
+
+ Assert.assertFalse(setAttributeThread.isAlive());
+ Assert.assertFalse(expireThread.isAlive());
+ Assert.assertNull(setAttributeException.get());
+ Assert.assertNull(expireException.get());
+ }
+}
diff --git a/test/org/apache/catalina/session/TestStandardSession.java b/test/org/apache/catalina/session/TestStandardSession.java
index 8fbea2421f..a4990cc471 100644
--- a/test/org/apache/catalina/session/TestStandardSession.java
+++ b/test/org/apache/catalina/session/TestStandardSession.java
@@ -24,6 +24,13 @@ import java.io.ObjectOutputStream;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+
+import jakarta.servlet.http.HttpSessionBindingEvent;
+import jakarta.servlet.http.HttpSessionBindingListener;
import org.junit.Assert;
import org.junit.Test;
@@ -104,7 +111,7 @@ public class TestStandardSession {
StandardSession s1 = new StandardSession(TEST_MANAGER);
s1.setValid(true);
- Map<String, NonSerializable> value = new HashMap<>();
+ Map<String,NonSerializable> value = new HashMap<>();
value.put("key", new NonSerializable());
s1.setAttribute(nestedNonSerializableKey, value);
s1.setAttribute(serializableKey, serializableValue);
@@ -118,6 +125,160 @@ public class TestStandardSession {
}
+ @Test
+ public void testBindingListenerAddedDuringExpiration() throws Exception {
+ StandardSession session = new StandardSession(TEST_MANAGER);
+ session.setValid(true);
+
+ CountDownLatch valueBoundEntered = new CountDownLatch(1);
+ CountDownLatch continueValueBound = new CountDownLatch(1);
+ CountDownLatch valueUnboundCalled = new CountDownLatch(1);
+ AtomicReference<Throwable> setAttributeException = new AtomicReference<>();
+ HttpSessionBindingListener listener = new HttpSessionBindingListener() {
+
+ @Override
+ public void valueBound(HttpSessionBindingEvent event) {
+ valueBoundEntered.countDown();
+ try {
+ continueValueBound.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+
+ @Override
+ public void valueUnbound(HttpSessionBindingEvent event) {
+ valueUnboundCalled.countDown();
+ }
+ };
+
+ Thread setAttributeThread = new Thread(() -> {
+ try {
+ session.setAttribute("listener", listener);
+ } catch (Throwable t) {
+ setAttributeException.set(t);
+ }
+ });
+ setAttributeThread.start();
+
+ try {
+ Assert.assertTrue(valueBoundEntered.await(10, TimeUnit.SECONDS));
+ session.expire();
+ } finally {
+ continueValueBound.countDown();
+ }
+ setAttributeThread.join(10000);
+
+ // The attribute lost the race with expiration: setAttribute() must not throw and must not leave the
+ // listener bound without a matching valueUnbound() call.
+ Assert.assertFalse(setAttributeThread.isAlive());
+ Assert.assertNull(setAttributeException.get());
+ Assert.assertTrue(valueUnboundCalled.await(10, TimeUnit.SECONDS));
+ }
+
+
+ @Test
+ public void testBindingListenerUnchangedValueUnboundOnceDuringExpiration() throws Exception {
+ NotifyStallingManager manager = new NotifyStallingManager();
+ manager.setContext(new StandardContext());
+
+ StandardSession session = new StandardSession(manager);
+ session.setValid(true);
+
+ AtomicInteger boundCount = new AtomicInteger();
+ AtomicInteger unboundCount = new AtomicInteger();
+ HttpSessionBindingListener listener = new HttpSessionBindingListener() {
+
+ @Override
+ public void valueBound(HttpSessionBindingEvent event) {
+ boundCount.incrementAndGet();
+ }
+
+ @Override
+ public void valueUnbound(HttpSessionBindingEvent event) {
+ unboundCount.incrementAndGet();
+ }
+ };
+
+ // Bind normally first.
+ session.setAttribute("listener", listener);
+ Assert.assertEquals(1, boundCount.get());
+
+ // Re-setting the same reference under the same name skips valueBound() (the default
+ // getNotifyBindingListenerOnUnchangedValue() is false), but that decision reads
+ // Manager.getNotifyBindingListenerOnUnchangedValue(). Stall there so the session can
+ // expire - and legitimately unbind the listener - while this call is still in flight.
+ manager.stall = true;
+ Thread setAttributeThread = new Thread(() -> session.setAttribute("listener", listener));
+ setAttributeThread.start();
+
+ try {
+ Assert.assertTrue(manager.atStallPoint.await(10, TimeUnit.SECONDS));
+ session.expire();
+ } finally {
+ manager.releaseStall.countDown();
+ }
+ setAttributeThread.join(10000);
+
+ Assert.assertFalse(setAttributeThread.isAlive());
+ Assert.assertEquals(1, boundCount.get());
+ // valueUnbound() must fire exactly once: from expire()'s sweep. The racing setAttribute()
+ // call must not add a second, unmatched valueUnbound() since valueBound() was never called
+ // for it.
+ Assert.assertEquals(1, unboundCount.get());
+ }
+
+
+ @Test
+ public void testNonListenerReplacementRejectedAfterConcurrentExpiration() throws Exception {
+ DistributableStallingContext context = new DistributableStallingContext();
+ Manager manager = new StandardManager();
+ manager.setContext(context);
+
+ StandardSession session = new StandardSession(manager);
+ session.setValid(true);
+
+ AtomicInteger unboundCount = new AtomicInteger();
+ HttpSessionBindingListener oldListener = new HttpSessionBindingListener() {
+
+ @Override
+ public void valueUnbound(HttpSessionBindingEvent event) {
+ unboundCount.incrementAndGet();
+ }
+ };
+ session.setAttribute("x", oldListener);
+
+ // The replacement value ("plainValue") is not itself a listener, so this call takes the
+ // unguarded put() path. Stall inside the distributable check - evaluated for every
+ // setAttribute() call, before that path is chosen - so the session can expire while this
+ // call is still in flight.
+ context.stall = true;
+ AtomicReference<Throwable> setAttributeException = new AtomicReference<>();
+ Thread setAttributeThread = new Thread(() -> {
+ try {
+ session.setAttribute("x", "plainValue");
+ } catch (Throwable t) {
+ setAttributeException.set(t);
+ }
+ });
+ setAttributeThread.start();
+
+ try {
+ Assert.assertTrue(context.atStallPoint.await(10, TimeUnit.SECONDS));
+ session.expire();
+ } finally {
+ context.releaseStall.countDown();
+ }
+ setAttributeThread.join(10000);
+
+ Assert.assertFalse(setAttributeThread.isAlive());
+ Assert.assertEquals(1, unboundCount.get());
+ // setAttribute() started while the session was valid but the session expired before the
+ // replacement was applied. It must not silently succeed against an invalidated session.
+ Assert.assertTrue(setAttributeException.get() instanceof IllegalStateException);
+ }
+
+
private StandardSession serializeThenDeserialize(StandardSession source)
throws IOException, ClassNotFoundException {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
@@ -137,12 +298,12 @@ public class TestStandardSession {
int count = 0;
Enumeration<String> names = s1.getAttributeNames();
while (names.hasMoreElements()) {
- count ++;
+ count++;
String name = names.nextElement();
Object v1 = s1.getAttribute(name);
Object v2 = s2.getAttribute(name);
- Assert.assertEquals(v1, v2);
+ Assert.assertEquals(v1, v2);
}
Assert.assertEquals(expectedCount, count);
@@ -151,4 +312,52 @@ public class TestStandardSession {
private static class NonSerializable {
}
+
+
+ /*
+ * A Manager whose getNotifyBindingListenerOnUnchangedValue() blocks the calling thread on demand, to open a
+ * controlled race window at that exact point in StandardSession.setAttribute().
+ */
+ private static class NotifyStallingManager extends StandardManager {
+ private volatile boolean stall;
+ private final CountDownLatch atStallPoint = new CountDownLatch(1);
+ private final CountDownLatch releaseStall = new CountDownLatch(1);
+
+ @Override
+ public boolean getNotifyBindingListenerOnUnchangedValue() {
+ if (stall) {
+ atStallPoint.countDown();
+ try {
+ releaseStall.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ return super.getNotifyBindingListenerOnUnchangedValue();
+ }
+ }
+
+
+ /*
+ * A Context whose getDistributable() blocks the calling thread on demand, to open a controlled race window at that
+ * exact point in StandardSession.setAttribute().
+ */
+ private static class DistributableStallingContext extends StandardContext {
+ private volatile boolean stall;
+ private final CountDownLatch atStallPoint = new CountDownLatch(1);
+ private final CountDownLatch releaseStall = new CountDownLatch(1);
+
+ @Override
+ public boolean getDistributable() {
+ if (stall) {
+ atStallPoint.countDown();
+ try {
+ releaseStall.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ }
+ return super.getDistributable();
+ }
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 5757c8a9c2..7ea0c83305 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -167,6 +167,14 @@
attribute is not available or not configured for the current user.
(markt)
</fix>
+ <fix>
+ Improve handling of session attribute addition concurrent with session
+ expiration. An application will now either see a successful addition
+ followed by expiration or the addition will not succeed. It is no longer
+ possible for the session to expire and the addition to succeed. This is
+ of particular not for attributes that implement
+ <code>HttpSessionBindingListener</code>. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.