Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CDI context propagation #474

Open
Emily-Jiang opened this issue Apr 14, 2021 · 17 comments
Open

CDI context propagation #474

Emily-Jiang opened this issue Apr 14, 2021 · 17 comments

Comments

@Emily-Jiang
Copy link
Contributor

In CDI spec, the context propagation was not addressed in the spec. The original ticket was raised:

Key: CDI-587
URL: https://issues.jboss.org/browse/CDI-587
Project: CDI Specification Issues
Issue Type: Epic
Components: Contexts
Affects Versions: 1.2.Final
Reporter: Romain Manni-Bucau

The overall idea is to ensure that it is not cause the code becomes asynchronous/reactive/... that we loose the instances or get another context.
An example is starting an AsyncContext in a servlet.
One proposal is to add a flag to ask for propagation of the context in async managed threads: @RequestScoped(inherited = true) which would reuse the same instances for these "new" threads.
Note however this issue is not only bound to servlets even if it is the easiest example.
The original thread disucssion on the list: http://cdi-development-mailing-list.1064426.n5.nabble.com/RequestScope-Life-Cycle-td5712701.html

In MicroProfile Context Propagation, some effort was made to address the context propagation and the implementation was made in Weld. However, it has a some issues, such as eclipse/microprofile-context-propagation#167

@manovotn
Copy link
Contributor

Note that other EE specs that have propagation within MP CP also don't have it mentioned in the specification - for instance JTA. The whole effort was MP oriented and by adding it to the CDI specification, it will outreach into SE or EE environment which I am not so sure is a good thing.

In MicroProfile Context Propagation, some effort was made to address the context propagation and the implementation was made in Weld. However, it has a some issues, such as eclipse/microprofile-context-propagation#167

Yes, most of the issues are captured in this discussion and they still hold true so the discussion is definitely worth a read.
But how do you suggest you could fix this on the spec level?

@cen1
Copy link

cen1 commented May 19, 2021

Does this https://issues.redhat.com/browse/CDI-452 fall under this issue or is it different? Asking as to not open a duplicate.

@Emily-Jiang
Copy link
Contributor Author

@cen1 I think the issue https://issues.redhat.com/browse/CDI-452 falls under this issue.

@njr-11
Copy link

njr-11 commented Oct 6, 2021

Note that other EE specs that have propagation within MP CP also don't have it mentioned in the specification - for instance JTA.

I think JTA is more an example of why it needs to be defined in the Jakarta EE spec. Because context propagation isn't considered in the JTA programming model there are severe shortcoming with respect to usability and behavior that you do or don't get across different transaction manager implementations for propagated transaction context. For example, it has constantly been a problem for us that the JTA transaction continues to be associated with the original thread, possibly overlapping the completion stage action when it does actually run on another thread. A vendor-specific mechanism is needed for it to even be possible for the application to do a suspend of transaction context, but even that doesn't fully clear up this problem except when the application goes to great lengths to work around it. And because each transaction manager supports transaction context propagation to a different degree, everywhere that applications rely on it is non-portable code. Standardization is needed on the Jakarta EE side, and lacking that MicroProfile just did the best that it could.

The whole effort was MP oriented and by adding it to the CDI specification, it will outreach into SE or EE environment which I am not so sure is a good thing.

I'd argue that it's definitely good to have the CDI spec standardize what and how CDI context behaves when accessed asynchronously and when/whether it is possible for which types of CDI context to be made available this way. Having this stanardized means that users would be able to write portable applications that can rely on getting consistent and predictable behavior, per the guarantees of the CDI (plus MP Context Propagation or Jakarta Concurrency) specification regardless of which providers are used. It would mean that the same CDI experts who are writing the CDI spec would have participation and ownership in defining how CDI context is or isn't to be used asynchronously and that when designing new capabilities in CDI, the CDI spec would continue to account for this rather than introducing new capability without any consideration for it and leaving MP Context Propagation/Jakarta Concurrency to later discover and deal with the mess after the fact, which might or might not even be possible. MicroProfile didn't engage the Jakarta CDI community originally for the same reason it didn't engage with Jakarta Transactions, which is that Jakarta EE didn't exist yet and Java EE was in limbo at the time. But now that Jakarta EE is here, all of these specifications will be made better if Jakarta EE can officially standardize requirements, behavior and expectations.

@scottkurz
Copy link
Contributor

scottkurz commented Oct 6, 2021

For batch, something like @RequestScoped(inherited = true) would allow context to be shared between the code starting a job and the code executing the job (tracking here), since the job typically (always?) executes on a separate thread.

Even some of the enhancement ideas adding scopes to share more within the job execution wouldn't do this.

One subtle question: could batch's use case be satisfied by the spec tying the capability to a concurrency API like ManagedExecutorService (assuming CDI would even want to do that) ? I think probably it could, though I guess another approach from the CDI perspective might be to define context propagation more generally (w/o requiring ManagedExecutorService). Just mentioning.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 20, 2022

I promised to share a slide deck that I have that illustrates known troubles with request context propagation. Here goes: https://docs.google.com/presentation/d/1EaMdHY9NkIGn1qx3KYyk9hjCnAfLGoer7n3HN-GgoTc/edit

@Azquelt
Copy link
Contributor

Azquelt commented Sep 20, 2022

We discussed this on the CDI weekly call two weeks ago and came up with quite a few issues. I'll write up what I remember, if anyone else has any corrections or more detail then please add that.

The problem isn't well defined

CDI doesn't have one context which may be propagated, it has several scopes and each have different rules. Any proposal must be more specific than "please propagate context".

Issues with propagating the request context between threads

One interpretation of this request for "CDI context propagation" is that the request context should be propagated to any task submitted from the request thread to allow code such as the following to work:

@Resource private ManagedExecutor executor;

@Inject private MyRequestScopedBean bean;

public void myMethod() {
    bean.setValue("foo");
    executor.submit(() -> {
        System.out.println(bean.getValue()); // prints foo
    });
}

We noted a number of problems with this.

  1. Lots of existing request scoped beans are not thread safe. If we start allowing the same request context to be active on two threads simultaneously, we will break things (slide 6 in Ladisek's presentation above)
  2. When does the request scope end? Generally the request scope ends when the request is sent back to the user, but threads started during the request are allowed to continue executing after the request has been completed. Should the request context deactivate while these threads are still running? (slide 7)
  3. I can't remember the details of this, but in Weld, the request context is tied to some object relating to the HTTP request which couldn't be accessed on another thread. There was a suggestion that we could copy the context, but that would mean that while existing beans would be shared between both threads, new beans created on either thread wouldn't be visible on the other. (slides 3,4,5 talking about "backward propagation").

Point 1 is the most pertinent. For this reason, we didn't think that propagating the request context between threads would ever be a good idea.

Different threading models place different requirements on things which are propagated between threads

In a reactive environment, operations will likely be broken up into lots of small peices of code which run asynchronously, but are guaranteed not to run concurrently. No blocking operations are allowed so it's very common for even a simple task to be broken into several asynchronous operations.

In a thread pool environment, blocking operations are allowed and operations usually run on a single thread unless several things need to be done concurrently. When submitting an asynchronous task, it's expected that it may run concurrently with the current thread.

If we define that some context is must be propagated in a particular way, we must make sure it makes sense for these different models.

Conversely, it may make sense for some context to be propagated in one model but not in another due to the different ways that applications are written.

Custom scopes

Some of the requested features could be implemented using a custom scope, which can be done today using existing CDI APIs.

For example, with a hypothetical JobScope for Batch, there would be nothing to stop you defining a way to activate the scope for a job before that job runs so that the thread that starts the job can have access to the same beans that will be available when the job is running. However, if you did that, you'd have to ensure that bean authors know that their JobScoped beans could be accessed when the job is not running.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 21, 2022

Very nice summary, thanks!

Ad item 3: the request context in Weld is not always just a thread-local Map. In HTTP servlet scenarios, it's a wrapper around HttpServletRequest, storing contextual instances into the HttpServletRequest attributes. There's even an implementation for EJBs that is a wrapper around InvocationContext, which stores contextual instances into the context data map. (Similarly, the session and conversation contexts, in the HTTP servlet scenario, are wrappers around HttpSession.)

@manovotn
Copy link
Contributor

Very nice summary, thanks!

Ad item 3: the request context in Weld is not always just a thread-local Map. In HTTP servlet scenarios, it's a wrapper around HttpServletRequest, storing contextual instances into the HttpServletRequest attributes. There's even an implementation for EJBs that is a wrapper around InvocationContext, which stores contextual instances into the context data map. (Similarly, the session and conversation contexts, in the HTTP servlet scenario, are wrappers around HttpSession.)

Ladislav is right (there are actually 4 implementations of req. context based on where it is used).
I'd also add that the HTTP variant indeed stores beans into HttpServletRequest attributes and while I haven't found any hard requirement to do this, the reason for it are most likely async servlets.

@cen1
Copy link

cen1 commented Sep 21, 2022

I think defining the problem is key here. One of them which I encountered often was propagating headers from incoming request to async rest clients. In non async world, I would usually inject the request context into the client implementation bean and add the headers there. If I wanted to use the client implementation in async it would fall on it's head and I would need to rewrite the implementation to pass all the headers through method params instead. This is obviously very ugly and frustrating when you need to do it.

While this case could be perhaps solved with context propagation through CDI there is another approach. For example, mp-rest-client allows you to propagate headers from incoming request but they simply give you the header map and not the whole context to solve this specific problem (e.g. org.eclipse.microprofile.rest.client.ext.ClientHeadersFactory). Essentially what you would need to do manually in JAX-RS client but packaged in nicer way.

So perhaps these problems that appear to require propagation can mostly be solved in different specifications with very narrow solutions instead.

@hantsy
Copy link
Contributor

hantsy commented Dec 19, 2022

The Context Propagation lib was used in the latest Spring 6.0/Spring Boot 3.0. https://micrometer.io/docs/contextPropagation

@m-reza-rahman
Copy link

Curious where this stands at the moment. Is this being looked at for a subsequent release? //cc @Emily-Jiang

@manovotn
Copy link
Contributor

The Context Propagation lib was used in the latest Spring 6.0/Spring Boot 3.0. https://micrometer.io/docs/contextPropagation

I am not sure I see how is this related? It looks like micrometer custom solution to propagating thread local values?
From the first glance it also looks almost one to one with Microprofile CP in its usage of context snapshot and restoration.
Either way, this library in no way addresses the issue that CDI faces in terms of context propagation.

Curious where this stands at the moment.

This comment sums it up well - #474 (comment)

@hantsy
Copy link
Contributor

hantsy commented Feb 24, 2024

@manovotn Currently CDI context propagation discussion focus on the context inheritance between scopes.

But in a Jakarta EE application, we could encountered issues when switch to execute on different thread executors, eg. an async thread, Kotlin Coroutines, how to make beans available when switched to different execution context(async context, reactive context, or coroutines context). For example, in a RequestScoped bean, it calls another async/reactive stream bean and run in the background. In the target async context, it should propagate the context state from the previous request scope, make request context state available in the async context without any extra handlers, (eg. the security context data by injecting a SecurityContext), even the request beans is destroyed earlier.

In the new Spring 6.x, it addressed this issue via https://micrometer.io/docs/contextPropagation (which is mainly maintained by Spring guys)

@JHahnHRO
Copy link

Maybe the way out is to rely on structured concurrency in the future? I know that that's a long way off until CDI can be based on some Java version that isn't even out yet, but the principle of structured concurrency solves some of the problems presented here, doesn't it?

What I mean by that: The CDI container could provide a (probably @Dependent) StructuredTaskScope (or some similar CDI-specific interface) bean that propagates the request context only from the thread that created the TaskScope to subtasks within that TaskScope. Since StructureTaskScope enforces that all subtasks end before the enclosing scope ends (which ManagedExecutor does not), there is no problem with the undefined end of the RequestScope.

Having the StructureTaskScope as an enclosing bracket also enables to capture the HttpRequest / backing map where the TaskScope is started and to allow read/write access to it from subtasks started within the scope.

Of course, this does not solve the problem that existing @RequestScoped beans are not written with thread safety in mind and sometimes explicitly rely on being single-threaded (I know I have written such beans in the no so recent past).

@Ladicek
Copy link
Contributor

Ladicek commented Feb 29, 2024

Yes, that would solve one of the problems, partially (only for cases where structured concurrency is a good fit, which should be like 90%, but definitely isn't 100%).

@Ladicek
Copy link
Contributor

Ladicek commented Feb 29, 2024

More broadly, the request scope (and the concept of normal scopes, in general) was designed for the thread per request paradigm.

It was in the days when Java EE prohibited (or discouraged, stricly speaking, because there has been no way to actually prevent it) users from creating their own threads, and there were no managed threads / executors. The Concurrency Utilities for Java EE specification (Jakarta Concurrency, nowadays) has since the very beginning recommended to not use @RequestScoped, @SessionScoped and @ConversationScoped beans as tasks or in tasks, precisely for the lifecycle reason. Thread safety is an additional concern for @RequestScoped beans.

This is increasingly not good enough for today's needs, but based on our experience, I'm fairly convinced that MicroProfile Context Propagation (which was adopted into Jakarta Concurrency verbatim) is not a good solution. What is a good solution, I don't know (yet :-) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants