Open Emily-Jiang opened 3 years ago
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?
Does this https://issues.redhat.com/browse/CDI-452 fall under this issue or is it different? Asking as to not open a duplicate.
@cen1 I think the issue https://issues.redhat.com/browse/CDI-452 falls under this issue.
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.
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.
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
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.
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".
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.
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.
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.
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.
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
.)
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 aroundHttpServletRequest
, storing contextual instances into theHttpServletRequest
attributes. There's even an implementation for EJBs that is a wrapper aroundInvocationContext
, which stores contextual instances into the context data map. (Similarly, the session and conversation contexts, in the HTTP servlet scenario, are wrappers aroundHttpSession
.)
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.
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.
The Context Propagation lib was used in the latest Spring 6.0/Spring Boot 3.0. https://micrometer.io/docs/contextPropagation
Curious where this stands at the moment. Is this being looked at for a subsequent release? //cc @Emily-Jiang
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 - https://github.com/jakartaee/cdi/issues/474#issuecomment-1252554813
@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)
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).
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%).
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 :-) ).
In CDI spec, the context propagation was not addressed in the spec. The original ticket was raised:
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 https://github.com/eclipse/microprofile-context-propagation/issues/167