[
https://issues.apache.org/jira/browse/TOMEE-4652?focusedWorklogId=1032354&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032354
]
ASF GitHub Bot logged work on TOMEE-4652:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 27/Jul/26 08:13
Start Date: 27/Jul/26 08:13
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1032354)
Time Spent: 20m (was: 10m)
> 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: 20m
> 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)
