-
Notifications
You must be signed in to change notification settings - Fork 81
improve application scope and extensions #927
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
base: main
Are you sure you want to change the base?
Conversation
|
When we merge this, I also need to submit a PR to the platform spec to update https://github.com/jakartaee/platform/blob/main/specification/src/main/asciidoc/platform/cdi-ee-spec/scopescontext_ee.adoc#context-management-for-built-in-scopes-in-jakarta-ee |
With the recent change in CDI itself [1], the application scope is now always active. The CDI EE specification no longer has to specify this, therefore this commit removes most of the specification text. What remains is the specification of the payload of the event fired when the application context is initialized or destroyed. [1] jakartaee/cdi#927
|
For the record, what I propose to submit to the Jakarta Platform spec is at https://github.com/Ladicek/jakartaee-platform/commits/improve-app-scope/ |
| An event with qualifier `@BeforeDestroyed(ApplicationScoped.class)` is synchronously fired when the application context is about to be destroyed, i.e. before the actual destruction. | ||
| An event with qualifier `@Destroyed(ApplicationScoped.class)` is synchronously fired when the application context is destroyed, i.e. after the actual destruction. | ||
|
|
||
| The application context is always active, since the event with qualifier `@Initialized(ApplicationScoped.class)` is fired and until the event with qualifier `@Destroyed(ApplicationScoped.class)` is fired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would rather frame the usability of the context by two events in which you can still use the context (@Initialized -> @BeforeDestroyed) but I suppose I can live with @Destroyed as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During @BeforeDestroyed notification, the application context is still guaranteed to be active. So I don't think you can say "until @BeforeDestroyed is fired", because that might be read as "during @BeforeDestroyed, the context is not active".
I think we could make it more precise by saying:
The application context is always active, starting after the event with qualifier
@Initialized(ApplicationScoped.class)is fired and ending before the event with qualifier@Destroyed(ApplicationScoped.class)is fired.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though to be honest, we simply say
The
@Dependentscope is always active.
and don't bother specifying when it starts and when it ends, even though there clearly are such points in time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During
@BeforeDestroyednotification, the application context is still guaranteed to be active. So I don't think you can say "until@BeforeDestroyedis fired", because that might be read as "during@BeforeDestroyed, the context is not active".I think we could make it more precise by saying:
The application context is always active, starting after the event with qualifier
@Initialized(ApplicationScoped.class)is fired and ending before the event with qualifier@Destroyed(ApplicationScoped.class)is fired.WDYT?
I was thinking something along the lines of:
The application context is always active beginning with firing of event with qualifier
@Initialized(ApplicationScoped.class)up to and including the notification of@BeforeDestroyed(ApplicationScoped.class)observers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though to be honest, we simply say
The
@Dependentscope is always active.and don't bother specifying when it starts and when it ends, even though there clearly are such points in time.
The @Dependent scope doesn't fire the init/destroy events though. At least I think it doesn't as it won't make sense to observe those anyway 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards this:
The application context is always active, starting before the event with qualifier
@Initialized(ApplicationScoped.class)is fired and ending after the event with qualifier@BeforeDestroyed(ApplicationScoped.class)is fired.
This is because during notification of @Initialized(AppScoped.class), the app scope is already active, so it must start before. For symmetry, I'm saying it ends after @BeforeDestroyed(AppScoped.class). I could also say it ends "before @Destroyed(AppScoped.class)", but I don't like it as much and one could argue it would be somewhat less precise :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry for nit picking but re-reading this section of the specification again, I am wondering if we aren't just re-stating the same thing.
The very few sentences prior to your addition already explain the events that mark the boundaries of app context lifecycle 🤔
What I mean is we already have this:
==== Application context lifecycle
The _application context_ is provided by a built-in context object for the built-in scope type `@ApplicationScoped`.
An event with qualifier `@Initialized(ApplicationScoped.class)` is synchronously fired when the application context is initialized.
An event with qualifier `@BeforeDestroyed(ApplicationScoped.class)` is synchronously fired when the application context is about to be destroyed, i.e. before the actual destruction.
An event with qualifier `@Destroyed(ApplicationScoped.class)` is synchronously fired when the application context is destroyed, i.e. after the actual destruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're fine with this just saying
The application scope is always active.
? :-)
That would be fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're fine with this just saying
The application scope is always active.
? :-)
That would be fine to me.
Yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
manovotn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am in favor of this PR as it makes the spec text more concise and easier to read 👍
For the record, what I propose to submit to the Jakarta Platform spec is at https://github.com/Ladicek/jakartaee-platform/commits/improve-app-scope/
Seems good, I'd just remark in the PR that the wording implies no change in the actual behavior.
| * The application scope is always active, since the event with qualifier {@code @Initialized(ApplicationScoped.class)} | ||
| * is fired and until the event with qualifier {@code @Destroyed(ApplicationScoped.class)} is fired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use of "since" here is wrong ("since" is usually for something that started in the past and is ongoing).
It also seems odd to say that the scope is always active and then place limits on when it's active.
| * The application scope is always active, since the event with qualifier {@code @Initialized(ApplicationScoped.class)} | |
| * is fired and until the event with qualifier {@code @Destroyed(ApplicationScoped.class)} is fired. | |
| * The application scope is active from when the event with qualifier {@code @Initialized(ApplicationScoped.class)} | |
| * is fired until the event with qualifier {@code @Destroyed(ApplicationScoped.class)} is fired. |
I'm still thinking about the change as a whole, and I'm not totally happy with it yet, but I think this suggestion to fix the wording here is necessary if we do keep the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also seems odd to say that the scope is always active and then place limits on when it's active.
Well, yes, I agree. However, we need to somehow set the limits on when you can start and stop using it while at the same time explaining that within those bounds it is just "always active".
I will gladly accept help of a native speaker in that regard :)
Also note the discussion we had earlier - #927 (comment)
I think we need to somehow merge that into the existing text...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just got rid of this complexity as agreed with @manovotn above :-) This part of javadoc now reads:
The application context is always active. It is destroyed when the application is shut down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Azquelt what is your take on the current state of the PR? Does it look better this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think "always" is too strong a requirement. At the very least the scope can only be active while the singular application context is active, and I would expect that some lifecycle events would occur before that.
In a tutorial or something, I think saying it's always active would be fine, but I'm not sure we can get away with that in the spec.
My second issue is that I'm still bothered by the fact that in an application server, there will probably be times where code is executing that's not part of an application and therefore there's no active application context on that thread, and therefore the scope is not active. Normally user code should not be running at this point, perhaps that's invisible to the user, but the reality of different applications being active on different threads makes the idea of saying that the scope is always active seem wrong.
I prefer the existing definition of when the application scope is active for Jakarta EE.
33e673b to
54f28c0
Compare
The application scope is now specified to be always active. This makes the specification simpler and more aligned with existing implementations, which already behave like that. (Registering custom application context either fails, in Weld, or is silently ignored, in ArC or OWB.) Extensions are now specified to cause a deployment problem if registration of a custom context for an always active built-in scope is attempted. The dependent context was already specified as always active, and per previous paragraph, the application context is now as well. The spec never talks about the singleton context, but that is naturally also always active. Further, build compatible extensions are now specified to allow registering custom contexts for the same set of scopes as portable extensions. Previously, registering custom contexts for built-in scopes was forbidden, which is too limiting and was never enforced (by the TCK and by implementations).
54f28c0 to
bcacf3c
Compare
|
Added an extra commit that defines global scopes. I think that definition is reasonably precise, even to address @Azquelt's objections. To quote:
Before merging the PR, I think I would prefer to squash the commits and reword the commit message, because these subjects are deeply intertwined. |
|
I think that definition makes sense and I am happy with it. I think this wording for the last sentence would be slightly better:
|
|
I improved the definition of global scopes slightly. Activation and deactivation are operations that are not defined for all contexts, so I got rid of those. The new definition reads:
You'll note that the new definition says when the context object is active, not the scope. That is fine, because the scope is also active due to this provision: https://jakarta.ee/specifications/cdi/4.1/jakarta-cdi-spec-4.1.html#active_context |
8afda17 to
4243913
Compare
Certain built-in scopes are defined to be _global_ (_always active_ in previous parlance). A global scope has a single context object and is active on all application threads for the duration of the application.
4243913 to
efff6fb
Compare
|
I improved the definition of global scopes somewhat and squashed the extra commits into one (which is still to be squashed with the first commit in this PR). There's still an illusion of a cycle between:
and
I say illusion because the
But then there's a few more places that should be improved, and I didn't feel like attacking that problem right now. Perhaps I should. CC @manovotn as agreed on the CDI call today :-) |
manovotn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have no objections 👍
The application scope is now specified to be always active. This makes the specification simpler and more aligned with existing implementations, which already behave like that. (Registering custom application context either fails, in Weld, or is silently ignored, in ArC or OWB.)
Extensions are now specified to cause a deployment problem if registration of a custom context for an always active built-in scope is attempted. The dependent context was already specified as always active, and per previous paragraph, the application context is now as well. The spec never talks about the singleton context, but that is naturally also always active.
Further, build compatible extensions are now specified to allow registering custom contexts for the same set of scopes as portable extensions. Previously, registering custom contexts for built-in scopes was forbidden, which is too limiting and was never enforced (by the TCK and by implementations).
Fixes #925