Context propagation

[pavolloffay]

Hi folks,

@Asynchronous annotation and maybe others execute the method on a different thread which breaks context propagated via thread locals. If these methods use mp-rest-client it breaks tracing or e.g. MDC.

On the spec level there could be a way to reliably propagate context between threads. It could be done by providing custom executor service or maybe via https://github.com/eclipse/microprofile-concurrency

[pavolloffay]

Cross-linking issue with mp-ot microprofile/microprofile-opentracing#108

[Emily-Jiang]

@pavolloffay we are fully aware of this, which leads to the specification of microprofile concurrency. We plan to do minimal in this spec for context propagation but rely on concurrency spec to fix the propagation issues.

[pavolloffay]

@Emily-Jiang thanks for the response. I could not find any references in FT repo on this feature. It would be good to create an issue there for people to subscribe.

Do you know what is the timeline for this to land in concurrency?

[Emily-Jiang]

MP Concurrency aims for MP 2.2 release.

[Emily-Jiang]

We need to revisit the discussion on whether and when to add MP Context Propagation to the MP umbrella release so that FT can integrate with Context Propagation.

[Azquelt]

Possible design:

@Asynchronous(propagated = ThreadContext.CDI,
              cleared = ThreadContext.SECURITY,
              unchanged = ThreadContext.ALL_REMAINING)
public CompletionStage<String> myMethod();

This would require the Fault Tolerance implementation to create a ThreadContext when the method is called and use it to clear and propagate the requested contexts when the asynchronous task runs.

[Ladicek]

One interesting thing is that ThreadContext.Builder has 3 methods: cleared, propagated, unchanged, while ManagedExecutor.Builder has only 2: cleared, propagated.

Not sure why is that.

CC @manovotn

[Azquelt]

We might require certain contexts to be propagated or cleared by default (either matching the default in the context propagation spec, which also allows it to be configured, or matching the current behaviour required by Fault Tolerance.)

We could test this both with the built-in contexts (e.g. check that the CDI RequestContext is propagated to the async task) and by creating our own custom test context.

If we test using the built-in contexts, we'd need to check what is actually verified by the context propagation TCK.

[manovotn]

Good question @Ladicek. Frankly, I am not sure, it probably has something to do with ThreadContext being applicable to any thread out there that may or may not have certain context on them once you get hold of it whereas with ManagedExecutor you have control over threads it operates with.

Maybe @njr-11 or @FroMage remember?

[Ladicek]

It also seems that there used to be a notion of "default contexts that are propagated/cleared", eclipse/microprofile-context-propagation#27, but that somehow disappeared. I don't know the details, but it seems unfortunate.

[njr-11]

@Ladicek the spec did not define the default set of contexts to propagate because we weren't able to reach a consensus on what the default set ought to be (specifically, whether to include transaction context). So the defaults are currently vendor-specific, but a future update to the spec could clarify.

Regarding,

One interesting thing is that ThreadContext.Builder has 3 methods: cleared, propagated, unchanged, while ManagedExecutor.Builder has only 2: cleared, propagated.

the above is due to a difference in usage patterns. ManagedExecutor supplies threads (typically from a pool) on which to run async requests, in which case the concept of having a pre-existing context on the thread that you would want to leave unchanged is meaningless and otherwise confusing. Whereas the ThreadContext pattern often involves running inline on a user-supplied thread where it makes sense that you may often want to leave some context unchanged. That said, there are cases where lines do get blurred between the two patterns and it could actually be useful to have unchanged for a ManagedExecutor. It's not something we had ruled out ever adding, just something that we didn't include initially, lacking a compelling reason to include it. If you have some good scenarios where it would be useful, let us know what they are, and maybe open an issue against the context propagation spec to consider adding it.

[Ladicek]

Thanks @njr-11, this is very useful.

I propose, for FT purposes, to:

1. Only make the propagated and cleared contexts configurable in @Asynchronous, because it's the same situation as ManagedExecutor per @njr-11's description above. (That is, it's an extra thread pool with no pre-existing contexts.)
2. Stay in line with ConProp when it comes to default propagated contexts. In addition, we should be explicit about the fact that it's not specified currently.

[manovotn]

@njr-11 thanks for refreshing memory on that.
Defaults were indeed ruled out. If memory serves, we (in SR) wanted to propagate all whereas in Liberty impl they wanted to avoid propagating transactions by default. You can however set some defaults via MP config (e.g. globally for all propagation you do in your app). See https://github.com/eclipse/microprofile-context-propagation/blob/master/spec/src/main/asciidoc/mpconfig.asciidoc

@Ladicek the other tricky part is the contexts you don't specify explicitly even is you list some in propagated or cleared, e.g. those represented by ALL_REMAINING. You'll need to decide if those are cleared or propagated. For instance ManagedExecutor.Builder#cleared() javadoc says that

ThreadContext.ALL_REMAINING is automatically appended to the set of cleared context if the propagated(java.lang.String...) set does not include ThreadContext.ALL_REMAINING.

[Ladicek]

I think we want to stay consistent with ConProp as much as possible, including when it comes to ALL_REMAINING.

[Emily-Jiang]

What do you mean by saying ConProp @Ladicek ? In Fault Tolerance spec, the spec already said the security context and application context are propagated. In Context Propagation, it is ok not to specify default as it does not know the scenario where in Fault Tolerance we have all of the information. I also think by default propagating CDI Context is expected. Another reason for Context Propagation not to specify default is due to the different opinions on Transaction context.

[Ladicek]

I use "ConProp" to mean "Context Propagation", because it's faster to write :-)

Unfortunately, the term has multiple meanings.

One, as you mention, is that in Jakarta EE environment, we specify that the naming and security contexts should be propagated.

Another is MicroProfile Context Propagation -- a completely different thing. We currently don't integrate with that at all.

This issue is about MP ConProp -- the "naming and security contexts are propagated" are a different thing. Integrating MP ConProp could subsume that, under certain circumstances. (MP ConProp would have to have a "naming" context defined, perhaps as part of the APPLICATION context, which is currently underspecified. Also, MP ConProp would have to be part of the MP Platform, so that we could really rely on it being present.)

[Emily-Jiang]

@Ladicek thanks. I am not good at guess the acronyms :o. Prop always translates to properties for me. Maybe I am too close to MP Config.
In yesterday's MP hangout, I raised the question on how to handle integrating MP Context Propagation to MP Fault Tolerance. I suggested to go with optinal. It was agreed upon and I have sent out a note so that other specs can use the same pattern.
As for APPLICATION context, from what @njr-11 replied to my gitter question, it is quite clear.

Application context includes the java:comp, java:module, and so forth JNDI namespace of the application component, as well as its thread context class loader..
Transaction context propagation means that the task or completion stage runs under the same transaction of the thread that submits the task or creates the stage. If the task or completion stage wants to roll back the transaction, it can do so with the usual rollback API, or it can mark the transaction for rollback with setRollbackOnly. The spec does not have anything where we auto-rollback due to a failure, which would limit flexibility of implementation by forcing rollback on exception paths.
Transaction context propagation also covers propagation of the lack of any transaction to a thread. So if the original thread was not running in a transaction to start with, then the task or completion stage will also run without any active transaction on the thread.

[Ladicek]

Application context includes the java:comp, java:module, and so forth JNDI namespace of the application component, as well as its thread context class loader..

That sounds nice, but isn't what the MP ConProp spec says, nor what its TCK tests. Specifically, the spec says that "it can determine the thread context class loader as well as the set of resource references that are available for lookup or resource injection", which is quite vague. The TCK test for the APPLICATION context doesn't test JNDI contexts at all (no wonder -- MicroProfile doesn't know anything about JNDI), and its test for the TCCL propagation doesn't really test anything. I'm aware of one MP ConProp implementation that fully passes the TCK, but doesn't propagate TCCL.

how to handle integrating MP Context Propagation to MP Fault Tolerance. I suggested to go with optinal

We can do that, but then we can't rely on it for propagating the naming and security contexts and probably need to keep the Jakarta EE-specific part of the spec.

[Emily-Jiang]

Application context includes the java:comp, java:module, and so forth JNDI namespace of the application component, as well as its thread context class loader..

That sounds nice, but isn't what the MP ConProp spec says, nor what its TCK tests. Specifically, the spec says that "it can determine the thread context class loader as well as the set of resource references that are available for lookup or resource injection", which is quite vague. The TCK test for the APPLICATION context doesn't test JNDI contexts at all (no wonder -- MicroProfile doesn't know anything about JNDI), and its test for the TCCL propagation doesn't really test anything. I'm aware of one MP ConProp implementation that fully passes the TCK, but doesn't propagate TCCL.

I raised the following issue for MP Context Propagation to fix eclipse/microprofile-context-propagation#192

We can do that, but then we can't rely on it for propagating the naming and security contexts and probably need to keep the Jakarta EE-specific part of the spec.

Agreed.

[njr-11]

The general language describing Application context and other context types is the unfortunate consequence of trying to write a spec that can be implemented by application servers as well as other containers/implementations which are not. Originally, the wording was more application server-specific and could mention things like java:comp and JNDI, but we had to make it more general to accommodate other implementations.

[Emily-Jiang]

I thought about this further and come up with the following proposal:

• In the spec doc on Jakarta EE context, we could add CDI Context in addition to security and naming context.
  So the spec should read like:

@Asynchronous
Threads that are servicing @asynchronous invocations should, for the duration of the invocation, have the correct security context, CDI and naming context associated._

• On API level: we could define the following two methods on @Asynchronous
propagated=’’, cleared=’’
@Asynchronous(propagated=ThreadContext.CDI, cleared=ThreadContext.Transaction)
  What specified on the annotation should override the default behaviour specified in the above bulletin point.

[Ladicek]

I have submitted an initial specification text proposal: #565. Comments welcome.

[Emily-Jiang]

Support integration with context propagation
Advantages:
+: Tcks - runtime can verify against the tck
+: end users knows some runtime might support context propagation
+: any methods with @Asynchronous will get some context propagated, defined by Context Propagation.
+: It will specify what will happen when both Jakarta EE and Context Propagation are present. Proposed: Context Propagation should win.

The integration is optional. Not portable.

Disadvantages:
1: end users don't know what contexts are propagated. It varies between runtimes.
- User can configure context propagation explicitly via context propagation properties for portability
2: end users don't which runtimes support this.
- Maybe we reword to say the integration is required if both FT and CP are present. TBC
3: More Spec and TCKs to maintain if the end users can't benefit from this feature.