TOMEE-4703 / TOMEE-4704 capture thread context on the submitting thread - #2940
TOMEE-4703 / TOMEE-4704 capture thread context on the submitting thread#2940rzo1 wants to merge 5 commits into
Conversation
…Context ApplicationThreadContextRestorer.endContext() restored the class loader it had saved and then called ThreadContext.exit(), which sets the loader again from the value the context recorded on entry. That value is the application class loader installed by begin(), so the thread was left with the application class loader instead of the one it arrived with. Exit first and restore the loader afterwards. This affects pool threads, where the captured context is never the thread's current context and the identity guard in begin() therefore never applies.
ApplicationThreadContextProvider stored a live reference to the submitting thread's ThreadContext and copied it later, in begin(), on the thread running the task. A ThreadContext is confined to its thread, so that copy raced with the owner and could fail with a ConcurrentModificationException (TOMEE-4699). Capture an immutable snapshot in currentContext() instead and build a new ThreadContext from it in begin(). The snapshot cannot hold a ThreadContext, because enter() modifies its argument and fails if that context was already entered, so one snapshot could not be applied twice. Context data that is tied to the invocation the capture is taken from is left out. InvocationContext is part of the interceptor chain the caller is still in, and BaseContext.getContextData() exposes its unsynchronized map to application code. DestroyContext references the captured context and would keep it reachable for the lifetime of the capture. Both are recreated on the thread the context is entered on. The context data is also formatted outside the map's monitor now, so that application hashCode() implementations no longer run under a lock that is taken on every invocation.
… CUTask.Context currentContextExecutor() was "command -> contextualRunnable(command).run()", so the context was captured inside execute(), on the thread submitting a command. ContextService specifies an executor that runs tasks on the calling thread "but with context that is captured from the thread that invokes currentContextExecutor". Capture the snapshot when the executor is created and reuse it, through a CUTask constructor that takes an existing snapshot. CUTask created one CUTask.Context per task, in the constructor, but a contextual proxy runs its task more than once and may run it on several threads at the same time. Concurrent invocations shared that Context, raced on its previous field and its exit task list, and failed with "Can't enter a context twice" when the callers had a context of their own. Create the Context per invocation.
…ontext CUTask.invoke() entered the CUTask context, started the container listeners and applied the context snapshot before the try block. An exception from any of them skipped taskStarting, taskAborted and taskDone, so the ManagedTaskListener was never notified, and skipped the finally block, which left the CUTask context set on the thread. The next task to run on that thread then found a context it had not entered. Move the setup inside the try. The listener is notified and the teardown runs whether the failure comes from establishing the context or from the task itself. The teardown only unwinds what was established: the container listeners that were started, and the context only if it was entered. CONTAINER_LISTENERS is read once, since the array is replaced when a listener is registered and the teardown walks the listeners that were started.
JorrenH
left a comment
JorrenH
left a comment
There was a problem hiding this comment.
Fixes the issues and moves the implementation closer to spec which is good.
One question I had; is synchronization still needed when the capture is synchronous on the owning thread? I.e. is it still possible for other threads to directly access the ThreadContext with these changes?
|
@JorrenH Good question. For the paths this PR changes, no. I'd still keep the
Once we've checked that nothing hands a live |
|
Full TCK build is ok. |
|
@jungm Just a note: I think this would be backportable to Since it changes when the context is captured (on submit instead of when the task runs), I'd do it as a separate |
3 participants
TOMEE-4703 / TOMEE-4704 — capture thread context on the submitting thread
Follow-up to #2939, which fixes the CME itself and should still go in for 10.x. This targets
mainand addresses why the race exists.
ApplicationThreadContextProvider.currentContext()runs on the submitting thread but stores a livereference to its
ThreadContext, and copies it later inbegin(), on the thread running the task.The SPI javadoc asks for the opposite:
ThreadContextProvider.currentContext"Captures from thecurrent thread a snapshot of the provided thread context type", returning an "immutable snapshot
... captured from the current thread", and
ThreadContextSnapshot"can be applied to any number ofthreads, including concurrently". Spec section 4.1.1 puts the same duty on the provider: "Capture or
produce snapshots of the provided type of thread context when ThreadContextProvider methods are
invoked."
Commits
340376ed25—endContext()restored the class loader beforeThreadContext.exit(), which thenoverwrote it, leaving pool threads on the application class loader. Pre-existing, standalone,
backport candidate for 10.3.0.
c940d73042— capture eagerly into an immutableThreadContext.Capture; build a newThreadContextfrom it perbegin(). ExcludesInvocationContextandDestroyContext, which aretied to the submitter's invocation.
c6cb5080c9—currentContextExecutor()captured inexecute()instead of at creation, and oneCUTask.Contextper task broke concurrent use of a contextual proxy.9d79f7de06— TOMEE-4704:CUTask.invoke()did its setup outside thetry, so a failure thereskipped the listener callbacks and the cleanup.
TCK run: https://ci-builds.apache.org/job/TomEe/job/pull-request-manual-with-tck/59/