Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ArgumentArraySizeWithLookupTest (and related) failing against Weld 6.0.0 in GF #593

Closed
arjantijms opened this issue Jun 17, 2024 · 10 comments

Comments

@arjantijms
Copy link

GlassFish 8 uses Weld 6.0.0.Beta1, and for some reason that's hard to understand several TCKs tests related to Lookups are failing.

These concern for instance ArgumentArraySizeWithLookupTest where it fails with:

org.jboss.weld.exceptions.DeploymentException: 
   WELD-001488: 
   Argument lookup configured for target bean class 
      org.jboss.cdi.tck.tests.invokers.lookup.ArgumentArraySizeWithLookupTest$MyService 
   and parameter 1 of target method 
      public java.lang.String org.jboss.cdi.tck.tests.invokers.lookup.ArgumentArraySizeWithLookupTest$MyService.hello(
          org.jboss.cdi.tck.tests.invokers.lookup.ArgumentArraySizeWithLookupTest$MyDependency1,
          org.jboss.cdi.tck.tests.invokers.lookup.ArgumentArraySizeWithLookupTest$MyDependency2) 
   is unsatisfied

After investigation it appears the Invoker lands in a bean archive associated with the extension that creates it, while the beans related to the arguments MyDependency1 and MyDependency2 land in a bean archive associated with the deployed application.

Bean invoker:

Screenshot 2024-06-17 at 14 59 40

Beans for arguments:

Screenshot 2024-06-17 at 15 13 56 Screenshot 2024-06-17 at 15 13 11

When "[beanDeploymentArchiveId=org-jboss.weld.bootstrap.WeldExtension]" is validated, it validates the Invoker. This fails since the Invoker needs MyDependency1, which is in "[beanDeploymentArchiveId=ArgumentArraySizeWithLookupTest731093ffe4ebf3f9125ba36f0e8234a887e35]".

@manovotn @Ladicek how is this supposed to work? Why does Weld passed the TCK, but still fails on GF?

@manovotn
Copy link
Contributor

Why does Weld passed the TCK, but still fails on GF?

I suppose it's differences in how WFLY and GF handle bean archives.

After investigation it appears the Invoker lands in a bean archive associated with the extension that creates it, while the beans related to the arguments MyDependency1 and MyDependency2 land in a bean archive associated with the deployed application.

That sounds like GF places the extension into a separate bean archive, why?
The test has a single WAR so everything should be in one.
I need to check on WFLY tomorrow once I am in front of my laptop but I don't think this happens there.

@arjantijms
Copy link
Author

I need to check on WFLY tomorrow once I am in front of my laptop but I don't think this happens there.

I'll check WildFly too to be sure then. org.jboss.weld.bootstrap.WeldStartup.getBeanDeployments() returns many as shown in the picture. Most of them associated with the different extenions that exists (such as the Faces/Mojarra one, etc).

@manovotn
Copy link
Contributor

@arjantijms so in WFLY, I can see that the invoker is registered in a different bean archive as well, namely

  • Weld BeanManager for org.jboss.weld.core.additionalClasses [bean count=...][1]

While BM of the test deployment archive is something like this:

  • Weld BeanManager for ArgumentArraySizeWithLookupTest731093ffe4ebf3f9125ba36f0e8234a887e35.war/ [bean count=...][2]

But I guess the difference in GF is the accessibility graph between deployments - in WFLY the first BM[1] always has the latter BM[2] under its list of accessible managers and can therefore "see" the dependencies and resolve them.

@arjantijms
Copy link
Author

@manovotn that's interesting.

I did a little tracing how the Invoker ends up in a different bean archive, and it's via:

org.jboss.weld.bootstrap.events.AfterBeanDiscoveryImpl.processBeanRegistration(BeanRegistration, GlobalEnablementBuilder)

There, the BeanRegistration already has an initBeanManager, which corresponds to something from the Arquillian test. I marked the runtime details with ###:

    private <T> void processBeanRegistration(BeanRegistration registration, GlobalEnablementBuilder globalEnablementBuilder) {
        Bean<?> bean = registration.initBean();
        BeanManagerImpl beanManager = registration.initBeanManager();

        ###  beanManager = Weld BeanManager for org.jboss.arquillian.testenricher.cdi.container.CDIExtension [bean count=32]

        if (beanManager == null) {
            // Get the bean manager for beans added via ABD#addBean()
            beanManager = getOrCreateBeanDeployment(bean.getBeanClass()).getBeanManager();
        } else {
            // Also validate the bean produced by a builder
              
            ### getBeanManager() = Weld BeanManager for deployment [bean count=14]

            ExternalBeanAttributesFactory.validateBeanAttributes(bean, getBeanManager());
            validateBean(bean);
        }
	   
        // code ommitted
        
        } else {
            ### beanManager = Weld BeanManager for org.jboss.arquillian.testenricher.cdi.container.CDIExtension [bean count=32]
            beanManager.addBean(bean);
            
            if (priority != null && bean.isAlternative()) {
                globalEnablementBuilder.addAlternative(bean.getBeanClass(), priority);
            }
        }
        containerLifecycleEvents.fireProcessBean(beanManager, bean, registration.extension);
    }

I've not yet found how "[beanDeploymentArchiveId=org-jboss.weld.bootstrap.WeldExtension]" is created from "Weld BeanManager for org.jboss.arquillian.testenricher.cdi.container.CDIExtension".

If there is no initBeanManager, as most regular beans added by other extensions (jersey, mojarra, etc) have, then the bean archive is found via

beanManager = getOrCreateBeanDeployment(bean.getBeanClass()).getBeanManager()

Which is typically (always?) the bean archive corresponding to the jar file the extension physically resides in.

@arjantijms
Copy link
Author

This one seems related too: weld/core#2909

In GlassFish, the LiteExtensionTranslator, which is also into play here, is added manually:

    @Override
    public Iterable<Metadata<Extension>> getExtensions() {
        if (extensions != null) {
            return extensions;
        }

        List<Metadata<Extension>> extensionsList = new ArrayList<>();
        // Register org.jboss.weld.lite.extension.translator.LiteExtensionTranslator in order to be able to execute build compatible extensions
        // Note that we only register this if we discovered at least one implementation of BuildCompatibleExtension

        List<Class<? extends BuildCompatibleExtension>> buildExtensions = getBuildCompatibleExtensions();

        if (!buildExtensions.isEmpty()) {
            try {
                extensionsList.add(new MetadataImpl<>(new LiteExtensionTranslator(buildExtensions, Thread.currentThread().getContextClassLoader())));
            } catch (Exception e) {
                throw new RuntimeException(e);
            }
        }

        for (BeanDeploymentArchive beanDeploymentArchive : getBeanDeploymentArchives()) {
            if (!(beanDeploymentArchive instanceof RootBeanDeploymentArchive)) {
                ClassLoader classLoader = new FilteringClassLoader(((BeanDeploymentArchiveImpl) beanDeploymentArchive)
                    .getModuleClassLoaderForBDA());
                extensions = context.getTransientAppMetaData(WELD_BOOTSTRAP, WeldBootstrap.class)
                    .loadExtensions(classLoader);

                if (extensions != null) {
                    for (Metadata<Extension> beanDeploymentArchiveExtension : extensions) {
                        extensionsList.add(beanDeploymentArchiveExtension);
                    }
                }
            }
        }

        return extensionsList;
    }

I do wonder where and how to setup "org.jboss.weld.core.additionalClasses".

@manovotn
Copy link
Contributor

This one seems related too: weld/core#2909

No, this is purely for Weld SE where there is always a synthetic bean archive. That's not the case for EE.

However, you are correct that LiteExtensionTranslator plays a role - the tests you mention are all using just BCE which are all executed via LiteExtensionTranslator portable extension. Here's how I registered it in WFLY.

I do wonder where and how to setup "org.jboss.weld.core.additionalClasses".

IIRC, that's a helper archive created in WFLY for classes coming from a non-bean archive and needing one to reside in.
See WeldDeployment class in WFLY.
I cannot tell you from the top of my head when exactly WFLY leverages this, I'd need to do some debugging.

I did a little tracing how the Invoker ends up in a different bean archive, and it's via:
org.jboss.weld.bootstrap.events.AfterBeanDiscoveryImpl.processBeanRegistration(BeanRegistration, GlobalEnablementBuilder)

Not sure I follow.
What's the extension inside BeanRegistration? Is is the LiteExtensionTranslator? If so, you are registering it under some Arq. archive which seems awkward.

@arjantijms
Copy link
Author

arjantijms commented Jun 18, 2024

Not sure I follow.
What's the extension inside BeanRegistration? Is is the LiteExtensionTranslator? If so, you are registering it under some Arq. archive which seems awkward.

It's hard to follow indeed. The LiteExtensionTranslator and its bean manager is used all the time, e.g. inside the InvokerFactory that the TCK extension gets to see, and in the synthesis method called from org.jboss.weld.lite.extension.translator.LiteExtensionTranslator.synthesis(AfterBeanDiscovery)

The configurator gets the Arquillian extension bean manager here:

 for (SyntheticBeanBuilderImpl<?> syntheticBean : syntheticBeans) {
            jakarta.enterprise.inject.spi.configurator.BeanConfigurator<Object> configurator;
            if (abd instanceof AfterBeanDiscoveryImpl) {
                // specify the receiver class to be the BCE extension, linking the bean to it
                // in EE env this affects the BM used for dynamic resolution inside bean creation method

                ### extensionClass = class org.jboss.cdi.tck.tests.invokers.lookup.ArgumentArraySizeWithLookupTest$TestExtension

                configurator = ((AfterBeanDiscoveryImpl) abd).addBean(syntheticBean.extensionClass);

                ### configurator.beanManager = the arquillean CDIExtension one

@arjantijms
Copy link
Author

So it seems the problem is most likely here:

Weld calls beanDeploymentFinder in AfterBeanDiscoveryImpl.addBean:

### beanClass = org.jboss.cdi.tck.tests.invokers.lookup.ArgumentArraySizeWithLookupTest$TestExtension

beanDeploymentFinder.getOrCreateBeanDeployment(beanClass).getBeanManager();

Which at some point delegates to a CDI11Deployment implementation, which happens to be org.glassfish.weld.DeploymentImpl and calls:

org.glassfish.weld.DeploymentImpl.loadBeanDeploymentArchive(Class<?>)

@Override
    public BeanDeploymentArchive loadBeanDeploymentArchive(Class<?> beanClass) {
        if (LOG.isLoggable(FINE)) {
            LOG.log(FINE, LOAD_BEAN_DEPLOYMENT_ARCHIVE, new Object[] { beanClass });
        }

        // Check if we have already created a bean archive for this bean class, and if so return it.

        for (BeanDeploymentArchive beanDeploymentArchive : beanDeploymentArchives) {
            if (((BeanDeploymentArchiveImpl) beanDeploymentArchive).getModuleBeanClasses().contains(beanClass.getName())) {

                // Don't stuff this Bean Class into the BeanDeploymentArchive's beanClasses,
                // as Weld automatically add theses classes to the BeanDeploymentArchive's bean Classes
                return beanDeploymentArchive;
            }

             // ... code ommitted
        }

        ClassLoader classLoaderKey = beanClass.getClassLoader();

        ### classLoaderKey = org.glassfish.web.loader.WebappClassLoader
   
        BeanDeploymentArchive extensionBeanDeploymentArchive = extensionBDAMap.get(classLoaderKey);
        if (extensionBeanDeploymentArchive != null) {
            return extensionBeanDeploymentArchive;
        }

        // If the beanDeploymentArchive was not found for the Class, create one and add it
}

And then org.jboss.arquillian.testenricher.cdi.container.CDIExtension happens to have the same class loader, since it's also an extension embedded in the war (but via a jar).

Hmmmm....

@arjantijms
Copy link
Author

I was able to pass this tests (and related failures) by giving the archives access to each other as follows:

        // Add the new archive to all existing archives
        for (BeanDeploymentArchive beanDeploymentArchive : beanDeploymentArchives) {
            beanDeploymentArchive.getBeanDeploymentArchives().add(newBeanDeploymentArchive);
        }

        // Add the existing archives archives to the new archive
        newBeanDeploymentArchive.getBeanDeploymentArchives().addAll(beanDeploymentArchives);

Outside the scope of this issue, but I wonder why the server has to do so many things to integrate, and why we can't move these things to Weld and the EE APIs.

@manovotn
Copy link
Contributor

I was able to pass this tests (and related failures) by giving the archives access to each other as follows:

Yes, that's pretty much what I tried to explain earlier in #593 (comment) :)

Outside the scope of this issue, but I wonder why the server has to do so many things to integrate, and why we can't move these things to Weld and the EE APIs.

I'd say because every EE server does it their own way. For instance the CL structure in WFLY has always been slightly different due to usage of JBoss Modules. I am not sure this could be unified as that spans further than just CDI; other EE technologies or custom server extensions might need a way to integrate themselves with CDI implementation in one way or another.
Besides, I think not every impl behaves like Weld + WFLY does and their accessibility graphs might be built differently or perhaps not exist at all.
For the record, here's a link to how WFLY calculates the graph and here's their extended BeanDeploymentArchive abstraction.
If you think some of those APIs would be usable for you as well, we could move them to Weld API.

Since this is now resolved, I will close this issue. Feel free to reopen if you think there is more to be done.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants