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

specify optional integration with MicroProfile Context Propagation #565

Closed
wants to merge 4 commits into from

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jun 30, 2020

Resolves #326.

Signed-off-by: Ladislav Thon lthon@redhat.com

@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 30, 2020

This is an initial specification text. If we agree on that, I volunteer to provide TCK tests.

@Ladicek Ladicek force-pushed the context-propagation branch from 2e336fc to 7fd3bee Compare June 30, 2020 15:19
@Ladicek Ladicek mentioned this pull request Jun 30, 2020
Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to leave this until MP Context Propagation revisit the suggestion of defining a minimum set of contexts to be propagated.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 30, 2020

This basically means moving Context Propagation integration out of 3.0, and I remember you wanted Context Propagation in 3.0, so why the change of mind? I thought we all reached a consensus on the call today, and I tried to capture that consensus in this PR.

@Emily-Jiang
Copy link
Member

@Ladicek What I meant your texts on Context Propagation, I have raised this issue . They will discuss it on their next call. We then have a confirmed value on what is propaged.

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Jun 30, 2020

MicroProfile Context Propagation intentionally doesn't define which contexts are propagated and which are cleared by default.
This is left to implementations.

This might not be correct if some conclusion can be reached for the above issue.

@Emily-Jiang
Copy link
Member

This basically means moving Context Propagation integration out of 3.0, and I remember you wanted Context Propagation in 3.0, so why the change of mind? I thought we all reached a consensus on the call today, and I tried to capture that consensus in this PR.

Not really, they will discuss the minimum set in their next meeting.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 1, 2020

I added that as a note to help users. I'm fine with deleting the note.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 1, 2020

Not really, they will discuss the minimum set in their next meeting.

From Fault Tolerance specification perspective, I couldn't care less. What matters is the released specification. If you think Context Propagation folks can, in a matter of few weeks, reach an agreement on something they couldn't agree on for months, then sure, but I have my doubts ;-)

@manovotn
Copy link
Contributor

manovotn commented Jul 1, 2020

I think this PR makes sense, otherwise you basically won't get MP CP into FT in time.
I share the doubts that MP CP will reach that conclusion/agreement in time (if at all).

@Ladicek Ladicek force-pushed the context-propagation branch from 7fd3bee to c9d1c64 Compare July 1, 2020 13:10
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 1, 2020

I tried to expand the note to make the relationship with ConProp more explicit.

Signed-off-by: Ladislav Thon <lthon@redhat.com>
@Ladicek Ladicek force-pushed the context-propagation branch from 727dbf9 to 729bf57 Compare July 16, 2020 08:52
Co-authored-by: Andrew Rouse <anrouse@uk.ibm.com>
Signed-off-by: Ladislav Thon <lthon@redhat.com>
@Ladicek Ladicek force-pushed the context-propagation branch from 729bf57 to be6adc2 Compare July 16, 2020 08:53
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 16, 2020

Added a TCK test. SmallRye Fault Tolerance passes this TCK as is and the test is skipped, and can be easily adjusted to pass this test by adding a few dependencies to the TCK runner module (because we already implement Context Propagation integration in the way it's specified here).

Signed-off-by: Ladislav Thon <lthon@redhat.com>
@Ladicek Ladicek force-pushed the context-propagation branch from 7ea187b to 5d86138 Compare July 17, 2020 15:07
=== MicroProfile Context Propagation

This section describes the expected behaviors when the implementation runs in an environment that provides implementation of MicroProfile Context Propagation.
These behaviors are currently optional because the MicroProfile Context Propagation specification is not part of the MicroProfile Platform.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will say:
These behaviours are only required if a runtime provides the implementation for both MicroProfile Fault Tolerance and MicroProfile Context Propagation Specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rhusar, this is what we talked about over email. Wanna share your perspective?

@Azquelt
Copy link
Member

Azquelt commented Jul 21, 2020

I ran into a problem implementing this.

It seems like in practise there's a limitation to the way that Context Propagation implementations propagate CDI scopes. Rather than having the same context active on both threads, they end up making a new context which references the same beans. This is often the same, but breaks if a bean gets initialized on the async thread. That bean instance is then only on the async thread and not on the original thread.

Details on the limitation here: eclipse/microprofile-context-propagation#167

We have existing tests which do exactly this in AsynchronousTest

Although the tests don't actually check that the context is propagated correctly, openliberty notices that this situation occurs and throws an exception. If we're adding integration with Context Propagation, we should update these tests so that they don't use this pattern which MP Context Propagation doesn't cope with correctly. We would need to make a call to the request scoped bean before we make the @Asynchronous call to ensure that it's initialized.

As a side note, I'm pretty disappointed with this limitation, I think it makes Context Propagation far less useful since the initialization time of CDI beans isn't something the user is supposed to have to worry about when using CDI.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 22, 2020

Interesting. My understanding is that this is underspecified in MP Context Propagation -- I think in Quarkus, the request context is shared between threads, and it's a compatible implementation as well. So this is an implementation decision. I agree that's yet another rather disappointing fact of life :-)

@manovotn
Copy link
Contributor

You are both correct, here is some related info from the top of my head; bits might be missing :)

@Azquelt In Weld, what we do (simplified) is that we create new context on the thread and feed it same instances. This works for most cases but breaks whenever you need what I will call "backward propagation" (there is likely better term for that) - a situation where thread A creates another bean in given context and thread B also wants to access that. In this scenario they both end up creating their own instances.

There are several reasons why it is like this.

  • Firstly, the initial set of use cases in CP didn't require this, we even specifically discussed it and I pointed it out several times.
  • Secondly, the implementation:
    • For req/session/conversation Weld stores the beans in the request (or session) and when you enter new thread, you no longer have access to that request. Hence we spawn a new context, one that doesn't require backing http request.
    • Lifecycle of those scopes is also bound to that of a request/session and the new thread lacks knowledge of that; it also brings up the question of which thread is eligible to destroy the context, or what happens if one thread destroys it and another keeps working on it.
    • Even if you did somehow implement it, it would require heavy synchronization in the bean stores which is an overhead you surely don't want to have outside of CP scenarios.
    • It requires users to write their beans in a thread-safe manner which I highly doubt they do for anything other than app scoped beans, but that can already be an issue; this just underlines it.

@Ladicek Finally, in Quarkus the imlementation of req. context is far simpler and we control the usage - e.g. knowing how we allow users to propagate contexts and that you in fact won't be working on the same context in multiple threads (unless you enforce it) allows us to share the whole context instance, bean store included. Also note that in Quarkus, we do not store those beans in http request.

With all that being said, I am not against doing improvements on Weld side so long as we can keep the non-CP functionality and performance unhindered. So if anyone is willing to dive into this, contributions are welcome and I'll try to assist them as much as I can. Thought it is not going to be an easy task.

CC @mkouba in case you want to add some details I forgot.

@Emily-Jiang
Copy link
Member

From you observed @Azquelt you will have a brandnew requested scoped instance on the new thread. It will be the same content but a different instance. Though it is not ideal, it is ok. How about SessionScoped and ConversationScoped? Does Weld inject a new bean as well @manovotn ?

@manovotn
Copy link
Contributor

All propagated scopes work the same way in Weld.

@Azquelt
Copy link
Member

Azquelt commented Jul 23, 2020

@manovotn Thanks for the writeup, that's really helpful and filled in some gaps in my understanding.

@Ladicek @Emily-Jiang If this limitation is permitted by context propagation, should we adapt our existing TCK test to allow it?

Also, if Context Propagation has its own idea of what it does with the CDI context, do we still want to state the the request context is active in asynchronous methods or do we delegate that to Context Propagation as well?

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 24, 2020

Good point re "request context is active in @Async methods" -- I totally need to add a sentence to the new spec about that. (Basically, if ConProp is present, then this doesn't apply and ConProp takes over.)

When it comes to the existing @Async tests that use the request context... with the exception of tests that verify that request context is active in @Async methods (or generally with the exception of tests that specifically verify something related to request context), would it be best to just use @Dependent?

…Propagation

Signed-off-by: Ladislav Thon <lthon@redhat.com>
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 28, 2020

Added one tiny commit with a note that in presence of Context Propagation, the request context is still active (and also propagated).

====
The _Asynchronous Usage_ chapter specifies that "context for `RequestScoped` must be active during the asynchronous method invocation".
When MicroProfile Context Propagation is used, this still applies.
The difference is that the request context will be not only active, but also propagated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The difference is that the request context will be not only active, but also propagated.
The difference is that the request context may be not only active, but also propagated.

Suggest changing "will" to "may" given that Context Propagation may be configured not to propagate the request context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had "(or cleared, depending on Context Propagation configuration)" at the end of the sentence. For some reason, I thought it's better to remove it :-)

@Emily-Jiang
Copy link
Member

Good point re "request context is active in @async methods" -- I totally need to add a sentence to the new spec about that. (Basically, if ConProp is present, then this doesn't apply and ConProp takes over.)

When it comes to the existing @async tests that use the request context... with the exception of tests that verify that request context is active in @async methods (or generally with the exception of tests that specifically verify something related to request context), would it be best to just use @dependent?

This has a serious impact to the end users. This means the method with @Asynchronous annotated are no longer support @RequestScoped, @ConversationScoped, etc, which is unacceptable limitation. With this hugh limitation, I can't see any value added by integrating with MP Context Propagation. With this said I am -1 to this integration until Weld can provide a working CDI context propagation without asking for the bean being initialised prior to the asynchronous calls.

Copy link
Member

@Emily-Jiang Emily-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained in my comments, I don't think any value gained with the integration but going backwards due to CDI context propagation behaviour. I think it is premature to spec this. We should focus on improving CDI context propagation as mentioned by Matej first. By the way, any runtime can go ahead to do this integration in their implementation if they wish and this spec does not prevent that from happening.

@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 30, 2020

I'm fine with deferring this to a later release because we find the Context Propagation specification unsatisfactory.

At the same time, I would like to point out one thing:

This has a serious impact to the end users. This means the method with @asynchronous annotated are no longer support @RequestScoped, @ConversationScoped, etc, which is unacceptable limitation.

This is simply not true.

@Ladicek Ladicek mentioned this pull request Aug 4, 2020
@Ladicek Ladicek closed this Sep 6, 2023
@Ladicek Ladicek deleted the context-propagation branch September 6, 2023 11:34
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

Successfully merging this pull request may close these issues.

Context propagation
4 participants