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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
