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

Ensures that the enabled flag is honored in JpaExtension's observer methods #8235

Merged

Conversation

ljnelson
Copy link
Member

This PR ensures that the observer method at the bottom of the JpaExtension class honors the enabled flag like other container-invoked methods in the class. Small oversight; shouldn't have any real-world effects either way.

Documentation impact: none.

…ethods

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
@ljnelson ljnelson added bug Something isn't working MP P4 cdi CDI jpa/jta integration java Pull requests that update Java code 4.x Version 4.x labels Jan 12, 2024
@ljnelson ljnelson self-assigned this Jan 12, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 12, 2024
…ricsFactory in an @afterall rule

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
@ljnelson ljnelson added this to Sprint Scope in Backlog Jan 13, 2024
@ljnelson
Copy link
Member Author

ljnelson commented Jan 13, 2024

This pull request in its current state causes Weld to not be able to find things out about itself (!), but only when run in the pipeline (i.e. not locally):

io.helidon.integrations.cdi.jpa.TestAnnotationRewriting.testTransactionalEntityManager  Time elapsed: 1.486 s  <<< ERROR!
java.lang.IllegalArgumentException: Unable to determine Bean metadata for org.jboss.weld.manager.BeanManagerImpl$InstanceInjectionPoint@4a070cf0

This may have to do with Weld's getReference implementation, core to all of its dependency injection. I need to determine whether this is a problem with Weld 5.1.1.SP2 or our repackaging of the core and the static work we do to make Weld work with native image.

See https://github.com/weld/core/blob/5.1.1.SP2/impl/src/main/java/org/jboss/weld/bean/builtin/BeanMetadataBean.java#L59. This is basically an assertion error in Weld itself; Weld cannot find an instance of Bean (!), but only when run in the pipeline.

I have no explanation yet for why running only in the pipeline would trigger an assertion error in Weld. I will retry the failing build a few times to ensure this isn't a pipeline-induced race condition or something of that nature (though the pipeline is single-threaded, as are the tests).

It is at least conceivable this is related to #8122.

@ljnelson
Copy link
Member Author

Can reproduce locally with, e.g., mvn clean install -Dtest='TestAnnotationRewriting#testAnnotationRewriting (runs just one test locally)

@ljnelson
Copy link
Member Author

Working theory: when TestAnnotationRewriting runs first (any test in it), you get this problem. If one of the more rigorous tests later runs first, you do not get this problem. On my Mac, other tests besides TestAnnotationRewriting run first and everything works fine.

Now need to see what's different. This is, at root, some kind of a Weld issue pertaining to very specific use cases where Bean metadata is not available.

The first thing that occurs to me is the injection scenario here is: observerMethod in TestAnnotationRewriting needs to be invoked, which requires its declaring class to be instantiated. That requires the declaring class' fields to be injected. It is at this point that Weld goes bananas. Line 63 of TestAnnotationRewriting is:

@PersistenceUnit(unitName = "test-resource-local")
private EntityManagerFactory emf;

…but there is something odd about trying to satisfy this (perfectly legal) injection point of an object (TestAnnotationRewriting itself) being instantiated for the purpose of having an observer method invoked on it that triggers this failure. I'm pretty sure other tests don't follow this pattern. The question is: what codepath in Weld is followed in this case, that is not followed in a more vanilla case?

…Bean metadata creation in certain codepaths

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
@ljnelson
Copy link
Member Author

This is a wild one.

I believe that the Weld bug or feature or codepath referred to above is:

If the first encounter of demand for a contextual reference occurs as part of bringing up another contextual reference so that an observer method on it can invoked—specifically when the observer method "host" is being injected with its dependencies—then (and perhaps only then) the (required) Bean metadata that Weld is obligated to create and offer to bean producers that want to use it is not created.

TestAnnotationRewriting is a test for which this is true: the first encounter of EntityManagerFactory occurs when an observer method needs to be invoked, so a TestAnnotationRewriting instance to "host" it needs to be created, and so its EntityManagerFactory-typed field needs to be injected. In this codepath alone, Weld will not create the Bean<EntityManagerFactory> that PersistenceExtension#produceEntityManagerFactory(Instance) needs. The cause appears to be something strange in the CreationalContextImpl tree-like structure that it builds to keep track of who is injecting what into whom: in the case of bringing up an observer method "host", no one, really, is injecting it into anything, and that confuses the tree such that the thing that is supposed to be non-null ends up being null and you get https://github.com/weld/core/blob/5.1.1.SP2/impl/src/main/java/org/jboss/weld/bean/builtin/BeanMetadataBean.java#L59.

When tests (in the pipeline or anywhere) run in a different order, this state of affairs does not hold when this test runs. That of course shouldn't be the case, unless I'm missing something: surely this test should fail no matter where it runs since it creates a brand new container every time.

My unconfirmed suspicion here is that because of the work we've done to make Weld run in native image there are static-like things lying around statically in static fields or something like that that persist (incorrectly?) across tests (maybe in HelidonContainerImpl?). I'm not fully sure of this part yet. Maybe other tests are not shutting down their containers properly? Haven't looked yet.

At any rate, the easy workaround here is to ensure that an observer method that fires on container startup simply iterates through the EntityManagerFactory instances. Then the codepath described above will never be the first, and the Weld bug or feature or whatever it is will not be seen.

I will look at the Weld codebase in detail later to see why the parent CreationalContextImpl doesn't have a bean in this one case.

Finally, the archetype-related changes in this PR are all due to the hard work of @tjquinno who tracked down bad test initialization in the Helidon archetype (metrics were being cleared in a @BeforeAll rule, not an @AfterAll rule as (he tells me) should have been the case.

@ljnelson ljnelson merged commit 224f99d into helidon-io:main Jan 14, 2024
12 checks passed
Backlog automation moved this from Sprint Scope to Closed Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x bug Something isn't working cdi CDI integration java Pull requests that update Java code jpa/jta MP OCA Verified All contributors have signed the Oracle Contributor Agreement. P4
Projects
Backlog
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants