jungm commented on PR #2849: URL: https://github.com/apache/tomee/pull/2849#issuecomment-5108821054
Thanks — the ordering correction was right, and I verified it independently before acting on it (`javap` on tomcat-catalina 11.0.23: `StandardContextValve` has zero `fireRequest*` calls; `StandardHostValve.invoke` has `fireRequestInitEvent` at 72, the Context pipeline at 113, `throwable()`/`status()` at 195/298/307, and `fireRequestDestroyEvent` at 327). Rather than only fix the javadoc, I moved the cleanup to `OpenEJBSecurityListener.RequestCapturer` on the Host pipeline, which wraps all of `StandardHostValve#invoke`. That covers `requestDestroyed`, the `throwable()`/`status()` paths, and `<error-page>` servlets/JSPs, and it stops pre-empting applications that complete their transaction in `requestDestroyed`. The javadoc now describes the real ordering. This also removes the `OpenEJBValve` call site entirely, so the "skipped if `listener.exit()` throws" hole goes away with it. Also addressed: - **Already-rolled-back status:** `rollback()` now checks `STATUS_ROLLEDBACK`/`STATUS_ROLLING_BACK` and logs at debug in that case, warning otherwise. - **Timeout reset pinning:** kept unconditional, with a comment explaining why the tradeoff differs from `resetError`. Reading the timeout first isn't available — Geronimo exposes no getter, only the package-private `getTransactionTimeoutMilliseconds(long)`. And `setTransactionTimeout(0)` stores a null value, which can't pin a webapp classloader the way a stored exception's stack trace can. - **Timeout never asserted:** you were right that the branch was untested. `Committer` now reads the effective timeout of its own transaction and the test distinguishes the leaked 120s from the 600s default. Worth noting the first version of this assertion was a false green — the field is an absolute deadline (`duration + currentTime`), not a duration, so comparing it to `120000` could never match. Fixed to compare the remaining duration, and confirmed it fails when the cleanup is removed. On the merge precondition: I have **not** run the Jakarta Transactions TCK, so the exclusions in apache/tomee-tck are untouched and this shouldn't merge on my testing alone. `tomee-catalina` and `tomee-embedded` suites are green. _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_ -- 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]
