rzo1 commented on PR #2849:
URL: https://github.com/apache/tomee/pull/2849#issuecomment-5088904759
Verified the premise against the Geronimo 4.0.0 sources — `unassociate()`
and `begin()`
are the only places `threadTx` and `transactionTimeoutMilliseconds` get
cleared, so a web
request really can strand both on a pooled exec thread. Rolling back at
request teardown
is the right fix and matches `TxBeanManaged.commit()`. The
single-exec-thread test is a
genuinely good reproduction, and the guard that fails loudly if thread reuse
didn't happen
is the right instinct.
The placement rationale is wrong, though, and it should be corrected in the
javadoc
because it changes what applications observe:
- The description says the valve runs after
`ServletRequestListener#requestDestroyed`.
It's the opposite. I disassembled tomcat-catalina 11.0.23:
`StandardContextValve` has no
`fireRequest*` call at all. `StandardHostValve.invoke` fires
`fireRequestInitEvent` at
offset 72, invokes the Context pipeline (where `OpenEJBValve` and
therefore `clean()`
live) at offset 125, and fires `fireRequestDestroyEvent` only at offset
327 — after the
pipeline returns.
So an application `ServletRequestListener` implementing a tx-per-request
pattern and
committing in `requestDestroyed` now finds the transaction already rolled
back, gets a
WARN on every single request, and an `IllegalStateException` from its own
`commit()`.
Filter- and servlet-based patterns are unaffected, so this doesn't
invalidate the fix —
but pre-empting `requestDestroyed` is a real behavioural change and
belongs in the
javadoc.
- Third uncovered path, more concrete than the async ones:
`StandardHostValve.invoke`
calls `throwable()`/`status()` at offsets 195/298/307, after the pipeline,
and
`custom()` dispatches the error page through
`ApplicationDispatcher.include()`, not a
Pipeline. So `<error-page>` servlets and JSPs run after `clean()` — they
leak exactly as
before this PR, and they now also run with the request's transaction
already rolled back
and the caller identity already cleared. If you want that covered,
`OpenEJBSecurityListener.RequestCapturer` on the Host pipeline
(`TomcatWebAppBuilder:317`) wraps all of it.
Smaller:
- In `OpenEJBValve`, `TransactionCleanup.clean()` is skipped if
`listener.exit()` throws.
The async path already gets this right with a nested `finally` — please
mirror it here.
- `setTransactionTimeout(0)` pins the very ThreadLocal entry that the
`CoreUserTransaction.resetError` hunk in this same PR argues against
pinning. Worth a
comment explaining why the tradeoff differs, or reading the timeout first
and only
resetting when non-zero.
- The timeout reset is never asserted. `Leaker` sets
`setTransactionTimeout(120)` with a
comment saying it must not leak, and then nothing checks it — the branch
is untested.
- Unconditional `rollback()` logs an ERROR when the leftover transaction is
no longer
rollback-able (already rolled back / marked for rollback by a timeout
reaper). Check
`getStatus()` against `STATUS_ROLLEDBACK`/`STATUS_ROLLING_BACK` first, or
log at debug.
Merge precondition rather than a code comment: please re-run the Jakarta
Transactions TCK
and drop the corresponding exclusions in apache/tomee-tck with this, since
that's the
harness that surfaced it.
--
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]
