[
https://issues.apache.org/jira/browse/TOMEE-4652?focusedWorklogId=1036156&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1036156
]
ASF GitHub Bot logged work on TOMEE-4652:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 17/Aug/26 18:50
Start Date: 17/Aug/26 18:50
Worklog Time Spent: 10m
Work Description: rzo1 commented on PR #2849:
URL: https://github.com/apache/tomee/pull/2849#issuecomment-5318834567
Currently short on time, so AI review only: I ran an adversarial review pass
over this PR, followed by a second pass whose job was to *refute* the first
one's findings against the actual code. Everything below survived that second
pass (two findings did not and are listed at the end as explicitly dismissed).
Take it as input, not as a verdict — I have not run the build.
**Overall: the idea is right and the root-cause analysis is accurate.**
Geronimo's `TransactionManagerImpl` keeps
`threadTx`/`transactionTimeoutMilliseconds` in ThreadLocals that only
`commit()`/`rollback()`/`suspend()` clear, servlets have no interceptor to
restore thread state, and Tomcat pools its exec threads — so a BMT servlet does
leak its transaction into the next request. Hooking cleanup into
`OpenEJBSecurityListener.RequestCapturer` is a deliberate and well-argued
placement, the `resetError` ThreadLocal `remove()` is correct, and
`UserTransactionLeakTest` is a genuine red/green regression test rather than a
vacuous one.
Three things worth addressing, none of which is a regression of existing
behaviour.
### 1. Rollback runs under Catalina's TCCL, after CDI request-context
teardown (minor, worth fixing before merge)
`TransactionCleanup.clean()` is called from `RequestCapturer.invoke`'s
finally block, and that valve is added to the **Host** pipeline
(`TomcatWebAppBuilder.java:317`), so it wraps `StandardHostValve`.
`StandardHostValve.invoke` does `Context.bind(MY_CLASSLOADER)` at entry, fires
`requestInit`/`requestDestroy` inside that region, and `Context.unbind(...)` in
every exit path — and `StandardContext.unbind` sets the TCCL unconditionally.
So by the time `clean()` runs, the TCCL is Catalina's loader and OWB's request
context has already been destroyed.
`transactionManager.rollback()` is not passive: `TransactionImpl.rollback()`
runs `afterCompletion()` over `interposedSyncList` + `syncList`, i.e.
application and JPA-provider code. Any `ServiceLoader.load(...)`,
`Class.forName(name, true, TCCL)` or `CDI.current()` in there resolves against
the wrong loader or throws `ContextNotActiveException`. It is also inconsistent
with the async path, where `AsyncContextImpl.fireOnComplete()` *does*
`context.bind(null)` around the listeners.
Severity is minor rather than major because this only executes when the
application has already leaked a transaction, and the pre-patch behaviour
(afterCompletion never running, or running on some later request's thread) is
strictly worse. But the fix is cheap — the valve has the `Request`:
```java
final Context ctx = request.getContext();
final ClassLoader old = ctx == null ? null : ctx.bind(false, null);
try {
TransactionCleanup.clean();
} finally {
if (ctx != null) ctx.unbind(false, old);
}
```
Note this does not restore the CDI request scope, only the classloader.
### 2. `AsyncContext.start(Runnable)` is still uncovered (minor)
`OpenEJBValve.invoke` only registers the `OpenEJBSecurityListener` as an
`AsyncListener` in the `else` branch guarded by `request.isAsync() &&
getAsyncContextInternal() != null`. On the request that *calls* `startAsync()`
from inside the servlet, the valve has already run and `isAsync()` was false,
so no listener is ever attached and `onComplete`/`onError`/`onTimeout` — hence
the new `asyncExit()` → `clean()` — never fire for it. (`onStartAsync` only
fires on already-registered listeners.)
The case that matters is `AsyncContext.start(Runnable)`:
`AsyncContextImpl.start` issues `ActionCode.ASYNC_RUN`, which
`AsyncStateMachine` submits to the connector endpoint's executor — a pooled
exec thread. Application code runs there, can `begin()` a `UserTransaction`,
and nothing unassociates it. That is the exact TOMEE-4652 symptom, still
reproducible after this patch.
This is missing coverage of a corner case that was equally broken before,
not something the patch introduces — so a follow-up is fine. But the PR
description's claim that `asyncExit()` covers async complete/error/timeout
should be corrected. (The `asyncExit()` hook is not dead weight, to be fair:
when `complete()` is called from a non-container thread, completion is
processed on a connector thread that need not have passed through
`RequestCapturer`.)
### 3. `asyncExit()` contradicts the class javadoc's own rationale (minor)
`TransactionCleanup`'s javadoc argues the Host-pipeline placement is
deliberate so cleanup happens *after*
`ServletRequestListener.requestDestroyed`, specifically so an app that
completes its transaction in `requestDestroyed` is not pre-empted. The second
call site does the opposite: `AsyncContextImpl.fireOnComplete()` binds the CL,
fires the `AsyncListener`s (→ `asyncExit()` → `clean()`), and only *then* calls
`Context.fireRequestDestroyEvent`. So on the async completion path a
transaction is rolled back before `requestDestroyed` gets a chance to commit it
— exactly the pre-emption the javadoc says was avoided. Either qualify the
javadoc or move the async hook.
### 4. `clean()` skips the stale association it claims to remove (nit)
The guard is `transaction != null && transaction.getStatus() !=
Status.STATUS_NO_TRANSACTION`. In `TransactionImpl`, a completed rollback (and
every commit path) ends with `status = STATUS_NO_TRANSACTION`, not
`STATUS_ROLLEDBACK`. So a transaction completed by calling
`Transaction.commit()`/`rollback()` directly on the `Transaction` object —
legal JTA, and the only way an association can survive at all, since
`TransactionManagerImpl.commit()/rollback()` always `unassociate()` in a
finally — presents as `threadTx != null && status == NO_TRANSACTION` and is
skipped. That contradicts the javadoc ("Restores the calling thread to a state
with no transaction associated to it"), and the orphaned entry stays in
`associatedTransactions` forever once the next `begin()` overwrites `threadTx`,
holding its `syncList`/`resources` (and thus the webapp classloader) and
drifting the JMX active-transaction count.
Dropping the guard is safe: `TransactionManagerImpl.rollback()` unassociates
in its finally even when `tx.rollback()` throws `IllegalStateException`, and
the `catch`/`suspend()` fallback covers the rest.
Two incidental corrections while in there: the
`STATUS_ROLLEDBACK`/`STATUS_ROLLING_BACK` branch in the private `rollback()`
helper is unreachable for a thread-associated Geronimo transaction, and the
comment about "a transaction the reaper already finished" does not describe
reality — `timeoutTimer.schedule` is commented out in this Geronimo version, so
there is no reaper.
### 5. Test parses the response before checking it succeeded (nit)
In `transactionDoesNotLeakToNextRequest`, `victim.substring(0,
victim.indexOf(" on "))` runs on the raw response. If `StatusReporter` hits its
catch block the body is `failed: …` with no `" on "` marker, so `indexOf`
returns -1 and the test dies with `StringIndexOutOfBoundsException` instead of
an assertion naming the actual response. `threadOf()` already guards this
correctly with `assertTrue(marker > 0)`.
### Checked and dismissed
- **`catch (Throwable)` without `ExceptionUtils.handleThrowable`** — that is
a Tomcat-internal convention, not this codebase's. Of the tomee-catalina
classes catching `Throwable`, exactly one (`MinimumErrorReportValve`, which
extends a Tomcat class) uses it. Rethrowing an OOME out of a teardown finally
lands in `StandardHostValve`'s own `catch (Throwable)` anyway.
- **The reflective `TransactionImpl.timeout` read being fragile w.r.t. a
configured `defaultTransactionTimeout`** — does not apply: the test builds its
own embedded `Container` and never touches the transaction manager, so it gets
`service-jar.xml`'s 10-minute default. The reflection itself is acknowledged in
a comment and fails loudly; the proposed alternative (assert a later
transaction "does not time out") would be slow and flaky.
Issue Time Tracking
-------------------
Worklog Id: (was: 1036156)
Time Spent: 1h 10m (was: 1h)
> UserTransaction state leaks across pooled Tomcat threads between requests
> -------------------------------------------------------------------------
>
> Key: TOMEE-4652
> URL: https://issues.apache.org/jira/browse/TOMEE-4652
> Project: TomEE
> Issue Type: Bug
> Components: TomEE Core Server
> Reporter: Markus Jung
> Assignee: Markus Jung
> Priority: Major
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> When a servlet or JSP request leaves a {{UserTransaction}} in a non-clean
> state, the next request served on the same pooled Tomcat exec thread inherits
> that state. The victim request then either misses an expected
> {{IllegalStateException}} or gets an exception it does not expect. This is a
> leaker/victim pair: the same test fails in one vehicle and passes in the
> other, and which tests fail depends on which request lands on which thread.
> The Transactions 2.0 TCK web vehicles show this directly. At the full
> baseline (no exclusions), 49 tests run, 40 pass, 9 fail. All three
> signature-test vehicles pass, so the fault sits in {{UserTransaction}}
> handling, not in transaction propagation itself. The first failures in test
> order sit in the rollback area, before any {{setTransactionTimeout}} call
> runs, which rules out a timeout-related cause for those failures.
> Run alone on a fresh server, each area behaves correctly on its own: the
> rollback area passes 10 of 10, {{settransactiontimeout}} passes 4 of 4, and
> {{setrollbackonly}} passes 7 of 8 (its one failure, the last request in that
> area, is a victim of its own earlier request, not a new bug). There is no gap
> around commit-after-timeout: {{settransactiontimeout001}} sleeps 30 seconds
> before calling {{commit()}}, and when it reaches that call in isolation,
> {{commit()}} throws as required.
> Because a failing request poisons whichever request follows it on the same
> thread, excluding only the ids that fail at baseline just moves the failure
> onto different tests (a 9-id exclusion list leaves 4 different tests
> failing). All three areas are excluded whole in the harness so the default
> run stays stable and green.
> h2. Steps to reproduce / TCK reference
> Run the Jakarta Transactions 2.0 TCK web vehicles (servlet and JSP) against
> TomEE 11 without exclusions. Affected test classes and methods, currently
> excluded in {{runner-standalone/exclusions/transactions.txt}} in the
> apache/tomee-tck harness repo:
> *
> {{com/sun/ts/tests/jta/ee/usertransaction/rollback/UserRollbackClient.java}}
> — {{testUserRollback001}} through {{testUserRollback005}}, each {{_from_jsp}}
> and {{_from_servlet}}
> *
> {{com/sun/ts/tests/jta/ee/usertransaction/setrollbackonly/UserSetRollbackOnlyClient.java}}
> — {{testUserSetRollbackOnly001}} through {{testUserSetRollbackOnly004}},
> each {{_from_jsp}} and {{_from_servlet}}
> *
> {{com/sun/ts/tests/jta/ee/usertransaction/settransactiontimeout/UserSetTransactionTimeoutClient.java}}
> — {{testUserSetTransactionTimeout001}} and
> {{testUserSetTransactionTimeout002}}, each {{_from_jsp}} and {{_from_servlet}}
> To confirm the fix, remove these three areas from {{transactions.txt}} and
> rerun the full baseline; all 49 tests should pass regardless of
> thread-to-request assignment.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
