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

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));

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 <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) {
+                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);
+        }
+    }
+}

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