This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 6d3a60be6e Improve handling of concurrent expiration and attribute
addition
6d3a60be6e is described below
commit 6d3a60be6e70e4c570ebb2966edc177f9a49873c
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 42c3b4a93a..27d200e114 100644
--- a/java/org/apache/catalina/session/StandardSession.java
+++ b/java/org/apache/catalina/session/StandardSession.java
@@ -676,12 +676,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(Globals.IS_SECURITY_ENABLED, null);
@@ -1069,11 +1074,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);
@@ -1084,20 +1091,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);
}
}
@@ -1152,6 +1174,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 162ab6475a..42c8127a91 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">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
