jakartaee / cdi

CDI specification
Apache License 2.0
215 stars 78 forks source link

Circular dependencies of non-Normal scopes #746

Closed nlisker closed 10 months ago

nlisker commented 10 months ago

The specs specify that

2.4 Dependency injection and lookup The container is required to support circularities in the bean dependency graph where at least one bean participating in every circular chain of dependencies has a normal scope, as defined in Normal scopes and pseudo-scopes. The container is not required to support circular chains of dependencies where every bean participating in the chain has a pseudo-scope.

I would like to inquire about this. Suppose I have 2 circularly dependent @Dependent with no state (I will add the EJB @Stateless to them too):

@Stateless
@Dependent
public class SL1 {

    @Inject
    SL2 sl2;
}

@Stateless
@Dependent
public class SL2 {

    @Inject
    SL1 sl1;
}

Why is it a problem to satisfy the injections here? Any couple will satisfy this chain (and in this case they are pooled, though I don't think it should matter).

Now suppose I add a @RequestScoped bean:

@RequestScoped
public class RS {}

and inject it also into SL1 and SL2. In this case, since both should share the same RS instance, the same two SL1 and SL2 instances can be injected into each other, solving the circularity.

manovotn commented 10 months ago

Firstly, the EJB plays no part in this, from CDI perspective its just circular dep of two dependent beans. Now, @Dependent is a non-normal scoped bean and as such the when you inject it, there is no client proxy, you are directly injecting a reference to the contextual instance. This is an issue as the CDI is trying to create an instance of SL1 and inject into it before returning it back (as part of the lifecycle management) and for that it needs an instance of SL2 which it also needs to inject into.

In more spec terms, you can look into this parts - https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#injection_and_resolution Namely:

The container is required to support circularities in the bean dependency graph where at least one bean participating in every circular chain of dependencies has a normal scope, as defined in Normal scopes and pseudo-scopes. The container is not required to support circular chains of dependencies where every bean participating in the chain has a pseudo-scope.

Which is precisely why this works with request scope in place, but not without it.

nlisker commented 10 months ago

CDI is trying to create an instance of SL1 and for that it needs an instance of SL2 and vice versa.

Alright, that explains it mostly.

The container is required to support circularities in the bean dependency graph where at least one bean participating in every circular chain of dependencies has a normal scope, as defined in Normal scopes and pseudo-scopes. The container is not required to support circular chains of dependencies where every bean participating in the chain has a pseudo-scope.

Yes, that's the part of the specs I linked.

Which is precisely why this works with request scope in place, but not without it.

In my second example, with the request scope, it still doesn't work actually. I assume that is because the request scoped bean is not part of the chain (that is, it's a leaf in the tree graph).

manovotn commented 10 months ago

Alright, that explains it mostly.

I edited the comment for some more clarity.

Yes, that's the part of the specs I linked.

Yea, my bad, didn't realize it's the same bit you linked, sorry.

In my second example, with the request scope, it still doesn't work actually. I assume that is because the request scoped bean is not part of the chain (that is, it's a leaf in the tree graph).

Ah, I see. Well, if you make it part of the chain ,that should work. The trick there is that the normal-scoped bean will in fact result in injection of a client proxy and not the actual bean which will break the circularity :)

nlisker commented 10 months ago

Got it. And could this behavior have changed since earlier versions, or was it always like that? I'm asking because I did a large versions update, which included JavaEE 8 to Jakarta 10, and after the update I started getting these circular dependency issues. I thought it might have been related to the default scan mode if there isn't a beans.xml. Or it could be because I changed something else as well, although I'm pretty sure I had this setup in EE 8 also.

As part of the update I changed the version of Wildfly too, hence the version of Weld, so maybe that could do it?

manovotn commented 10 months ago

And could this behavior have changed since earlier versions, or was it always like that?

As far as CDI goes, this was always the same. Here's a link to CDI 2.0 spec text with the same wording - https://jakarta.ee/specifications/cdi/2.0/cdi-spec-2.0#injection_and_resolution I cannot say why you didn't see the previously; there could have been some change in Weld for sure but nothing in particular comes to mind.

nlisker commented 10 months ago

After digging into this more, I came up with a couple of findings.

I attached a sample project. It includes a couple of @Dependent circular reference beans and an @RequestScoped bean that is not part of the circular relation. It also includes a weld-junit test (Weld SE) and an Arquillian test (Weld EE). Note that the arquillian.xml file needs to point to an existing server destination (Wildfly 30 in my case).

zCDItest.zip (This is an Eclipse/Gradle project, but the sources are what's important.)

I found that Weld SE does eager injection, and will fail the test immediately during its startup phase, while Weld EE does lazy injection and will fail during runtime if it can't handle the circular dependencies. This could be a problem for those who're trying to test their code with weld-junit because it's more restrictive and does not reflect the real application. Run both tests to see SE failing and EE passing.

The second thing I found is that some circular references are tolerated (for non-normal scoped beans). It seems that Weld EE goes beyond the specs. The attached project contains such a setup. There, bean 1 calls bean 2 which calls bean 1 (the circularity is in the injections, not the method calls, which would have resulted in a SOE). Indeed, the instance of bean 1 that does the invocation of bean 2 and the instance of bean 1 that gets invoked by bean 2 are different. This is not unexpected since the container can choose which instance to use (and I think that they are pooled).

The questions are why and how does this work, and what is the "limit" of circularity that is tolerated. Weld EE will fail on circular dependencies for more convoluted setups. I suspect this is what I hit during my update - I changed something seemingly benign and hit some illegal setup (though you could say that I was lucky that what I had previously worked at all because it's out of the specs :) ).

manovotn commented 10 months ago

zCDItest.zip (This is an Eclipse/Gradle project, but the sources are what's important.)

Few things of note:

I found that Weld SE does eager injection

It most certainly doesn't; the injection is lazy in both cases. But the scenario you are describing should fail at deployment validation which is before any actual instantiation happens.

while Weld EE does lazy injection and will fail during runtime if it can't handle the circular dependencies

Here's a test directly from Weld testsuite that can run under EE container which fails at validation time - https://github.com/weld/core/blob/master/tests-arquillian/src/test/java/org/jboss/weld/tests/beanDeployment/circular/DependentCircularInjectionTest.java Now, you can easily modify this case to what you presented and I am still seeing a deployment exception in such a case. To be precise, the test expects the exception so it will succeed if an exception is thrown. Furthermore, I tried the same scenario in Weld SE and it behaves exactly the same :shrug:

In order to run that test:

nlisker commented 10 months ago

Thanks @manovotn. I tried to remove the EJB @Stateless annotation in my test and then I got the circularity exception in the EE test. With @Stateless EJBs, the circularity test passes with no exception. I understand that this is not in the pure-CDI realm because the container does more than just CDI (unlike in the SE case).

In this case my questions are:

  1. Where can I read about such "interactions" between CDI and EJB, seeing that the EJB annotations "circumvent" CDI's circular dependency exception?
  2. Is there a way to unit test while taking EJB annotations into account, or do I have to use a full container with Arquillian at this point (even if I don't actually need the other subsystems)?

I appreciate the instructive and quick replies.

manovotn commented 10 months ago

Thanks @manovotn. I tried to remove the EJB @Stateless annotation in my test and then I got the circularity exception in the EE test. With @Stateless EJBs, the circularity test passes with no exception. I understand that this is not in the pure-CDI realm because the container does more than just CDI (unlike in the SE case).

Hmm, that's not what I observed. Using the above linked test, you can add the stateless annotation and I still saw it pass (i.e. throw exception) Is there any specific Weld/WFLY setup you are using?

In this case my questions are:

  1. Where can I read about such "interactions" between CDI and EJB, seeing that the EJB annotations "circumvent" CDI's circular dependency exception?

Well, I suppose CDI spec in terms of EE integration bits, EJB spec (possibly, don't know that one up close) and then WFLY docs for its EJB subsytem. From my personal experience I can say that there are some grey areas for sure and each server does some of its own tricks to make these integrations work.

  1. Is there a way to unit test while taking EJB annotations into account, or do I have to use a full container with Arquillian at this point (even if I don't actually need the other subsystems)?

I don't think there is any other way - for tests which use EE integration bits, its just Arq.

nlisker commented 10 months ago

Hmm, that's not what I observed. Using the above linked test, you can add the stateless annotation and I still saw it pass (i.e. throw exception) Is there any specific Weld/WFLY setup you are using?

Here is all the info of my test:

@ExtendWith(ArquillianExtension.class)
public class WeldEETest {

    @Deployment
    public static WebArchive createDeployment() {
        return ShrinkWrap.create(EmbeddedGradleImporter.class)
                .forThisProjectDirectory()
                .importBuildOutput()
                .as(WebArchive.class);
    }

    @Test
    public void test() {
    }
}
@Stateless
@Dependent
public class Fish {

    @Inject
    Water water;
}
@Stateless
@Dependent
public class Water {

    @Inject
    Fish fish;
}

arquillian.xml (pointing to Wildfly 30.0.0.Final)

<arquillian
    xmlns="http://jboss.org/schema/arquillian"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xsi:schemaLocation="http://jboss.org/schema/arquillian http://jboss.org/schema/arquillian/arquillian_1_0.xsd">

    <engine>
        <property name="deploymentExportPath">build/deployments</property>
    </engine>

    <container qualifier="wildfly-managed" default="true">
        <configuration>
            <property name="jbossHome">C:\Dev\Servers\wildfly-30.0.0.Final</property>
            <property name="serverConfig">standalone-full.xml</property>
            <property name="allowConnectingToRunningServer">true</property>
        </configuration>
    </container>
</arquillian>

build.gradle (probably only important for the version numbers):

// CDITest //
plugins {
    id("war")
}

repositories {
    mavenCentral()
    maven {
        url "https://repo.gradle.org/gradle/libs-releases/"
    }
}

ext {
    gradleWrapperVersion = "8.5";
    javaVersion = "21";

    jakartaVersion = "10.0.0";

    junitVersion = "5.10.1";
    junitPlatformVersion = "1.10.1"

    weldJunitVersion = "4.0.1.Final";

    arquillianVersion = "1.8.0.Final";
    arquillianWildflyContainerVersion = "5.0.1.Final";
    shrinkwrapResolverVersion = "3.3.0";
}

dependencies {
    providedCompile("jakarta.platform:jakarta.jakartaee-api:$jakartaVersion")

    testImplementation("org.junit.jupiter:junit-jupiter:$junitVersion")
    testImplementation("org.junit.platform:junit-platform-launcher:$junitPlatformVersion")

    testImplementation("org.jboss.weld:weld-junit5:$weldJunitVersion")

    testImplementation("org.jboss.arquillian.junit5:arquillian-junit5-container:$arquillianVersion")
    testImplementation("org.jboss.arquillian.protocol:arquillian-protocol-servlet-jakarta:$arquillianVersion")
    testImplementation("org.wildfly.arquillian:wildfly-arquillian-container-managed:$arquillianWildflyContainerVersion")
    testImplementation("org.jboss.shrinkwrap.resolver:shrinkwrap-resolver-gradle-depchain:$shrinkwrapResolverVersion")

}

java {
    toolchain.languageVersion.set(JavaLanguageVersion.of(javaVersion))
}

tasks.named("wrapper") {
    gradleVersion = gradleWrapperVersion
}

No beans.xml (shouldn't make a difference since the default scanning mode is annotated).

Running the test passes (doesn't throw an exception) in this setup. I will try to match the versions used in the test you linked and see if something changes along the way.

From my personal experience I can say that there are some grey areas for sure and each server does some of its own tricks to make these integrations work.

Then perhaps the smarter thing for me to do is not rely on server-specific tricks if I can.

nlisker commented 10 months ago

If I change the code to use constructor injections, just like in the linked test:

@Stateless
@Dependent
public class Water {

    @Inject
    public Water(Fish fish) {
    }
}
@Stateless
@Dependent
public class Fish {

    private Water water;

    @Inject
    public Fish(Water water) {
        this.water = water;
    }
}

Then I get the circular dependencies exception without @Stateless, but with @Stateless I get a different exception:

org.jboss.arquillian.container.spi.client.container.DeploymentException: Cannot deploy 5b73d625-6616-4184-a08f-fec1e672fa20.war: {"WFLYCTL0062: Composite operation failed and was rolled back. Steps that failed:" => {"Operation step-1" => {"WFLYCTL0080: Failed services" => {"jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".INSTALL" => "WFLYSRV0153: Failed to process phase INSTALL of deployment \"5b73d625-6616-4184-a08f-fec1e672fa20.war\"
    Caused by: org.jboss.as.server.deployment.DeploymentUnitProcessingException: WFLYEE0024: Could not configure component Water
    Caused by: org.jboss.as.server.deployment.DeploymentUnitProcessingException: WFLYEJB0127: Jakarta Enterprise Beans Water of type zCDItest.Water must have public default constructor"},"WFLYCTL0412: Required services that are not installed:" => ["jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".WeldStartService","jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".beanmanager"],"WFLYCTL0180: Services with missing/unavailable dependencies" => ["service jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".weld.weldClassIntrospector is missing [jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".WeldStartService, jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".beanmanager]","service jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".batch.artifact.factory is missing [jboss.deployment.unit.\"5b73d625-6616-4184-a08f-fec1e672fa20.war\".beanmanager]"]}}}
    at org.jboss.as.arquillian.container.ArchiveDeployer.deployInternal(ArchiveDeployer.java:185)
    at org.jboss.as.arquillian.container.ArchiveDeployer.deployInternal(ArchiveDeployer.java:163)
    at org.jboss.as.arquillian.container.ArchiveDeployer.deploy(ArchiveDeployer.java:91)
    at org.jboss.as.arquillian.container.CommonDeployableContainer.deploy(CommonDeployableContainer.java:262)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$3.call(ContainerDeployController.java:151)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$3.call(ContainerDeployController.java:118)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.executeOperation(ContainerDeployController.java:239)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.deploy(ContainerDeployController.java:118)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
    at org.jboss.arquillian.container.impl.client.container.DeploymentExceptionHandler.verifyExpectedExceptionDuringDeploy(DeploymentExceptionHandler.java:47)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.container.impl.client.ContainerDeploymentContextHandler.createDeploymentContext(ContainerDeploymentContextHandler.java:71)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.container.impl.client.ContainerDeploymentContextHandler.createContainerContext(ContainerDeploymentContextHandler.java:54)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
    at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$1.perform(ContainerDeployController.java:92)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController$1.perform(ContainerDeployController.java:77)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.forEachDeployment(ContainerDeployController.java:232)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.forEachManagedDeployment(ContainerDeployController.java:212)
    at org.jboss.arquillian.container.impl.client.container.ContainerDeployController.deployManaged(ContainerDeployController.java:77)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
    at org.jboss.arquillian.core.impl.EventImpl.fire(EventImpl.java:62)
    at org.jboss.arquillian.container.test.impl.client.ContainerEventController.execute(ContainerEventController.java:96)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.invokeObservers(EventContextImpl.java:103)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:90)
    at org.jboss.arquillian.test.impl.TestContextHandler.createClassContext(TestContextHandler.java:83)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.test.impl.TestContextHandler.createSuiteContext(TestContextHandler.java:69)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.jboss.arquillian.core.impl.ObserverImpl.invoke(ObserverImpl.java:86)
    at org.jboss.arquillian.core.impl.EventContextImpl.proceed(EventContextImpl.java:95)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:133)
    at org.jboss.arquillian.core.impl.ManagerImpl.fire(ManagerImpl.java:105)
    at org.jboss.arquillian.test.impl.EventTestRunnerAdaptor.beforeClass(EventTestRunnerAdaptor.java:89)
    at org.jboss.arquillian.junit5.ArquillianExtension.beforeAll(ArquillianExtension.java:35)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

"Water must have public default constructor" comes from EJB, so it's a different exception now. Still, the case I'm looking at is with field injection, not constructor injection.

manovotn commented 10 months ago

"Water must have public default constructor" comes from EJB, so it's a different exception now.

Yea, according to EJB spec, there should be a public no-arg constructor as well. That makes the bean invalid from both, EJB and CDI perspective and based on which validation kicks in first, you might see one or the other exception.

Still, the case I'm looking at is with field injection, not constructor injection.

Right, that shouldn't make a difference - circularity should be detected in both cases. And changing the Weld test to field injection still gives me the same valid output.

No beans.xml (shouldn't make a difference since the default scanning mode is annotated).

Also correct; I have tried modifying the Weld test to not include beans.xml and that worked just fine.

manovotn commented 10 months ago

WFLYEJB0127: Jakarta Enterprise Beans Water of type zCDItest.Water must have public default constructor"

Ah, I think I can finally see the same state you do. The Weld test I used didn't have public classes which then led to those classes not being recognized as EJB beans (which apparently doesn't throw an error but does log a warning in server logs). With that out of the way I get the constructor complaint from EJB, which is also valid and can be easily remedied. So, fixing that (whether you use field or c-tor injection), I can finally see the test deploying without CDI complaining at all. Here's what I changed in that test - https://github.com/weld/core/compare/master...manovotn:core:statelessValidationReproducer?expand=1

manovotn commented 10 months ago

@nlisker alright, looking at a little bit of a debug, for a stateless bean, Weld will create an 'enterprise proxy', so if you inspect any injection point, you will see that you are in fact injecting something like:

class org.jboss.weld.tests.beanDeployment.circular.Fish$Proxy$_$$_Weld$EnterpriseProxy$

I don't know the EJB code enough to tell you exactly why is this required but my guess is on the beans being pooled. Anyway, as I side effect, you basically side-step the limitation of CDI non-normal scoped beans and can resolve a circular dependency. Just like when you use circ. dep. between two, say, application scoped beans (as those will use a proxy as well, albeit client proxy in that case).

Note that this is also allowed by CDI spec as it neither enforces nor forbids this to work:

The container is not required to support circular chains of dependencies where every bean participating in the chain has a pseudo-scope.

TL;DR:

nlisker commented 10 months ago

(Edited the comment because you posted the answers a few seconds before I posted the questions)

Alright, so there is indeed a hidden interaction at work here between EJB and CDI. I agree that the CDI specs are satisfied ("not required" doesn't mean "not allowed").

If I make the test more complicated (like in my real code), I can create a circular dependency failure even for @Dependent @Stateless beans, but it isn't the responsibility of CDI anyway (and probably EJB either). I find it interesting nonetheless to find out what trips things up. :)

manovotn commented 10 months ago

Alright, so there is indeed a hidden interaction at work here between EJB and CDI.

And the exact same setup may or may not work on other EE server.

If I make the test more complicated (like in my real code), I can create a circular dependency failure even for @Dependent @Stateless beans, but it isn't the responsibility of CDI anyway (and probably EJB either). I find it interesting nonetheless to find out what trips things up. :)

Yes, I can imagine there are some more complex cases where it won't work. If you want to stick to what is supported by specs and portable, don't create circular dep between non-normal scoped beans. I know, oftentimes easier said than done :)

I'll close this issue as I believe we found out what's happening. If you think otherwise, feel free to reopen.

nlisker commented 10 months ago

Yes, this is fine, thanks again for the help!