The VertxContext logic used by the ManagedExecutor should be configurable

[mrickly]

Description

Currently, the ManagedExecutor runs submitted tasks with the runWith method of the ContextHandler returned by VertxCoreRecorder.executionContextHandler. The task is dispatched using the captured VertxContext. It should be possible to set a flag in order to make sure that every task is dispatched on a new duplicate of the captured context. Unclear to me why the same context object is used. Our current "solution" to this problem is to create our own ContextHandler and qualified ManagedExecutor. This leads to code duplication and is obviously far from ideal. Update: we removed our own qualified ManagedExecutor and use the ContextInternal.dispatch on a new duplicated context directly instead (after submitting the task to the usual ManagedExecutor).

Implementation ideas

What about a deep copy of the context every time a dispatch occurs?

[geoand]

What's the use case you have in mind for this enhancement?

[mrickly]

@geoand : It is currently impossible to use ContextLocals in tasks run by the ManagedExecutor, because they share the same Vertx Context. We would like to be able to store information at some point during task execution, and recover it later on.

[geoand]

@cescoffier WDYT?

[mrickly]

I also would like to point out that I'm really unsure how the whole open telemetry stuff would work when used with the ManagedExecutor, since it is also stored in the VertxContext. It's not just our own key/values.

[cescoffier]

I understand the idea, but the implementation is hard. There are cases where we need to propagate and case we should not. I would like having elan explicit API driving the decision - maybe by introducing our own managed execution service sub-interface.

[mrickly]

@cescoffier : Allowing the users to decide which part of the context they want to copy to the new thread by extending the API is certainly one way to do it. The simplest case is copy all (deep copy). I'm not sure there is a use case for a shared context though.

[cescoffier]

The idea was something like this:

@Inject QuarkusManagedExecutor executor;

// ...
executor.submit(mytask, ctx -> /* clear / copy / do whatever you want with the context /*);

[knutwannheden]

@cescoffier Your idea sounds very similar to what MP Context Propagation's ThreadContext (and by extension ManagedExecutor) already provides. Using the respective Builder APIs (or the @ManagedExecutorConfig and @ThreadContextConfig annotations from SmallRye) I can create an instance where I declare which context are propagated or cleared when executing some task. While this doesn't allow specifying the context to propagate or clear per individual call, I wonder if there is an actual use case for this.

AFAICT Quarkus doesn't "implement" the predefined ThreadContext.SECURITY context, nor any separate MP thread contexts for tracing (in OpenTracing days I think SmallRye did have a separate ThreadContext for this), logging MDC data, or the Vert.x request. Instead all of these are under the hood implemented using Vert.x contexts and as a result can't be propagated or cleared using MP Context Propagation.

I assume this was a conscious decision and I would like to understand why the MP Context Propagation falls short of Quarkus' requirements. Can you provide some insight?

[cescoffier]

@knutwannheden yes, and no. I need to ask @FroMage, but the goal here is to change the content of the Vert.x local context, which can be used to propagate stuff, but not only.

We decided to use Vert.x duplicated context instead of thread locals as it's way more efficient to represent an async execution, and also, it works with Loom (thread locals with loom can be touchy).

[knutwannheden]

Looking at SmallRye's implementations of ThreadContext and ThreadContextProvider I can see that they indeed rely on Java ThreadLocals, but I thought that the MP Context Propagation abstractions were high-level enough so that they would allow for different (ThreadLocal independent) implementations. I assume also Vert.x contexts could be used as the "backend".

[cescoffier]

In theory yes, in practice it's very tricky (I failed multiple times)

[knutwannheden]

In theory yes, in practice it's very tricky (I failed multiple times)

Thanks! This answer already helps me better understand the code.