[
https://issues.apache.org/jira/browse/TOMEE-4703?focusedWorklogId=1040785&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1040785
]
ASF GitHub Bot logged work on TOMEE-4703:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 10/Sep/26 17:52
Start Date: 10/Sep/26 17:52
Worklog Time Spent: 10m
Work Description: rzo1 commented on PR #2940:
URL: https://github.com/apache/tomee/pull/2940#issuecomment-5623050207
@JorrenH Good question. For the paths this PR changes, no. `capture()` runs
on the thread that owns the context, `begin()` builds a new `ThreadContext` per
task on the thread running it, and `SecurityThreadContextProvider` only copies
the context that was just entered on that same thread. So the concurrency
utilities no longer touch another thread's `ThreadContext`.
I'd still keep the `synchronized` blocks for now:
- `data` is still a `Collections.synchronizedMap`, and its contract requires
holding the map's monitor while iterating. Dropping the blocks but keeping the
map would just be incorrect in a different way.
- `ThreadContext` is public, and references to it escape in other places.
For example, `PoolEndpointHandler` keeps the creating thread's context in a
field, and `RequestScopedThreadContextListener.DestroyContext` holds one. I
haven't checked all of those, so I can't say yet that it is truly confined to
its thread.
- A monitor nobody else is holding costs almost nothing, and the maps are
small.
Once we've checked that nothing hands a live `ThreadContext` to another
thread, we could replace the `synchronizedMap` with a plain `HashMap` in a
follow-up issue. That's a bigger change than this PR should make, though.
Issue Time Tracking
-------------------
Worklog Id: (was: 1040785)
Time Spent: 40m (was: 0.5h)
> ApplicationThreadContextProvider stores a live ThreadContext reference
> instead of an immutable snapshot
> -------------------------------------------------------------------------------------------------------
>
> Key: TOMEE-4703
> URL: https://issues.apache.org/jira/browse/TOMEE-4703
> Project: TomEE
> Issue Type: Bug
> Components: TomEE Core Server
> Affects Versions: 11.0.0-M1
> Reporter: Richard Zowalla
> Assignee: Richard Zowalla
> Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Follow\-up to TOMEE\-4699, which fixes the
> {{ConcurrentModificationException}} in {{ThreadContext}}'s copy constructor.
> This ticket is about the reason that race exists at all.
> h2. Problem
> {{ApplicationThreadContextProvider.currentContext\(\)}} runs on the
> _submitting_ thread but stores only a live reference to that thread's
> {{ThreadContext}}:
> {code:java}
> return new ApplicationThreadContextSnapshot\(appContext.getId\(\),
> ThreadContext.getThreadContext\(\)\);
> {code}
> The copy happens later, in {{ApplicationThreadContextSnapshot.begin\(\)}}, on
> the _executor_ thread:
> {code:java}
> boolean changeThreadContext = threadContext \!= null && threadContext \!=
> ThreadContext.getThreadContext\(\);
> ThreadContext oldThreadContext = changeThreadContext ?
> ThreadContext.enter\(new ThreadContext\(threadContext\)\) : null;
> {code}
> By then the submitter has returned from {{submit\(\)}} and keeps mutating its
> own context \(e.g. {{InterceptorStack}} sets and removes
> {{InvocationContext.class}} around every business method\). A
> thread\-confined object has escaped its thread; the CME in TOMEE\-4699 is the
> symptom that happens to be loud.
> h2. Spec
> This reads against the Jakarta Concurrency 3.1 SPI contract.
> {{ThreadContextProvider.currentContext}}, javadoc:
> {quote}Captures from the current thread a snapshot of the provided thread
> context type.
> @return immutable snapshot of the provided type of context, captured from the
> current thread.{quote}
> {{ThreadContextSnapshot}}, class javadoc:
> {quote}An immutable snapshot of a particular type of thread context.
> The captured context represented by this snapshot can be applied to any
> number of threads, including concurrently.{quote}
> Spec section 4.1.1, Third\-Party Context Provider's Requirements:
> {quote}Capture or produce snapshots of the provided type of thread context
> when ThreadContextProvider methods are invoked.{quote}
> {{ApplicationThreadContextSnapshot}} satisfies none of these: it captures
> nothing, it is not immutable, and it cannot be applied concurrently.
> To be precise about what is _not_ claimed: the spec body does not pin an
> exact instant of capture for {{ManagedExecutorService.submit\(\)}}. It does
> for the contextual proxy family — {{ContextService.contextualRunnable}} and
> its siblings each say "Context is captured at the time is invoked."
> h2. Proposed fix
> Capture eagerly in {{currentContext\(\)}}, on the owning thread: an immutable
> {{Map}} plus {{beanContext}} and {{primaryKey}}, with {{begin\(\)}} building
> a fresh {{ThreadContext}} per application. The race then cannot occur and no
> locking is needed on the hot path.
> Note the snapshot cannot simply hold a pre\-built {{ThreadContext}}:
> {{ThreadContext.enter\(\)}} writes {{entered}} and {{oldClassLoader}} into
> its argument and throws {{IllegalStateException\("ThreadContext is already
> entered"\)}} on a second call, so one such snapshot could never be applied
> twice, let alone concurrently.
> h2. Prerequisites — this is not a one\-liner
> A prototype surfaced two issues that must be handled first or alongside:
> # _TCCL restore ordering._
> {{ApplicationThreadContextRestorer.endContext\(\)}} restores
> {{oldClassLoader}} and _then_ calls {{ThreadContext.exit\(\)}}, which
> overwrites the TCCL again from the exiting context's own saved loader. Today
> the {{threadContext \!= ThreadContext.getThreadContext\(\)}} identity guard
> keeps {{exitThreadContext}} false on the same\-thread inline path, so this
> stays dormant. Eager capture makes the guard meaningless \(a copy is never
> {{==}}\), which activates it: callers of {{currentContextExecutor\(\)}} would
> return carrying the application classloader. Fix is to exit the ThreadContext
> first and restore the loader last. This is worth its own commit — it also
> fixes a pre\-existing loader leak on pool threads.
> # _The capture is shallow._ {{InterceptorStack}} line 93 puts the submitter's
> live {{InvocationContext}} into the map on every business method and removes
> it only on unwind, so it is always present when an EJB submits.
> {{ReflectionInvocationContext}} holds an unsynchronized {{TreeMap}} \(line
> 41\) and a stateful interceptor {{Iterator}}, exposed to application code via
> {{BaseContext.getContextData\(\)}} \(line 91\). Today that cross\-thread
> share is timing\-dependent; after the change every task would get it. The
> capture needs an exclusion set for values bound to the submitter's in\-flight
> invocation.
> h2. Adjacent defects found while investigating
> Filing here for the record; each may deserve its own ticket.
> * {{ContextServiceImpl.currentContextExecutor\(\)}} is {{command \->
> contextualRunnable\(command\).run\(\);}} \(line 141\), so capture happens
> inside {{execute\(\)}} on the executing thread. The javadoc requires context
> "captured from the thread that invokes currentContextExecutor". Wrong thread
> entirely, independent of this ticket.
> * {{CUTask.Context.enter\(\)}} throws {{IllegalStateException\("Can't enter a
> context twice..."\)}} and {{CUHandler}} reuses one task per proxy, so the
> "any number of threads, including concurrently" guarantee is not reachable
> end\-to\-end for contextual proxies even after this fix.
> * {{RequestScopedThreadContextListener}} line 65 stores a {{DestroyContext}}
> in the context data that holds a direct reference to the {{ThreadContext}},
> so copying the map still pins the submitter's context. Any claim that eager
> capture improves retention is wrong unless this is excluded too.
> * {{ThreadContext.dataToString\(\)}} iterates {{data.entrySet\(\)}}
> unsynchronized — the same defect class as TOMEE\-4699, reachable from
> {{ThreadContext.toString\(\)}}. Being fixed in the TOMEE\-4699 PR.
> h2. Scope
> {{main}} / 11.x only. The minimal fix in TOMEE\-4699 \(PR #2939\) remains the
> right backport for 10.x.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
