Richard Zowalla created TOMEE-4703:
--------------------------------------
Summary: 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
Reporter: Richard Zowalla
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)
