From 9fba0861c79dfcad5c361df033c07a3002fee4fc Mon Sep 17 00:00:00 2001 From: Markus Jung Date: Thu, 23 Jul 2026 21:05:23 +0200 Subject: [PATCH 1/2] TOMEE-4652 - roll back UserTransaction left over by a request A servlet or JSP that leaves a bean managed UserTransaction incomplete leaks that transaction to the next request served on the same pooled Tomcat exec thread. Geronimo's TransactionManagerImpl keeps the thread-to-transaction association (and the per-thread transaction timeout) in ThreadLocals that are only cleared on commit()/rollback(). EJBs are wrapped by container interceptors that restore the thread state; plain servlets have no equivalent, and OpenEJBValve's finally block cleaned up only the security context. Add TransactionCleanup, invoked from the request teardown finally in OpenEJBValve (sync path) and OpenEJBSecurityListener.asyncExit() (async complete/error/timeout). It rolls back and unassociates any dangling transaction and resets the per-thread transaction timeout, which leaks the same way. Also make CoreUserTransaction.resetError(null) remove() the ThreadLocal instead of set(null) so pooled threads don't keep an empty entry pinned. Adds UserTransactionLeakTest, which forces two sequential requests onto one exec thread (maxThreads=1) and asserts the second sees no leaked transaction. --- .../openejb/core/CoreUserTransaction.java | 6 +- .../catalina/OpenEJBSecurityListener.java | 6 +- .../apache/tomee/catalina/OpenEJBValve.java | 1 + .../tomee/catalina/TransactionCleanup.java | 80 +++++++++++ .../embedded/UserTransactionLeakTest.java | 135 ++++++++++++++++++ 5 files changed, 226 insertions(+), 2 deletions(-) create mode 100644 tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java create mode 100644 tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java diff --git a/container/openejb-core/src/main/java/org/apache/openejb/core/CoreUserTransaction.java b/container/openejb-core/src/main/java/org/apache/openejb/core/CoreUserTransaction.java index 5d917657286..b8e27ee3a92 100644 --- a/container/openejb-core/src/main/java/org/apache/openejb/core/CoreUserTransaction.java +++ b/container/openejb-core/src/main/java/org/apache/openejb/core/CoreUserTransaction.java @@ -51,7 +51,11 @@ public static RuntimeException error(final RuntimeException e) { } public static void resetError(final RuntimeException old) { - ERROR.set(old); + if (old == null) { // don't pin an empty entry to the thread, these are pooled + ERROR.remove(); + } else { + ERROR.set(old); + } } public static RuntimeException error() { diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java index 45cf0e9d430..345bdda524a 100644 --- a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java @@ -64,7 +64,11 @@ private void asyncExit() { try { exit(); } finally { - requests.remove(); + try { + requests.remove(); + } finally { + TransactionCleanup.clean(); + } } } diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java index 64b58ebf2b1..361a848b975 100644 --- a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java @@ -45,6 +45,7 @@ public void invoke(final Request request, final Response response) throws IOExce getNext().invoke(request, response); } finally { listener.exit(); + TransactionCleanup.clean(); } } else { request.getAsyncContextInternal().addListener(new OpenEJBSecurityListener(securityService, request)); diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java new file mode 100644 index 00000000000..05e746a91ea --- /dev/null +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java @@ -0,0 +1,80 @@ +/* + * 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.tomee.catalina; + +import org.apache.openejb.loader.SystemInstance; +import org.apache.openejb.util.LogCategory; +import org.apache.openejb.util.Logger; + +import jakarta.transaction.Status; +import jakarta.transaction.Transaction; +import jakarta.transaction.TransactionManager; + +/** + * Rolls back and unassociates any transaction a request left behind on the worker thread. + * + * A servlet or JSP using a bean managed {@link jakarta.transaction.UserTransaction} is not + * wrapped by a container interceptor, so nothing restores the thread state when the request + * ends. Since Tomcat pools its exec threads, a transaction still associated at that point is + * inherited by whichever request is served next on the same thread, which then sees a bogus + * transaction status. The per thread transaction timeout set via + * {@link TransactionManager#setTransactionTimeout(int)} leaks the same way. + * + * @see TOMEE-4652 + */ +public final class TransactionCleanup { + private static final Logger LOGGER = Logger.getInstance(LogCategory.TRANSACTION, TransactionCleanup.class); + + private TransactionCleanup() { + // no-op + } + + /** + * Restores the calling thread to a state with no transaction associated to it. Any dangling + * transaction is rolled back since the request that started it can no longer complete it. + */ + public static void clean() { + final TransactionManager transactionManager = SystemInstance.get().getComponent(TransactionManager.class); + if (transactionManager == null) { + return; + } + + try { + final Transaction transaction = transactionManager.getTransaction(); + if (transaction != null && transaction.getStatus() != Status.STATUS_NO_TRANSACTION) { + LOGGER.warning("Request ended with an active transaction " + transaction + + ", rolling it back to avoid leaking it to the next request on this thread"); + transactionManager.rollback(); + } + } catch (final Throwable t) { + LOGGER.error("Failed to roll back the transaction left over by this request", t); + // the rollback failed, but the association must not survive this request either + try { + transactionManager.suspend(); + } catch (final Throwable suspendFailure) { + LOGGER.error("Failed to unassociate the transaction left over by this request", suspendFailure); + } + } + + // begin() only resets this once a transaction is actually started, so reset it explicitly + try { + transactionManager.setTransactionTimeout(0); + } catch (final Throwable t) { + LOGGER.error("Failed to reset the transaction timeout left over by this request", t); + } + } +} diff --git a/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java b/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java new file mode 100644 index 00000000000..ffab520e2b8 --- /dev/null +++ b/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java @@ -0,0 +1,135 @@ +/* + * 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.tomee.embedded; + +import org.apache.openejb.loader.IO; +import org.apache.openejb.util.NetworkUtil; +import org.junit.Test; + +import jakarta.annotation.Resource; +import jakarta.servlet.ServletException; +import jakarta.servlet.annotation.WebServlet; +import jakarta.servlet.http.HttpServlet; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import jakarta.transaction.Status; +import jakarta.transaction.UserTransaction; +import java.io.IOException; +import java.net.URL; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * A request that leaves a UserTransaction behind must not poison the next request served on the + * same pooled Tomcat exec thread. + * + * @see TOMEE-4652 + */ +public class UserTransactionLeakTest { + @Test + public void transactionDoesNotLeakToNextRequest() throws IOException { + try (final Container c = new Container(new Configuration() + .http(NetworkUtil.getNextAvailablePort()) + // a single exec thread guarantees both requests land on the same thread + .property("connector.attributes.maxThreads", "1") + .property("connector.attributes.minSpareThreads", "1") + .property("openejb.additional.include", "tomee-")) + .deployClasspathAsWebApp()) { + + final String base = "http://localhost:" + c.getConfiguration().getHttpPort(); + + // the leaker: begins a transaction and never completes it + final String leaker = IO.slurp(new URL(base + "/leak")); + assertTrue(leaker, leaker.startsWith("begun")); + + // the victim: must see a clean thread, not the transaction above + final String victim = IO.slurp(new URL(base + "/status")); + + // guard: the whole point is thread reuse, so fail loudly if that did not happen + assertEquals("both requests must share the exec thread for this test to mean anything", + threadOf(leaker), threadOf(victim)); + + assertEquals("STATUS_NO_TRANSACTION", victim.substring(0, victim.indexOf(" on "))); + + // and must still be able to run a transaction of its own + assertEquals("committed", IO.slurp(new URL(base + "/commit"))); + } + } + + private static String threadOf(final String response) { + final int marker = response.indexOf(" on "); + assertTrue("no thread name in response: " + response, marker > 0); + final String rest = response.substring(marker + " on ".length()); + final int end = rest.indexOf(' '); + return end < 0 ? rest : rest.substring(0, end); + } + + @WebServlet(urlPatterns = "/leak", loadOnStartup = 1) + public static class Leaker extends HttpServlet { + @Resource + private UserTransaction ut; + + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) throws ServletException, IOException { + try { + ut.setTransactionTimeout(120); // must not leak to the next request either + ut.begin(); + resp.getWriter().write("begun on " + Thread.currentThread().getName()); + } catch (final Exception e) { + resp.getWriter().write("failed: " + e.getMessage()); + } + } + } + + @WebServlet(urlPatterns = "/status", loadOnStartup = 1) + public static class StatusReporter extends HttpServlet { + @Resource + private UserTransaction ut; + + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) throws ServletException, IOException { + try { + final int status = ut.getStatus(); + resp.getWriter().write((status == Status.STATUS_NO_TRANSACTION + ? "STATUS_NO_TRANSACTION" : "leaked status " + status) + + " on " + Thread.currentThread().getName()); + } catch (final Exception e) { + resp.getWriter().write("failed: " + e.getMessage()); + } + } + } + + @WebServlet(urlPatterns = "/commit", loadOnStartup = 1) + public static class Committer extends HttpServlet { + @Resource + private UserTransaction ut; + + @Override + protected void service(final HttpServletRequest req, final HttpServletResponse resp) throws ServletException, IOException { + try { + // would throw NotSupportedException ("Nested Transactions are not supported") + // if the leaked transaction were still associated with this thread + ut.begin(); + ut.commit(); + resp.getWriter().write("committed"); + } catch (final Exception e) { + resp.getWriter().write("failed: " + e.getClass().getSimpleName() + " " + e.getMessage()); + } + } + } +} From d218410fd7f06e774515f87ac9d185e790ed8c85 Mon Sep 17 00:00:00 2001 From: Markus Jung Date: Tue, 28 Jul 2026 21:35:06 +0200 Subject: [PATCH 2/2] TOMEE-4652 - move transaction cleanup to the Host pipeline Review feedback on the placement, which was both documented backwards and covering less than it claimed. StandardContextValve has no fireRequest* call at all; StandardHostValve fires requestInitEvent, invokes the Context pipeline, and only then runs throwable()/status() and fires requestDestroyEvent. So cleaning up from OpenEJBValve (Context pipeline) ran *before* requestDestroyed rather than after it, pre-empting applications that complete their transaction there, and left servlets and JSPs leaking exactly as before since they are dispatched through an include rather than a Pipeline. Move the call to OpenEJBSecurityListener.RequestCapturer, which is on the Host pipeline and wraps all of StandardHostValve#invoke, and correct the javadoc to describe the real ordering. This also drops the call from OpenEJBValve entirely, so the reported "skipped when listener.exit() throws" hole goes away with it. Also from review: - don't log an ERROR when the leftover transaction was already rolled back or is rolling back; unassociate it at debug instead - explain in resetTimeout() why pinning the timeout ThreadLocal is the right tradeoff here even though resetError() avoids pinning: the timeout cannot be read back, and a null value cannot pin a classloader - actually assert the timeout does not leak. The Leaker set a 120s timeout with a comment saying it must not leak and nothing checked it. Committer now reads the effective timeout of its own transaction and the test distinguishes the leaked 120s from the 600s default. Verified this assertion fails when the cleanup is removed. --- .../catalina/OpenEJBSecurityListener.java | 8 +++- .../apache/tomee/catalina/OpenEJBValve.java | 1 - .../tomee/catalina/TransactionCleanup.java | 43 +++++++++++++++++-- .../embedded/UserTransactionLeakTest.java | 31 +++++++++++-- 4 files changed, 74 insertions(+), 9 deletions(-) diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java index 345bdda524a..d5f428c7ed8 100644 --- a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBSecurityListener.java @@ -100,7 +100,13 @@ public void invoke(final Request request, final Response response) throws IOExce try { getNext().invoke(request, response); } finally { - requests.remove(); + try { + requests.remove(); + } finally { + // on the Host pipeline, so this wraps requestDestroyed and the error page + // handling in StandardHostValve too, see TransactionCleanup + TransactionCleanup.clean(); + } } } } diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java index 361a848b975..64b58ebf2b1 100644 --- a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/OpenEJBValve.java @@ -45,7 +45,6 @@ public void invoke(final Request request, final Response response) throws IOExce getNext().invoke(request, response); } finally { listener.exit(); - TransactionCleanup.clean(); } } else { request.getAsyncContextInternal().addListener(new OpenEJBSecurityListener(securityService, request)); diff --git a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java index 05e746a91ea..727516858e0 100644 --- a/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java +++ b/tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java @@ -34,6 +34,15 @@ * transaction status. The per thread transaction timeout set via * {@link TransactionManager#setTransactionTimeout(int)} leaks the same way. * + * This runs from {@link OpenEJBSecurityListener.RequestCapturer}, which sits on the Host + * pipeline and therefore wraps all of {@code StandardHostValve#invoke}. That placement is + * deliberate: the Context pipeline returns before + * {@link jakarta.servlet.ServletRequestListener#requestDestroyed}, before the + * {@code throwable()}/{@code status()} error handling, and before {@code } + * servlets and JSPs, which are dispatched through an include rather than a Pipeline. Cleaning + * up from the Context pipeline would leave all of those uncovered and would also pre-empt an + * application that completes its transaction in {@code requestDestroyed}. + * * @see TOMEE-4652 */ public final class TransactionCleanup { @@ -56,9 +65,7 @@ public static void clean() { try { final Transaction transaction = transactionManager.getTransaction(); if (transaction != null && transaction.getStatus() != Status.STATUS_NO_TRANSACTION) { - LOGGER.warning("Request ended with an active transaction " + transaction - + ", rolling it back to avoid leaking it to the next request on this thread"); - transactionManager.rollback(); + rollback(transactionManager, transaction); } } catch (final Throwable t) { LOGGER.error("Failed to roll back the transaction left over by this request", t); @@ -70,7 +77,35 @@ public static void clean() { } } - // begin() only resets this once a transaction is actually started, so reset it explicitly + resetTimeout(transactionManager); + } + + private static void rollback(final TransactionManager transactionManager, final Transaction transaction) + throws Exception { + // a transaction the reaper already finished is not a leak the application can be blamed + // for, so unassociate it just the same but don't shout about it + final int status = transaction.getStatus(); + if (status == Status.STATUS_ROLLEDBACK || status == Status.STATUS_ROLLING_BACK) { + LOGGER.debug("Request ended with an already rolled back transaction " + transaction + + ", unassociating it from this thread"); + } else { + LOGGER.warning("Request ended with an active transaction " + transaction + + ", rolling it back to avoid leaking it to the next request on this thread"); + } + transactionManager.rollback(); + } + + private static void resetTimeout(final TransactionManager transactionManager) { + // begin() only clears the timeout once a transaction is actually started, so a request + // that set one without beginning leaves it on the thread. + // + // This does pin a ThreadLocal entry, which the CoreUserTransaction.resetError change in + // this same commit argues against. The tradeoff differs: there is no way to read the + // pending timeout back (Geronimo exposes no getter, only the package private + // getTransactionTimeoutMilliseconds(long)), so skipping the call when nothing was set is + // not an option, and setTransactionTimeout(0) stores a null value rather than an + // exception. A null valued entry cannot hold a webapp classloader alive the way a stored + // exception's stack trace can, so leaking the wrong timeout is the worse of the two. try { transactionManager.setTransactionTimeout(0); } catch (final Throwable t) { diff --git a/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java b/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java index ffab520e2b8..8bf33115b18 100644 --- a/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java +++ b/tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java @@ -17,6 +17,7 @@ package org.apache.tomee.embedded; import org.apache.openejb.loader.IO; +import org.apache.openejb.loader.SystemInstance; import org.apache.openejb.util.NetworkUtil; import org.junit.Test; @@ -27,8 +28,10 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import jakarta.transaction.Status; +import jakarta.transaction.TransactionManager; import jakarta.transaction.UserTransaction; import java.io.IOException; +import java.lang.reflect.Field; import java.net.URL; import static org.junit.Assert.assertEquals; @@ -66,11 +69,25 @@ public void transactionDoesNotLeakToNextRequest() throws IOException { assertEquals("STATUS_NO_TRANSACTION", victim.substring(0, victim.indexOf(" on "))); - // and must still be able to run a transaction of its own - assertEquals("committed", IO.slurp(new URL(base + "/commit"))); + // and must still be able to run a transaction of its own, with the container default + // timeout rather than the 120s the leaker set on this thread + assertEquals("committed with the default timeout", IO.slurp(new URL(base + "/commit"))); } } + private static final long LEAKED_TIMEOUT_MS = 120 * 1000L; + + /** + * Geronimo keeps the effective timeout in a private field of TransactionImpl with no getter, + * so reflection is the only way to observe which timeout a transaction actually got. The field + * holds an absolute deadline (duration + currentTime), so turn it back into a duration. + */ + private static long timeoutMillisOf(final Object transaction) throws Exception { + final Field field = transaction.getClass().getDeclaredField("timeout"); + field.setAccessible(true); + return field.getLong(transaction) - System.currentTimeMillis(); + } + private static String threadOf(final String response) { final int marker = response.indexOf(" on "); assertTrue("no thread name in response: " + response, marker > 0); @@ -125,8 +142,16 @@ protected void service(final HttpServletRequest req, final HttpServletResponse r // would throw NotSupportedException ("Nested Transactions are not supported") // if the leaked transaction were still associated with this thread ut.begin(); + + // if the timeout leaked, this transaction inherits the leaker's 120s instead of + // the 600s container default + final long timeoutMillis = timeoutMillisOf(SystemInstance.get() + .getComponent(TransactionManager.class).getTransaction()); ut.commit(); - resp.getWriter().write("committed"); + + resp.getWriter().write(timeoutMillis <= LEAKED_TIMEOUT_MS + ? "committed with the leaked 120s timeout" + : "committed with the default timeout"); } catch (final Exception e) { resp.getWriter().write("failed: " + e.getClass().getSimpleName() + " " + e.getMessage()); }