[
https://issues.apache.org/jira/browse/TOMEE-4703?focusedWorklogId=1040660&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1040660
]
ASF GitHub Bot logged work on TOMEE-4703:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 10/Sep/26 07:20
Start Date: 10/Sep/26 07:20
Worklog Time Spent: 10m
Work Description: otbutz commented on code in PR #2940:
URL: https://github.com/apache/tomee/pull/2940#discussion_r3976531790
##########
container/openejb-core/src/main/java/org/apache/openejb/core/ThreadContext.java:
##########
@@ -151,13 +152,98 @@ public ThreadContext(final BeanContext beanContext, final
Object primaryKey, fin
this.currentOperation = operation;
}
+ /**
+ * Copy constructor. Must be called on the thread that owns
<code>that</code>, since a
+ * ThreadContext is confined to its thread. Use {@link #capture()} to pass
a context to
+ * another thread.
+ */
public ThreadContext(final ThreadContext that) {
this.beanContext = that.beanContext;
this.primaryKey = that.primaryKey;
- this.data.putAll(that.data);
+ synchronized (that.data) {
+ this.data.putAll(that.data);
+ }
this.oldClassLoader = that.oldClassLoader;
}
+ /**
+ * Returns an immutable copy of the calling thread's context, which may be
passed to other
+ * threads. Must be called on the thread that owns the context.
+ *
+ * @return the capture, or <code>null</code> if no context is entered on
this thread
+ */
+ public static Capture capture() {
+ final ThreadContext current = threadStorage.get();
+ return current == null ? null : new Capture(current);
+ }
+
+ /**
+ * Immutable copy of the state a {@link ThreadContext} propagates: bean
context, primary key and
+ * context data. Per-thread state such as the class loader to restore, the
entered flag and the
+ * current operation is not included.
+ * <p>
+ * A capture may be applied to any number of threads, including
concurrently.
+ * {@link #newThreadContext()} returns a separate mutable {@link
ThreadContext} for each caller,
+ * since {@link ThreadContext#enter(ThreadContext)} modifies its argument
and fails if that
+ * context was already entered.
+ */
+ public static final class Capture {
+
+ /**
+ * Context data tied to the invocation a capture is taken from, listed
by class name to avoid
+ * a dependency on the types. It is not propagated:
+ * <ul>
+ * <li><code>InvocationContext</code> is part of the interceptor
chain the calling thread is
+ * still in. It is single use, and {@link
BaseContext#getContextData()} exposes its
+ * unsynchronized map to application code.</li>
+ * <li><code>DestroyContext</code> references the captured context
and would keep it
+ * reachable for the lifetime of the capture. A new one is
created when the context is
+ * entered on another thread.</li>
+ * </ul>
+ */
+ private static final Set<String> NON_PROPAGATED = Set.of(
+ "jakarta.interceptor.InvocationContext",
+
"org.apache.openejb.cdi.RequestScopedThreadContextListener$DestroyContext");
+
+ private final BeanContext beanContext;
+ private final Object primaryKey;
+ private final Map<Class, Object> data;
+
+ private Capture(final ThreadContext that) {
+ this.beanContext = that.beanContext;
+ this.primaryKey = that.primaryKey;
+
+ final Map<Class, Object> copy = new HashMap<>();
+ synchronized (that.data) {
+ for (final Map.Entry<Class, Object> entry :
that.data.entrySet()) {
+ if (!NON_PROPAGATED.contains(entry.getKey().getName())) {
+ copy.put(entry.getKey(), entry.getValue());
+ }
+ }
Review Comment:
Nitpick, but I would avoid the double inversion here.
```suggestion
for (final Map.Entry<Class, Object> entry :
that.data.entrySet()) {
if (NON_PROPAGATED.contains(entry.getKey().getName())) {
continue;
}
copy.put(entry.getKey(), entry.getValue());
}
```
Issue Time Tracking
-------------------
Worklog Id: (was: 1040660)
Time Spent: 0.5h (was: 20m)
> 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: 0.5h
> 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)
