TOMEE-4652 - roll back UserTransaction left over by a request

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() {

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();
+            }
         }
     }
 
@@ -96,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();
+                }
             }
         }
     }

tomee/tomee-catalina/src/main/java/org/apache/tomee/catalina/TransactionCleanup.java

@@ -0,0 +1,115 @@
+/*
+ * 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.
+ *
+ * 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 <error-page>}
+ * 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 <a href="https://issues.apache.org/jira/browse/TOMEE-4652">TOMEE-4652</a>
+ */
+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) {
+                rollback(transactionManager, transaction);
+            }
+        } 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);
+            }
+        }
+
+        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) {
+            LOGGER.error("Failed to reset the transaction timeout left over by this request", t);
+        }
+    }
+}

tomee/tomee-embedded/src/test/java/org/apache/tomee/embedded/UserTransactionLeakTest.java

@@ -0,0 +1,160 @@
+/*
+ * 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.loader.SystemInstance;
+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.TransactionManager;
+import jakarta.transaction.UserTransaction;
+import java.io.IOException;
+import java.lang.reflect.Field;
+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 <a href="https://issues.apache.org/jira/browse/TOMEE-4652">TOMEE-4652</a>
+ */
+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, 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);
+        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();
+
+                // 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(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());
+            }
+        }
+    }
+}