[alesj]

Now, the main question that I have is how to create the Archives for the jars in the lib dir? And how to have those jars scanned for XML files?

We could do it per invocation.

e.g. (pseudo code)

Class<?> beanClass = ...; // parameter from Deployment::loadBeanDeploymentArchive
BDA bda = getBDA(beanClass);
if (bda == null)
{
   URL jarURL = getJarURL(beanClass);
   URL beansXML = getBeansXML(jarURL);
   if (beansXML == null)
      return null;

   // TODO ... some sort of simple embedded Weld scanning mechanism
}

Since I guess we don't to do much for those jars; e.g. they should not have JEE components?

[flavia.rainone]

Ales Justin wrote:

Now, the main question that I have is how to create the Archives for the jars in the lib dir? And how to have those jars scanned for XML files?

We could do it per invocation.

e.g. (pseudo code)

Class<?> beanClass = ...; // parameter from Deployment::loadBeanDeploymentArchive
BDA bda = getBDA(beanClass);
if (bda == null)
{
   URL jarURL = getJarURL(beanClass);
   URL beansXML = getBeansXML(jarURL);
   if (beansXML == null)
      return null;
 
   // TODO ... some sort of simple embedded Weld scanning mechanism
}

I thought we should provide a Deployment/BDA structure for Weld on AS startup, for all jars in the lib that contain the beans.xml file? Just like we do for all archives that are deployed.

And, then, in a latter moment, if Weld requests for a class that lies in a lib jar that lacks the bean.xml, yes, we should look for the lib jar and create the BDA accordingly?

Ales Justin wrote:

Since I guess we don't to do much for those jars; e.g. they should not have JEE components?

+1

[pmuir]

Flavia Rainone wrote:

I thought we should provide a Deployment/BDA structure for Weld on AS startup, for all jars in the lib that contain the beans.xml file? Just like we do for all archives that are deployed.

And, then, in a latter moment, if Weld requests for a class that lies in a lib jar that lacks the bean.xml, yes, we should look for the lib jar and create the BDA accordingl?

Yes, I think Flavia's interpretation is correct.

[alesj]

Why the rush? :-)

If you ask me, I would do it lazily aka on_demand.

Scanning the whole common/lib is gonna be slow,

and imo, people should not be using libs to deploy Weld components -- simply put them in deploy/.

Or why would you do it upfront?

[flavia.rainone]

Ales Justin wrote:

Why the rush? :-)

If you ask me, I would do it lazily aka on_demand.

Scanning the whole common/lib is gonna be slow,

and imo, people should not be using libs to deploy Weld components -- simply put them in deploy/.

Or why would you do it upfront?

I've been thinking about correctness all this time. If somebody places a jar in lib and expects that the beans are created upfront when AS starts, then this would not work using your approach.

Thinking about performance, though, I think that we should trade that by better performance, i.e., we should use the suggested lazy approach.

And I don't see any useful scenario where somebody would want to have a bean deployed to lib dir loaded at startup, specially if no other bean deployed in deploy dir has a dependency towards it.

Pete, does the lazy approach fullfills the Weld prerequisites regarding lib jars?

[pmuir]

Flavia Rainone wrote:

Ales Justin wrote:

Why the rush? :-)

If you ask me, I would do it lazily aka on_demand.

Scanning the whole common/lib is gonna be slow,

and imo, people should not be using libs to deploy Weld components -- simply put them in deploy/.

Or why would you do it upfront?

Because the Java EE/CDI specs require it.

I've been thinking about correctness all this time. If somebody places a jar in lib and expects that the beans are created upfront when AS starts, then this would not work using your approach.

Ok, I think we've got crossed wires. You can't deploy a library jar in some "context" outside a deployment. The beans don't get created or anything outside the context of a deployment.

Another way to think of it is that a library in default/lib with a beans.xml in it works just like a library in WEB-INF/lib.

[flavia.rainone]

Pete Muir wrote:

I've been thinking about correctness all this time. If somebody places a jar in lib and expects that the beans are created upfront when AS starts, then this would not work using your approach.

Ok, I think we've got crossed wires. You can't deploy a library jar in some "context" outside a deployment. The beans don't get created or anything outside the context of a deployment.

Another way to think of it is that a library in default/lib with a beans.xml in it works just like a library in WEB-INF/lib.

In that case, why the lazy approach wouldn't work?

From what I can see, if it works just like a library in WEB-INF/lib, it means that we will need to provide a BDA for a lib jar only when Weld requests for one through loadBeanDeploymentArchive, right?

Or, if I'm wrong, do you mean that a BDA for every lib jar with a META-INF/beans.xml should be provided as direct or indirect result of BeanDeploymentArchive.getBeanDeploymentArchives() for every BDA in a Deployment?

Explaining better, given commos/lib/mylib.jar with the following structure:

mylib.jar

  |_

      META-INF/beans.xml

  |_

     whatever classes in here

Say we create a BDA upfront, let's call it MY_LIB_BDA, and add this BDA to DefalutDomain Classpath. No deployment is created upfront.

Now, say my-ejb.jar with a META-INF/beans.xml file is deployed to deploy dir. We create a Deployment for it, of course, and we create a BDA for it as well. This BDA, called MY_EJB_BDA, belongs to Default Domain Classpath. So, when Weld invokes MY_EJB_BDA.getBDAs(), it should , directly on indirectly (i.e., by walking on the BDA graph structure), reach MY_LIB_BDA?

This is different from the lazy approach because, with this second approach, you are not required to call loadBeanDeploymentArchive in order to obtain MY_LIB_BDA.  A simple BeanDeploymentArchive.getBeanDeploymentArchives will get you there.

Is that what Weld requires us to do?

Pete Muir wrote:

Flavia Rainone wrote:

Ales Justin wrote:

Why the rush? :-)

If you ask me, I would do it lazily aka on_demand.

Scanning the whole common/lib is gonna be slow,

and imo, people should not be using libs to deploy Weld components -- simply put them in deploy/.

Or why would you do it upfront?

Because the Java EE/CDI specs require it.

BTW, I've read the spec sometime ago. Of course I don't remember all the details so, if you think rereading some specific part of it might be useful, let me know.

Message was edited by: Flavia Rainone

[pmuir]

Flavia Rainone wrote:

Pete Muir wrote:

I've been thinking about correctness all this time. If somebody places a jar in lib and expects that the beans are created upfront when AS starts, then this would not work using your approach.

Ok, I think we've got crossed wires. You can't deploy a library jar in some "context" outside a deployment. The beans don't get created or anything outside the context of a deployment.

Another way to think of it is that a library in default/lib with a beans.xml in it works just like a library in WEB-INF/lib.

In that case, why the lazy approach wouldn't work?

From what I can see, if it works just like a library in WEB-INF/lib, it means that we will need to provide a BDA for a lib jar only when Weld requests for one through loadBeanDeploymentArchive, right?

Or, if I'm wrong, do you mean that a BDA for every lib jar with a META-INF/beans.xml should be provided as direct or indirect result of BeanDeploymentArchive.getBeanDeploymentArchives() for every BDA in a Deployment?

Explaining better, given commos/lib/mylib.jar with the following structure:

mylib.jar

  |_

      META-INF/beans.xml

  |_

     whatever classes in here

Say we create a BDA upfront, let's call it MY_LIB_BDA, and add this BDA to DefalutDomain Classpath. No deployment is created upfront.

Now, say my-ejb.jar with a META-INF/beans.xml file is deployed to deploy dir. We create a Deployment for it, of course, and we create a BDA for it as well. This BDA, called MY_EJB_BDA, belongs to Default Domain Classpath. So, when Weld invokes MY_EJB_BDA.getBDAs(), it should , directly on indirectly (i.e., by walking on the BDA graph structure), reach MY_LIB_BDA?

This is different from the lazy approach because, with this second approach, you are not required to call loadBeanDeploymentArchive in order to obtain MY_LIB_BDA.  A simple BeanDeploymentArchive.getBeanDeploymentArchives will get you there.

Is that what Weld requires us to do?

Yes. If there is an accessible (i.e. classes can be loaded from it) library jar with META-INF/beans.xml, then Weld expects you to return it as part of the BDA graph of the Deployment.

The loadBeanDeploymentArchive method solely exists for the case that someone adds a bean based on a class which isn't present in the BDA graph via a CDI lifecycle listener. For every bean defined in such a way, Weld calls loadBeanDeploymentArchive, expecting the correct BDA to be returned.

IOW this is a kinda a two stage process - firstly the BDAs which are defined declaratively (META-INF/beans.xml) are handed to Weld, and then BDAs which are defined programatically are loaded by Weld (via loadBeanDeploymentArchive).

Because the Java EE/CDI specs require it.

BTW, I've read the spec sometime ago. Of course I don't remember all the details so, if you think rereading some specific part of it might be useful, let me know.

I simply meant that the reason we have to load library jars as BDAs is because the spec says so ;-)

[flavia.rainone]

Flavia Rainone wrote:

Finally, the tests are very poor. I just ported the already existing ones to test the new deployers. So writing real tests is what I'll be doing next, and I'm sure I have quite a few bugs to catch ;-)

I stumbled upon some issue here.

The problem is located in WeldFilesDeployer:

         Iterable<VirtualFile> classpaths = getClassPaths(unit);
         for (VirtualFile cp : classpaths)
         {
            VirtualFile wbXml = cp.getChild("META-INF/beans.xml");
            if (wbXml.exists())
            {
               // add url
               wbFiles.add(wbXml);
               // add classes
               cpFiles.add(cp);

            }
         }

This deployer deploys the cpFiles of the current unit only if it has the META-INF/beans.xml file.

The problem with this is that this results in an ArchiveInfo without any classes in it if we deploy an ejb-jar without META-INF/beans.xml. In the future, if Weld request for a BDA associated with the ejb-jar by calling Deployment.loadBeanDeploymentArchive, it will get a BDA that has no classes in it.

So, I tried to "fix" it:

         Iterable<VirtualFile> classpaths = getClassPaths(unit);
         for (VirtualFile cp : classpaths)
         {
            VirtualFile wbXml = cp.getChild("META-INF/beans.xml");
            if (wbXml.exists())
            {
               // add url
               wbFiles.add(wbXml);
            }

            // add classes
            cpFiles.add(cp);

         }

This resulted in a series of test failures. All related with the criteria that we use to decide whether to put a class in a BDA.

Overall, these are the scenarios that are broken:

- if one or more lib jars in an ear happen to not have a META-INF/beans.xml file, the lib classes were not part of the BDA that represents the ear (note that currently I'm creating a BDA for every classLoader. So, for an ear, there is a single BDA that contains all the info regarding lib jars and ejb-jars in it).

- if one ore more ejb-jars in an ear happen to not have a META-INF/beans.xml file, the classes of those non-CDI ejb-jars were not part of the resulting BDA

- if in a war that contains a WEB-INF/beans.xml we have one ore more lib jars that don't have a META-INF/beans.xml, the classes of those libs are not part of the resulting BDA (we currently create a single BDA for every war; this BDA represents both the classes and the libs of the war)

- if, in a war that lacks a WEB-INF/beans.xml, we have one ore more lib jars that have the META-INF/beans.xml, the classes of the war (WEB-INF/classes) are not part of the resulting BDA.

In all the scenarios I described, the classes that were not part of the resulting BDA became part of the BDA with my "fix".

Notice that, interestingly, in the way things are implemented today, if you pass one of the missing classes as a parameter to a Deployment.loadBeanDeploymentArchive, my implementation will return the requested BDA correctly, but still the BDA wil lack the requested class!

So, we clearly have made some mistaken assumption here:

- either the classes that were not part of the BDAs in the previously described scenarios were supposed to be in the BDA and I fixed the bug. Now, all I have left to do is fixing the tests as well.

- or we should not create a BDA for every ClassLoader. We should be creating a BDA for every classpath entry (i.e., there should be a BDA for the WEB-INF/classes war; and another BDA for every lib in the war; simillarly, there should be a BDA for every distinct jar in the ear archive). As a result, we have one ore more BDAs that can see each other simultaneously because they all happen to belong to the same ClassLoader. Also, in a single AS instace, we will have a larger number of BDAs when compared to my current implementation.

Which way should I go now?

[alesj]

Yes. If there is an accessible (i.e. classes can be loaded from it) library jar with META-INF/beans.xml, then Weld expects you to return it as part of the BDA graph of the Deployment.

OK, this then rules out my lazy approach. :-)

But I would still do this on_demand, when (but just before) the first CDI/Weld deployment is deployed.

That would be the default behavior, but it could be made optional to do this at boot time instead.

The loadBeanDeploymentArchive method solely exists for the case that someone adds a bean based on a class which isn't present in the BDA graph via a CDI lifecycle listener. For every bean defined in such a way, Weld calls loadBeanDeploymentArchive, expecting the correct BDA to be returned.

IOW this is a kinda a two stage process - firstly the BDAs which are defined declaratively (META-INF/beans.xml) are handed to Weld, and then BDAs which are defined programatically are loaded by Weld (via loadBeanDeploymentArchive).

So, if I understand this correctly, BDAs created via "loadBeanDeploymentArchive" don't require beans.xml?

Simply add the class' owner jar as a BDA archive?

[flavia.rainone]

Ales Justin wrote:

The loadBeanDeploymentArchive method solely exists for the case that someone adds a bean based on a class which isn't present in the BDA graph via a CDI lifecycle listener. For every bean defined in such a way, Weld calls loadBeanDeploymentArchive, expecting the correct BDA to be returned.

IOW this is a kinda a two stage process - firstly the BDAs which are defined declaratively (META-INF/beans.xml) are handed to Weld, and then BDAs which are defined programatically are loaded by Weld (via loadBeanDeploymentArchive).

So, if I understand this correctly, BDAs created via "loadBeanDeploymentArchive" don't require beans.xml?

Simply add the class' owner jar as a BDA archive?

I have just chatted with Pete, and this is what he has to say about it:

(11:43:25) Pete: so it looks like from what you say that the BDA you are returning from loadBDA is pre-populated with everything in that archive?
(11:44:00) Flavia: Yes, it has always been like this. The only difference is that a few classes were missing.
(11:45:59) Pete: hmm, I think the intention is to only have the classes in that BDA that have been explicitly requested via loadBDA
(11:46:07) Flavia: Aha!
(11:46:16) Flavia: That is a missing piece of the puzzle then
(11:46:18) Pete: (of course, if it's an existing BDA, then it should have the regular classes)
(11:47:14) Flavia: As I said in the forum, mapping a BDA to a ClassLoader
(11:47:26) Flavia: puts several jars in the same umbrella.
(11:47:54) Flavia: So, currently, the BDA "umbrella" for several jars contain only the classes of the jars that have the beans.xml file. Is this correct?
(11:48:02) Flavia: From the Weld point of view
(11:48:29) Pete: i think that sounds fine to me
(11:48:52) Flavia: Ok, so in that case, I think that Ales's suggestion is the way to go
(11:48:56) Flavia: Ales suggested a third option
(11:49:25) Flavia: According to Ales, what is there is correct the way it is, which is also what you are saying
(11:49:45) Flavia: The only thing here is that, if a loadBeanDeploymentArchive is invoked with a class that is not in the BDA before
(11:50:10) Flavia: considering the BDA already exists but one of the jars it represents lacks the META-INF/beans.xml file
(11:50:29) Flavia: in that case, the class should be added to the already existing BDA before it is returned
(11:51:25) Pete: i think in our case
(11:51:29) Pete: it depends on the classloader
(11:51:37) Pete: /Module
(11:51:52) Pete: if it's in a Module which already has an associated BDA, then add it to that BDA and return that BDA
(11:52:05) Pete: if it's in a Module with no BDA, create a new BDA with just that class in
(11:52:25) Flavia: Cool, that's easy to do
(11:52:59) Flavia: So, I should always add the class, and never start scanning the jar it belongs to in order to add all the classes to the BDA?
(11:53:31) Pete: for loadBDA, no, never
(11:53:42) Flavia: Aha! Now I think I get it
...
(11:55:05) Pete: basically, you can think of loadBDA as a special way of adding classes to BDA which only adds those classes
(11:55:24) Flavia: Yes... Ales and I were talking about this today on the call
(11:55:39) Flavia: His question is when, in which scenario, loadBDA is called for a class that is not in the BDA?
(11:55:44) Pete: ah
(11:55:53) Pete: well, if you look at CDI lifecycle events
(11:56:05) Pete: you can see that people can create a Bean<> for any class that is on the classpath
(11:56:14) Pete: so to properly enforce the accessibility rules
(11:56:21) Pete: we need to know where it is in the BDA graph
(11:56:56) Flavia: Hm... so you may end up needing to create a bean of a type that is not in the BDA
(11:57:05) Pete: exactly

So, in a nutshell, all I have to do is change DeploymentImpl.loadBDA() so it adds the Class to the BDA if the BDA it is returning lacks that class.

[flavia.rainone]

Ales Justin wrote:

Yes. If there is an accessible (i.e. classes can be loaded from it) library jar with META-INF/beans.xml, then Weld expects you to return it as part of the BDA graph of the Deployment.

OK, this then rules out my lazy approach. :-)

But I would still do this on_demand, when (but just before) the first CDI/Weld deployment is deployed.

That would be the default behavior, but it could be made optional to do this at boot time instead.

I agree with you. This is next on my todo list.

[alesj]

The behavior should be deterministic / well-defined from the spec.

My guess is the previous behavior was correct, if not, then like you said, it's just a matter of fixing the tests.

But if it was correct, then your impl needs fixing in the following way -- actually a small change:

Notice that, interestingly, in the way things are implemented today, if you pass one of the missing classes as a parameter to a Deployment.loadBeanDeploymentArchive, my implementation will return the requested BDA correctly, but still the BDA will lack the requested class!

== simply dynamically add the classes of requested class' owner jar to BDA's classes.

Even thought our "glue" for BDA mapping is CL, it doesn't mean they are 1:1.

And with that suggested fix I don't see the need for more fine grained mapping.

[alesj]

but it could be made optional to do this at boot time instead.

The easiest way to do this would be to add a deployer and somesort of weld-lib repository service to the bootstrap process.

e.g. part of bootstrap.xml sub-systems, which would simply take a set of lib locations to check,

and spit out a set of lib jars that contain META-INF/beans.xml -- note, there is no Weld dependency, hence can be easily put into bootstrap

[flavia.rainone]

This is about the TODO comment that Ales added to DeploymentImpl:

public BeanDeploymentArchive loadBeanDeploymentArchive(Class<?> beanClass)
   {
      // collection to mark the archives we have already searched
      Collection<Archive> searchedArchives = new HashSet<Archive>();
      // collection to mark the classpaths we have already searched
      Collection<Classpath> searchedClasspaths = new HashSet<Classpath>();
      // TODO -- why the search? beanClass' ClassLoader should be mapped to Archive?
      // need to throw an IllegalArgumentException if the Archive is not reachable from
      // the archives contained in the archives of this deployment
      Archive archive = findArchive(beanClass, archives, searchedArchives, searchedClasspaths);
      if (archive == null)

I put a temporary answer right below the TODO as you can see. It is undeniable that Ales' suggested approach is much simpler and faster than searching for the Archive in the graph.

The problem is that we need to throw the IllegalArgumentException whenever the Archive is not reachable. I'm wondering if that exception is really necessary, as IMHO the gains of following Ales suggestion outweight the fact that we are not checking for Archive visibility.

So, in order for this exception to happen, we would need to assume that Weld is asking for a bean loaded by a ClassLoader that is not visible to the ClassLoader that loaded the current deployment. Would this scenario ever happen?