ManagedBeanRegistry CDI lifecycle fixes #2092

yrodiere · 2017-12-20T12:35:53Z

@sebersole Here are the fixes I mentioned yesterday regarding the ManagedBeanRegistry. Mainly this is about making sure that, when shouldRegistryManageLifecycle is false, the registry really doesn't manage the CDI lifecycle at all and delegates to the CDI runtime.

All of the commits in this PR are follow-ups to already-fixed tickets:

WARNING: I had to use CDI 2.0 APIs, and those APIs are not available in WildFly 11. So I added a build step to patch the WildFly server.

yrodiere · 2017-12-20T17:15:50Z

@sebersole Following @Sanne's advice, I managed to avoid the switch to WildFly 12. This PR now targets WildFly 11, with only a patch file applied to the server to upgrade Weld to a version supporting CDI 2.0.

sebersole

Overall these seem like some very complicated changes. I'm going to have to sit down and look through them

sebersole · 2017-12-22T21:48:56Z

...e/src/main/java/org/hibernate/resource/beans/internal/JpaCdiLifecycleManagementStrategy.java

+ * even if there is already an instance in the CDI context.
+ * This means singletons are not really singletons, but this seems to be the behavior required by
+ * the JPA 2.2 spec.
+ */


But see, this is not true. For a given class we only ever generate one ManagedBean instance = they are singletons (unless of course they are used again in a non-JPA context). Notice the caching of them initially in a Map keyed by class...

unless of course they are used again in a non-JPA context

My point exactly. They are singleton in the JPA context only, not in the CDI context overall. I can rephrase the comment to clarify that if you like.

Also, even in the JPA context, such "not-actually-singleton" beans will be instantiated once per ServiceRegistry, which unless I'm mistaken means once per SessionFactory/persistence unit. So even from the JPA point of view, they cannot always be viewed as singletons.

yrodiere · 2018-01-05T17:32:24Z

Now working on tests as discussed; I couldn't finish today, but I'll probably be able to push something on Monday.

yrodiere · 2018-01-08T14:03:35Z

Added non-regression tests as requested.

sebersole · 2018-01-12T13:28:19Z

@yrodiere Just because it is easier to ask than to see it in the PR.... you are using CDI 2.0's SE bootstrap now, right? Instead of using WildFly?

yrodiere · 2018-01-12T13:30:00Z

@sebersole Yes. I did exactly what you did in your own tests.

sebersole · 2018-01-12T13:36:59Z

I do still see CDI 2.0 patches against WildFly though : https://github.com/hibernate/hibernate-orm/pull/2092/files#diff-1d01abe59e76ef186ae0415fefca546d Is that just for completeness? If we are not testing "CDI in WildFly" (and we should not be), then this should not be needed.

…

On Fri, Jan 12, 2018 at 7:30 AM Yoann Rodière ***@***.***> wrote: @sebersole <https://github.com/sebersole> Yes. I did exactly what you did in your own tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2092 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUE-8Y6plQY-6tn1OX8z-ldZRaypa4ks5tJ15YgaJpZM4RIVGj> .

yrodiere · 2018-01-12T13:44:04Z

Is that just for completeness?

It is. If anything relies on CDI in your WildFly ITs, without this patch the IT will probably fail.

If you don't think you'll ever use CDI in your WildFly ITs, then yes you can skip this commit. I can remove it if you want.

sebersole · 2018-01-12T14:34:34Z

We currently do not do any in-WF CDI tests (we used to, but I changed that to use the SE bootstrapping). IMO that's outside the scope of what we should be testing. I can just remove that as I integrate your PR locally. Or you can if you really want to :)

…

On Fri, Jan 12, 2018 at 7:44 AM Yoann Rodière ***@***.***> wrote: Is that just for completeness? It is. If anything relies on CDI in your WildFly ITs, without this patch the IT will probably fail. If you don't think you'll ever use CDI in your WildFly ITs, then yes you can skip this commit. I can remove it if you want. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2092 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUE27zvD_Zsyj_8ceCPLh2-nhyKQ0Vks5tJ2GlgaJpZM4RIVGj> .

This commit should not change the current behavior, it is only about moving code to separate classes to make the following changes clearer.

…epending on the 'shouldRegistryManageLifecycle' parameter The registry should not manage the bean lifecycle when 'shouldRegistryManageLifecycle' is false. The easiest way to do so is to use BeanManager.createInstance to retrieve beans in the Standard CDI lifecycle strategy: it correctly retrieves singletons from the CDI context instead of instantiating them again. Also, fix javax.enterprise.inject.spi.Bean-based instance destructions: we used to only request destruction to the creational context, which is wrong because it may skip the execution of @PostDestroy methods in particular.

@Alternative

…n when possible This should take care of @Alternative in particular.

…sed beans

…ryManageLifecycle = false

yrodiere · 2018-01-12T14:47:14Z

I can just remove that as I integrate your PR locally. Or you can if you
really want to :)

I rebased on master and removed the commit.

sebersole · 2018-01-12T15:00:18Z

Ok, thanks. I am integrating this now.

…

On Fri, Jan 12, 2018 at 8:47 AM Yoann Rodière ***@***.***> wrote: I can just remove that as I integrate your PR locally. Or you can if you really want to :) I rebased on master and removed the commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2092 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOUE3XCf6Xo92ri27J2CRQQypHhvXXKks5tJ3BzgaJpZM4RIVGj> .

sebersole · 2018-01-16T19:46:44Z

@yrodiere I did some work on top of this as discussed and pushed upstream in prep for tomorrow's Beta1 release. It's just a Beta, so if you find anything we need to change its not a big deal - we'll get it into the next one

yrodiere added the Preview label Dec 20, 2017

yrodiere force-pushed the bean_registry_cdi_lifecycle_fixes branch from 1d4fb72 to f0f1c8f Compare December 20, 2017 17:11

yrodiere changed the title ~~[PREVIEW] ManagedBeanRegistry CDI lifecycle fixes~~ ManagedBeanRegistry CDI lifecycle fixes Dec 20, 2017

yrodiere added Ready for review and removed Preview labels Dec 20, 2017

yrodiere requested a review from sebersole December 20, 2017 17:14

yrodiere mentioned this pull request Dec 20, 2017

HSEARCH-2978 + HSEARCH-2979 Target CDI 2.0 + upgrade to ORM 5.3.0.Beta1/JPA 2.2 + use ORM ManagedBeanRegistry for FieldBridge injection hibernate/hibernate-search#1598

Merged

yrodiere force-pushed the bean_registry_cdi_lifecycle_fixes branch from f0f1c8f to 3d24c1c Compare December 21, 2017 08:49

sebersole reviewed Dec 22, 2017

View reviewed changes

yrodiere force-pushed the bean_registry_cdi_lifecycle_fixes branch 2 times, most recently from ede1255 to 65bc115 Compare January 5, 2018 16:21

yrodiere force-pushed the bean_registry_cdi_lifecycle_fixes branch 2 times, most recently from cf51133 to 3b600f5 Compare January 8, 2018 14:03

yrodiere force-pushed the bean_registry_cdi_lifecycle_fixes branch from 3b600f5 to a4dfcc8 Compare January 10, 2018 16:22

sebersole self-assigned this Jan 12, 2018

yrodiere added 5 commits January 12, 2018 15:46

HHH-12133 Move CDI lifecycle management code to dedicated strategies

cc4758e

This commit should not change the current behavior, it is only about moving code to separate classes to make the following changes clearer.

HHH-12133 Use the BeanManager-provided ambiguous dependency resolutio…

8a7d44a

…n when possible This should take care of @Alternative in particular.

HHH-12133 Ignore managed bean release errors related to already relea…

4b85bd7

…sed beans

HHH-12133 Add tests for uses of ManagedBeanRegistry with shouldRegist…

3f67a2d

…ryManageLifecycle = false

yrodiere force-pushed the bean_registry_cdi_lifecycle_fixes branch from a4dfcc8 to 3f67a2d Compare January 12, 2018 14:46

sebersole closed this Jan 16, 2018

Uh oh!

ManagedBeanRegistry CDI lifecycle fixes #2092

ManagedBeanRegistry CDI lifecycle fixes #2092

Uh oh!

Conversation

yrodiere commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yrodiere commented Dec 20, 2017

Uh oh!

sebersole left a comment

Choose a reason for hiding this comment

Uh oh!

sebersole Dec 22, 2017

Choose a reason for hiding this comment

Uh oh!

yrodiere Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

yrodiere Jan 3, 2018

Choose a reason for hiding this comment

Uh oh!

yrodiere commented Jan 5, 2018

Uh oh!

yrodiere commented Jan 8, 2018

Uh oh!

sebersole commented Jan 12, 2018

Uh oh!

yrodiere commented Jan 12, 2018

Uh oh!

sebersole commented Jan 12, 2018 via email

Uh oh!

yrodiere commented Jan 12, 2018

Uh oh!

sebersole commented Jan 12, 2018 via email

Uh oh!

yrodiere commented Jan 12, 2018

Uh oh!

sebersole commented Jan 12, 2018 via email

Uh oh!

sebersole commented Jan 16, 2018

Uh oh!

Uh oh!

yrodiere commented Dec 20, 2017 •

edited

Loading