CDI context propagation

[FroMage]

I didn't see in the spec anywhere where we define how CDI should propagate its context with CP. In particular, the current implementation of CDI context propagation turns out to have confusing semantics, where it does not capture the request context entirely, but only the values currently added to the context.

My opinion was that it was obvious that when we want the request context to be captured, it has to be the entire request context (which is a mutable Map), which means being able to see all modifications to it that happened before. Not just the current values defined in the request context.

So it was implemented as capturing values currently active at this point, which is wrong IMO and pretty useless, unfortunately. So perhaps we need to clarify this in the spec?

To explain why we want to propagate the entire mutable context, here's an imperative example:

// nothing in the current request context
a(); // causes lazy beans to be added to the request context
b(); // can use the added request-context beans

It's pretty obvious that the request context (like the application context) is a mutable Map that gets mutated, and "happens-before" modifications are visible to later instructions. It's a context that follows the current request.

Now here's the same application in a reactive fashion:

// nothing in the current request context
return threadContext.withContextPropagation(CompletableFuture.completedFuture(null))
 .thenAccept(v-> a()) // causes lazy beans to be added to the request context
 .thenAccept(v -> b()); // can use the added request-context beans

To any reactive user, it's pretty obvious that this should work exactly the same as the imperative case. Except with the current implementation of CDI context propagation, the request context is captured before a() adds things to it, and restored empty so that b() has request-scoped beans vanish from its request scope.

In practice, this causes the request scope to not really be propagated in a reactive pipeline (since "happens-before" mutations are not always visible). It differs from the imperative code, and does not follow the principle of least surprise (beans vanishing from the request context and potentially later coming back to it are very hard to debug).

It also makes lazy request-scoped beans unusable with reactive applications, since to make them properly follow the request lifecycle we would require to eagerly load them all at the start of the request, which is very bad for performance.

Finally, note that this is not regular at all with the application scope, which is propagated entirely, with the right "happens-before" mutations visible.

I didn't think this would be something the spec should define, since it's CDI-specific, and I thought pretty obvious, but perhaps we can make this generic and specify that while context-propagation is ThreadContextProvider-specific, we encourage behaviour to follow the "happens-before" semantics for mutable contexts, so that reactive code behaves in the same way to imperative code.

[FroMage]

Addentum: the practise of capturing/restoring values inside a request context (as opposed to just propagating the request context entire) appears to cause values to never be destroyed, which is a horrible source of bugs, with DB connections obtained more than once per request, and never released.

[manovotn]

(CDI) Application context is not part of context propagation here as that context is backed by a concurrent map and all instances are shared across application. This has implications for users that define such beans.

Opposed to that are request scoped beans, stored in a thread local map. Current propagation is achieved by copying req. scoped instances from current active context to the new thread where the context is activated (without any link to the original HTTP request by the way).

Note that contexts can be manipulated by events, users or frameworks and can end abruptly leading to destruction of contextual instances during context deactivation and destruction.
Sharing whole underlying storage would mean that the storage (map) needs to be thread-safe which translates into somewhat slower in performance but it would also be risky for when any thread ends its request context, it will attempt to destroy all instances despite them being referenced in other threads and possibly still used.

As for what you try to achieve - the "happens-before" sequence of events - this seems like a more common problem that could affect more contexts than just CDI. I might be missing something but...
What you effectively ask for is to capture a snapshot that is dynamic which kinda defeats the design that was made here. I think MP CP tried to make sure that the same context can be applied repeatedly with same results which is what you are seeing. E.g.:

return threadContext.withContextPropagation(CompletableFuture.completedFuture(null))

this creates the immutable snapshot, in your example with no req. bean active

.thenAccept(v-> a()) // causes lazy beans to be added to the request context

this applies the snapshot (makes sure there is req. context active but with no beans) and runs something on it

.thenAccept(v -> b()); // can use the added request-context beans

here you are running on the same snapshot, e.g. it is "reset" into whatever state was in the snapshot, if you want to preserve the state, you would need to capture the snapshot after a() was invoked and run on top on that one instead

[FroMage]

Well, remember CP was created to alleviate the lack of pipeline-local (equivalent to thread-local for reactive pipelines), so the taking a CDI snapshot rather than propagating the entire request scope makes it completely unusable. Again, please consider the actual use-cases rather than theoretical aspects around concurrent execution, which is not what we're trying to solve here.

[manovotn]

That would mean we were wrong from the start, because even javadoc of ThreadContextSnapshot says An immutable snapshot of a particular type of thread context.

[manovotn]

Snapshot being immutable makes a lot of sense IMO.
But what you propose could be implemented either via some config or some different API where basically any subsequent 'action' would capture snapshot from previous step.

I don't really see a fault with purely CDI context propagation for this issue. Like I said, other contexts are likely to suffer from similar problems as you described here for CDI simply due to immutable nature of snapshots.

[njr-11]

The intent of documenting ThreadContextSnapshot instances as immutable is reflected in the two sentences after it in the JavaDoc,

 * <p>The captured context represented by this snapshot can be applied to
 * any number of threads, including concurrently.</p>
 *
 * <p>Any state that is associated with context applied to a thread should
 * be kept, not within the snapshot, but within the distinct
 * <code>ThreadContextController</code> instance it creates each time it is applied
 * to a thread.</p>

If we consider what would happen if a ThreadContextSnapshot were to instead store state (about how to restore a previous context) and then the snapshot was applied to two threads at once, that information would be overwritten and it would become impossible to properly restore one of the threads to its previous state after the task/action completes. This was the original reason for the language stating immutable.

That said, it is still quite valuable for most context types to have the context itself behave as immutable. A good example is Security context. When you create a completion stage via MP Context Propagation with Security context propagation enabled, you are intending for the action to run as the user (USER1) that is currently on the thread. If you happen to log in a different user (USER2) after that point (or within a prior stage to which you also propagated context which runs first), you still want the stage that you created under USER1 to run as USER1, not a different user.

Here's an example of the above,

running as USER1
stage2 = stage1.thenRun(() -> { ... do stuff, then log in as USER2 & do more stuff });
stage3 = stage2.thenRun(() -> { /* this better be running as USER1 */ });

There are other context types, however, such as Transaction that could be viewed as semi-immutable. Transaction context can be considered immutable in that the snapshot is of a particular transaction ID which never changes. It is that exact transaction id which is re-established on the thread where the completion stage action runs. The transaction's state, however, can vary, and individual stages have the ability to mark it rollback-only or outright commit it or roll it back, and it is up to the application developer to ensure to do the right thing and coordinate between stages. This sort of context propagation might be thought of as an immutable pointer to the actual transaction, whose state can change.

If I'm understanding the debate here, it's about which model CDI context propagation ought to follow.

[FroMage]

Right. Spot on with the Transaction context.

The fundamental property of the CDI Request Context, like its Application Context, is that it's a mutable map, where lazy values can appear, and which must be destroyed at the end of its lifecycle. Those two contexts only differ in the span of their lifecycle, who can view them (everyone vs a single request), and how they are implemented (but that's irrelevant to their behaviour).

At the moment, the CDI ThreadContextProvider uses snapshots of values to capture/restore not the request context, but its current state, which breaks lazy values and destroy semantics (there's never an end for them since they can be captured). I don't think this is the right way to do it, and it breaks so many things and semantics that it's hard to argue it should be.

Perhaps it's indeed the snapshot immutability text that needs to be changed to explain those two models?

[njr-11]

I was reading through what we currently have documented for CDI context, as well as experimenting with our current implementation (based on Weld) to see how it behaves in this area.

The only discussion of the meaning of CDI context appears on the JavaDoc for the CDI constant on ThreadContext,

     * CDI context propagation includes request, session and conversation contexts.
     * When CDI context is propagated, all of the above mentioned contexts that are
     * currently active will be available to the contextualized task with preserved 
     * state. <pre><code>
    ManagedExecutor exec = ManagedExecutor.builder()
        .propagated(ThreadContext.CDI, ThreadContext.APPLICATION)
        .build();
    
    &#64;Inject
    MyRequestScopedBean requestBean;
    
    &#64;GET
    public void foo() {
        exec.supplyAsync(() -&gt; {
            String state = reqBean.getState();
            // do some work with the request state
        }).thenApply(more -&gt; {
            // request state also available in future stages
        });
    }

It uses the somewhat ambiguous wording "preserved state" rather than a more precise term like "snapshot of state as of the point in time when context was captured" or "shared state across all operations participating in the request". The comments in the code example that follow do seem to hint at preserving state across multiple completion stage actions, but aren't explicit enough. There is sufficient ambiguity for an implementation to interpret it either way, and we should definitely clarify this section of doc as well as the language around snapshots.

Our immediate concern should be around whether our implementations are doing the best thing for users.

When I look into the current behavior of our Weld-based implementation of CDI context, I'm seeing similar results to what @FroMage was pointing out. In two separate requests I tried the following,

Request 1:

        requestBean.getState();
        CompletableFuture<Void> cf = appCDIExecutor.runAsync(() -> {
            requestBean.setState(requestBean.getState() + ",1");
        }).thenRunAsync(() -> {
            requestBean.setState(requestBean.getState() + ",2");
        }).thenRunAsync(() -> {
            requestBean.setState(requestBean.getState() + ",3");
        });
        cf.join();

        System.out.println(requestBean.getState()); // prints DEFAULT_VALUE,1,2,3

Request 2

        CompletableFuture<Void> cf = appCDIExecutor.runAsync(() -> {
            requestBean.setState(requestBean.getState() + ",A");
        }).thenRunAsync(() -> {
            requestBean.setState(requestBean.getState() + ",B");
        }).thenRunAsync(() -> {
            requestBean.setState(requestBean.getState() + ",C");
        });
        cf.join();

        System.out.println(requestBean.getState());

The second request (bean not used first) is outputting "DEFAULT_VALUE" rather than "DEFAULT_VALUE,A,B,C".

In my opinion the behavior of request 2 above is counterintuitive, is a less useful behavior, and likely to make writing reactive applications more difficult. @manovotn do you know if it would be possible with Weld to achieve the opposite behavior and directly access the request from the other threads rather than operating on a copy of it? Are there other side-effects here that I'm not thinking of? Or other reasons why it would be undesirable to switch it?

[Emily-Jiang]

What @manovotn said is absolutely true from CDI point of view (immutable copy propagation). For context propagation, it was made to a copy not sharing. I did see what @FroMage and @njr-11 are trying to demonstrate look less useful as the chain is revert back to beginning.

I am wondering whether Weld can be updated to propagate the context in this fashion:

Now: 1->2->3. It seems 1->2 and then 1->3
New: 1->2->3. Can we make 1->2, (a new Request Context created) 2->3 instead
What do you think @manovotn ?

[manovotn]

@njr-11 directly accessing request context won't work because it operates on thread locals. Also the original task is backed by actual HTTP request which serves as a storage for beans (and we have local cache to fasten things up for repeated queries); any subsequent propagation means you are on a thread that no longer has access to HTTP request and no way of linking that. This in practice means we cannot update the actual storage even if we wanted to.

So how it works:
Original thread -> uses HTTP request as storage + cache on top
Some thread after propagation -> new request context is created, plain Map as storage, no links to HTTP request

It goes along the same lines for session and conversation scopes just with HTTP session instead.
Any change you do in other threads won't be able to update the HTTP request/session hence leading to inconsistent stages. Aside from that sharing the whole maps present a danger in clearing the map from one thread where others still use it since request ended for whatever reasons there.

Why don't we instead discuss that ThreadContext should capture new context after each 'action' and use it for the following one? That seems to be what you are after and would work even with the current immutable design plus allowing to actually use immutable stages repeatedly if we decide to allow that.
E.g. this would achieve what @FroMage and @njr-11 both mentioned, right?
return threadContext.withContextPropagation(CompletableFuture.completedFuture(null)) //captures state 1
.thenAccept(v-> a()) // runs with state 1 and at the end captures state 2
.thenAccept(v -> b()); // runs with state 2 and at the end captures state 3

I think both our impls override the Futures already and can manipulate it in some way?

[Emily-Jiang]

@manovotn I think propagate context after each action meets my expectation as well, since we are fixing the problem of relaying instead of going back to the starting point.

[manovotn]

Note that what I suggested above doesn't solve the problem with things like @PreDestroy callbacks which currently 'don't work' with Weld (or Arc in Quarkus). Only the initiating thread can invoke them upon context destruction. All subsequent/parallel threads then only deactivate contexts but never destroy them (so never invoke pre destroy) as that would lead to awkward situations and inconsistent stages (instance pre destroy callback invoked and you can still use it in another thread).

I am mentioning this because it is what @FroMage originally used when coming up with this issue.
Even if propagation works the way that was suggested here, the problem with pre destroy would still exist.

[njr-11]

Thanks for the replies - I was worried about the possibility of additional complications.

Unfortunately, the approach of re-capturing context between stages isn't workable because the completion stage programming model allows for stages to depend upon multiple other stages. The linear model where each stage follows exactly one stage is an oversimplification. For example, in a scenario where an actionD depends on two prior stages,

stageB = stageA.thenRunAsync(actionB);
stageC = stageA.thenRunAsync(actionC);

It would be wrong for

stageB.runAfterBoth(stageC, actionD);

to apply the CDI context for actionD differently than,

stageC.runAfterBoth(stageB, actionD);

We cannot have the former re-capturing CDI context from actionB and the latter from actionC.