Hi Richard, Thanks for your thorough reply! I've addressed 1.) on the PR. I'm not sure how to fix 2.) since copying the thread context on the submitted thread is the inherent issue. Resolving TOMEE-4703 would also resolve the concurrency problems if I understand it correctly. That sounds like a better long-term solution while this fix can address the immediate cause on 10.x.
I've also logged the CUTask issue as TOMEE-4704, feel free to add any information if I missed any. I think the point on ThreadPoolExecutor not surfacing errors is an api choice for the `void execute(Runnable)` signature. We can surface the error by using the `Future<?> submit(Runnable)` method instead. As long as TOMEE-4703 and TOMEE-4704 are addressed, I don't think this is an actual issue. Cheers, Jorren Op di 8 sep 2026 om 21:04 schreef Richard Zowalla <[email protected]>: > > Hi Jorren, > > Thanks for the report and the patch - the analysis in the ticket is spot on, > and the reproduction notes are appreciated. > > +1 on the PR from me. synchronized (that.data) acquires the same monitor > Collections.synchronizedMap uses internally, so the entrySet() iteration > inside putAll is safe. > It's minimal and back portable, and I'd like to get it into 10.x the ticket > has no fix version yet. > > Two things I'd add to the PR before merge: > > 1.) ThreadContext.dataToString() has the same defect - it streams > data.entrySet() unsynchronized, and it's reachable from > ThreadContext.toString(), which ApplicationThreadContextSnapshot.toString() > embeds. Worth fixing in the same commit. To keep the critical section short > and avoid calling entry.getValue().hashCode() (arbitrary application code) > while ho-lding a monitor that sits on the EJB invocation hot path, better to > copy under the lock and format outside it. > > 2.) Separately, the copy constructor also reads that.oldClassLoader, a plain > non-volatile field, with no happens-before. The map lock doesn't cover that, > so the copy as a whole still isn't safely published. Not a blocker for this > fix, just worth knowing the patch addresses the part that throws rather than > the whole cross-thread read. > > On the deeper cause: I think the CME is a symptom of > ApplicationThreadContextSnapshot holding a live reference to another thread's > ThreadContext instead of a captured value. currentContext() runs on the > submitting thread but stores only the reference; the copy happens later in > begin(), on the executor thread, while the submitter has moved on. That reads > against the SPI: ThreadContextProvider.currentContext is specified as > "Captures from the current thread a snapshot", returning an "immutable > snapshot ... captured from the current thread", and ThreadContextSnapshot is > "An immutable snapshot ... can be applied to any number of threads, including > concurrently". Spec §4.1.1 puts the same on the provider: "Capture or produce > snapshots of the provided typeof thread context when ThreadContextProvider > methods are invoked." > > Capturing eagerly in currentContext(), an immutable map plus the scalars, > with begin() building a fresh ThreadContext per application, removes the race > at the source and needs no locking at all. Note it can't just stash a > pre-built ThreadContext, since ThreadContext.enter() writes into its argument > and throws on a second entry, so one snapshot could never be applied twice. > > I've build that locally and it isn't free: dropping thethreadContext != > ThreadContext.getThreadContext() exposes an inverted restore ordering in > ApplicationThreadContextRestorer.endContext() (it restores the TCCL, then > ThreadContext.exit() overwrites it again), and the shallow copy makes the > sharing of the submitter's live InvocationContext deterministic. Both are > fixable, but it's clearly 11.x material, not a 10.x backport. I'll write it > up on a separate ticket: https://issues.apache.org/jira/browse/TOMEE-4703 > > On CUTask: please do file it, and I don't think it's optional. > initialContext.enter(), the container-listener onStart loop and > contextService.enter(snapshot) all sit outside the try, so when > enter(snapshot) threw the CME,taskStarting/taskAborted/taskDone never ran > which is exactly why you saw silent loss and not a stack trace. The finally > never runs either, so initialContext.exit() is skipped and CUTask.CURRENT > stays set on that pool thread. The next task scheduled onto it then hits > IllegalStateException("Can't enter a context twice, create a new one, and > call enter() on that."), so one CME can poison a pool thread for every task > after it. That makes your second point a bug, independent of the map race. > > Your third point (exceptions propagating into the ThreadPoolExecutor with > nothing surfacing them) is the other half of why this was invisible. Happy to > see that as its own ticket too; three small tickets are easier to review and > backport than one large one. > > Hope it makes sense… > > Thanks again, > Richard > > > Am 08.09.2026 um 16:21 schrieb Jorren Hendriks via dev > > <[email protected]>: > > > > Hello everyone, > > > > I've submitted a small patch <https://github.com/apache/tomee/pull/2939> to > > fix a concurrency issue in ThreadContext we encountered on several > > applications. The fix addresses the immediate issue and would unblock our > > other applications from migrating from TomEE 8 to 10. A review would be > > appreciated! > > > > As mentioned in the jira ticket > > <https://issues.apache.org/jira/browse/TOMEE-4699>, this did expose a > > possibly other issue. Inside CUTask exceptions are not caught on lines > > 70-85. This means neither taskAborted or taskDone is ever called when an > > exception is thrown. Since this is not a blocking issue for me, I'll leave > > it to you to decide if it's worth changing. I can file this as a separate > > JIRA ticket if that works better for you. > > > > Thanks, > > Jorren > > > > -- > > *Confidentiality Note:* This email and any attachments may contain > > confidential information and are intended solely for the named recipients. > > If you are not an intended recipient, any use, copying, disclosure, or > > distribution is prohibited. Please notify the sender and delete the message > > and any copies. Confidential information may not be shared with any third > > party without the prior written consent of NSX bv. > -- *Confidentiality Note:* This email and any attachments may contain confidential information and are intended solely for the named recipients. If you are not an intended recipient, any use, copying, disclosure, or distribution is prohibited. Please notify the sender and delete the message and any copies. Confidential information may not be shared with any third party without the prior written consent of NSX bv.
