From 9fba0861c79dfcad5c361df033c07a3002fee4fc Mon Sep 17 00:00:00 2001 From: Markus Jung Date: Thu, 23 Jul 2026 21:05:23 +0200 Subject: [PATCH] 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()); + } + } + } +}