-
30. Re: Testing Deployers with new Reflect + ClassPool
I have filled in the TODOs locally and have fixed those tests. Part of those failures was being caused by this bug https://jira.jboss.org/jira/browse/JBREFLECT-66.
Ales was also right that RegisterModuleCallback shoudln't ignore modules of other types different than AbstractDeploymentClassLoaderPolicyModule and this was causing part of the failures.
After fixing both issues, I still see some failures. The problem here is that these failures are random and cannot be reproduced from my Eclipse test case environment. These are the tests that fail sometimes:
ClassPoolTestCase.testHierarchyNonDeploymentModule JavassistTypeInfoTestCase.testDomainHierarchy
JavassistTypeInfoTestCase.testHierarchyNonDeploymentModule
I'll continue investigating. Once I have all this sorted out, I'll commit the full version of those tests. -
31. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
I'll continue investigating. Once I have all this sorted out, I'll commit the full version of those tests.
The cause of the failure is that the cleanup of classpools is not being performed. At the moment RegisterModuleCallback.removeModule is invoked, the classloader in the module is already null:logger.debug("Removing module " + module); ClassLoader classLoader = ClassLoading.getClassLoaderForModule(module); ClassPoolRepository.getInstance().unregisterClassLoader(classLoader); domainRegistry.cleanupLoader(classLoader); registeredModules.remove(module); unregisteredModules.remove(module);
I can use the maps in domainRegistry in order to get to the classloader and "fix" this. However, those maps use WeakReference and I can never be sure whether the class loader is gc'ed at the moment I try to use those maps. If it is gc'ed, the associated ClassPool won't be cleaned up.
To workaround this scenario, I can see three options here:
1. replace the CL weak reference by a hard reference. That will garantee that we can do the right cleanup. This has a downside because weak references are safe against memory leaks. But we will always get rid of the hard reference on removeModule, and this method must always be called or otherwise the classpools won't work.
2. add checks in strategic points of the code. Those checks will look for the classpools associated with a null CL, performing a clean up of those (there is already a method for doing this in ScopedClassPoolRepository). IMO, this has the disadvantage that we are trying to cover ourselves from a ClassPool cleanup that failed, when in reality we should be fixing the cleanup itself instead of trying to cover the failure in other points of the code.
3. create an extra map in DomainRegistry that maps Modules to ClassPools. The downside of this option is that it kind of breaks the current design, because we use:ClassPool javassist.scopedpool.ScopedClassPoolRepository.registerClassLoader(ClassLoader cl)
to create a ClassPool, and we use:void javassist.scopedpool.ScopedClassPoolRepository.unregisterClassLoader(ClassLoader cl)
to clean it up. So we would have to create an unregister method that receives the module as a paramter and add this method to org.jboss.classpool.spi.ClassPoolRepository (subclass of ScopedCPRepository).
Which way should I go? My vote is for the first option. -
32. Re: Testing Deployers with new Reflect + ClassPool
This kind of problem indicates that CL is not the right key to map things against.
OK, you definitely need some CL mapping,
as that's what is available from plain / non-deployment code.
But you should make it in such way that you can do proper cleanup with Module only.
e.g.
With register code you create hierarchy with Module's help, but also apply proper CL mappings.
Where you somehow tie those CL mappings with Module.
(perhaps a simple WeakHashMap by CL key is enough here)
But the real cleanup code is done by Module mapping. -
33. Re: Testing Deployers with new Reflect + ClassPool
"alesj" wrote:
But you should make it in such way that you can do proper cleanup with Module only.
e.g.
With register code you create hierarchy with Module's help, but also apply proper CL mappings.
Where you somehow tie those CL mappings with Module.
(perhaps a simple WeakHashMap by CL key is enough here)
But the real cleanup code is done by Module mapping.
Those CL mappings are already tied with Module. What I currently lack is a map from Modules to ClassPools. I guess I'll create a JBossClDelegatingCPRepository (BTW, maybe we should refactor those names?) and add a unregister(ClassLoader, Module) like the one below:public void unregisterClassLoader(ClassLoader classLoader, Module module){ ClassPool classPool = registeredModules.remove(module); // remove from the newly created map module->classpool if (classLoader == null) classPool.close(); // perform CP cleanup else this.unregisterClassLoader(classLoader); // needs to call the original unregister module so // that the registeredCLs mapping is cleaned up as well; this method also invokes classPool.close() // and this should not be done twice } -
34. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
(...)I guess I'll create a JBossClDelegatingCPRepository (BTW, maybe we should refactor those names?
On a separate note, as it is today, the jboss-classpool project contains several legacy classpools, that were useful for previous classloader models.
I've kept those intact because the plan is to port the refactoring I've done on JBoss AOP (JBAOP-742) to JBoss AOP's trunk. For that, I would like to have Kabir's validation on JBAOP-742 refactoring and on the removal of some classpools. IMO the only classpool we should keep, besides JBossClDelegatingCP, is the unified class loader compatible (plus some helper CPs, such as Temp CPs). The other classpools, such as JBoss5ClassPool, are never used and I I think they could be removed. That would greatly simplify jboss-classpool project. And, on a next level, once this code cleanup is complete, I wonder if we can find a shorter name for JBossClDelegatingCP and a simpler CP hierarchy for it. -
35. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
The other classpools, such as JBoss5ClassPool, are never used and I I think they could be removed.
Yes I think this can be removed. As I remember, it this was the first implementation of understanding the JBoss 4 style classloading using the JBoss 5 classloading spi. This was then later replaced by the delegating classpools which were able to understand import/export by delegating to Module -
36. Re: Testing Deployers with new Reflect + ClassPool
I found a random bug in ClassPool project.
Check this scenario:
1. An ear with two wars is created and deployed.
2. When the test invokes ClassPoolRepository.registerClassLoader, passing the ear class loader as an argument, it will trigger the creation of the ClassPool corresponding to the ear module. This creation is performed by JBossClDelegatingClassPoolFactory
3. The factory will perform a series of steps: creating the parent class pools(a), then creating the bootstrap classpools (b), creating the classpool domain(c), and finally creating the requested class pool itself(d).
4. In the given scenario, the class loader has no parent, so there is no parent class pool to be created (a).
5. When the factory reaches the bootstrap classpool creation is the problem. The "bootstrap" modules are the modules that have been registered in the RegisterModuleCallback but have no corresponding classpool yet. So, in the given scenario, the factory will iterate for a collection containing both war modules, as none of them has the corresponding classpool created yet.
7. Starting with the first war, this classpool creation will result in recursive call to JBossClDelegatingClassPoolFactory. It will again iterate for the unregistered classpool collection, which now contains only the other war. Again, we have a recursive call to create the classpool for this second war.
8. Reaching the second war, the factory will have no "bootstrap" classpools to create. So it will move on to step (c), creating the class pool domain. But the domain of the war is the ear classloader. So, it will create the classPool for the ear that was requested during step 2. It will then create the classpool for this second war and return.
9. Now, once the recursive call is returned to the context in which the classpool for the first war is created, the "bootstrap" cp creation is considered finished(b), and it will move on to creation of the domain classpool(c). Again, the domain is the ear classloader. As this step invokes ClassPoolRepository.registerClassLoader, it will be able of recognizing that a classpool for this ear has already been created, during the previous step.
10. Returning to the original context of the CP factory, in which the cp for the ear was being created. This step will try to create a CP for the second "bootstrap" module, that is the second war. Again, as this invokes ClassPoolRepository.registerClassLoader, it will recognize that this CP is created and consider this done (b).
11. Finally, the ClassPoolFactory finds the CP domain of the ear (app classLoader) (c) and creates the CP corresponding to the ear module (d). The problem is that this ear has already been created during step 8.
So, I thought that this "bootstrap" cp creation was not needed. We first create the parent pools and then the domain pools, so why would we need to create other pools? I resolved the problem getting rid the "bootstrap" cp creation. Unfortunately, this breaks the testNonDeploymentModule test. This test deploys a jar that contains the following beans.xml file:<deployment xmlns="urn:jboss:bean-deployer:2.0"> <classloader><inject bean="anys-classloader:0.0.0"/></classloader> <classloader name="anys-classloader" xmlns="urn:jboss:classloader:1.0" import-all="true"> <capabilities> <package name="org.jboss.test.deployers.vfs.reflect.support.web"/> </capabilities> <root>${jboss.tests.url}</root> </classloader> <bean name="AnyServlet" class="org.jboss.test.deployers.vfs.reflect.support.web.AnyServlet"/> </deployment>
The module corresponding to anys-classloader is registered in the RegisterModuleCallback and would result in a CP creation during the "bootstrap" CP creation. As I commented out that part of the code, this test no longer works.
I was wondering is there is any way of identifying that specific type of scenario. If I could do that, I would generate the "bootstrap" CP only for "non-deployment" modules. This would avoid the problem in the scenario I described above without breaking testNonDeploymentModule. -
37. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
I was wondering is there is any way of identifying that specific type of scenario. If I could do that, I would generate the "bootstrap" CP only for "non-deployment" modules. This would avoid the problem in the scenario I described above without breaking testNonDeploymentModule.
Actually, this is not what I want to do. This kind of fix would be useless, because, AFAIK, I could create the same ear scenario I described before using an beans.xml file. In that case, the "war" corresponding modules wouldn't be filtered out of JBossClDelegatingCPFactory.registerBootstrapLoaders, and the bug would occur again. -
38. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
"flavia.rainone@jboss.com" wrote:
I was wondering is there is any way of identifying that specific type of scenario. If I could do that, I would generate the "bootstrap" CP only for "non-deployment" modules. This would avoid the problem in the scenario I described above without breaking testNonDeploymentModule.
Actually, this is not what I want to do. This kind of fix would be useless, because, AFAIK, I could create the same ear scenario I described before using an beans.xml file. In that case, the "war" corresponding modules wouldn't be filtered out of JBossClDelegatingCPFactory.registerBootstrapLoaders, and the bug would occur again.
I want to filter out the "bootstrap" modules that correspond to a submodule of the module whose CP is currently being created. In other words, in the scenario I described, where the war classLoader has the ear class loader as its domain (using an adapter), if the ear module is the one that was originally requested to JBossClDelegatingCPFactory, the war module is going to be skipped as a bootscrap cp. I think that that solves the bug.
Is there an easy way of filtering out those scenarios? If not, I'm going to use the recursive structure of JBossClDelegatingCPFactory.registerBootstrapLoaders itself to abort the creation of a CP when its domain corresponds to a CP that is being created in an outer scope of the recursion. -
39. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
Is there an easy way of filtering out those scenarios? If not, I'm going to use the recursive structure of JBossClDelegatingCPFactory.registerBootstrapLoaders itself to abort the creation of a CP when its domain corresponds to a CP that is being created in an outer scope of the recursion.
I meant filtering out those modules, and not scenarios. -
40. Re: Testing Deployers with new Reflect + ClassPool
This is the lifecycle of module resgistry and class pools creation today:
1. Use RegisterModuleCallback as a ModuleRegistry so it can be notified of modules being deployed and undeployed
2. RegisterModuleCallback.addModule adds the module to an unrgisteredModules collection.
3. When ClassPoolRepository.registerClassLoader(CL) is invoked, this will trigger a call to JBossClDelegatingCPFactory.
4. The factory will invoke RegisterModuleCallback.registerModule. At this point, the module is going to be reomved from the unregisteredModules collection and added to the registeredModulesCollection.
5. The factory will iterate through all unregisteredModules of RegisterModuleCallback (the so-called "bootstrap" modules) in order to create the "bootstrap" CPs.
Ales suggested me that I got rid of this way of creating ClassPools, making sure that the ClassPool is created by RegisterModuleCallback.addModule. This way, we get rid of the unregisteredCP collection and we get rid of the "bootstrap" CP creation that causes the aforementioned bug.
Unfortunately, that doesn't work :(. I need the ClassLoader in order to create the ClassPool, and at the point RegisterModuleCallback.addModule is invoked the class loader associated with the module is null.
I'm assuming that there is no easy way of filtering out the modules I need to filter. So I'll use the recursion structure itself to deduct when the module needs to be filtered out and I'll abort the CP creation for those modules. -
41. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
So I'll use the recursion structure itself to deduct when the module needs to be filtered out and I'll abort the CP creation for those modules.
That worked. :)
However, I still see failures. This time, it looks like there is an error with the unregistering of "non-depolyment" modules. This error is not visible because ClassLoading.removeModule ignores Exceptions thrown by ModuleRegistries. But as a result of this Exception, the cleanup is not done properly and we end up with trash class pools in the default domain, which results in a failure on ReflectTestSuite. -
42. Re: Testing Deployers with new Reflect + ClassPool
The tests work now. I committed the fixes and the complete version of Ales tests.
I have done a clean checkout of deployers tests and confirmed they are passing for me.
This is a complete list of the Jiras involved with the bugs discussed in this thread:
https://jira.jboss.org/jira/browse/JBDEPLOY-216
https://jira.jboss.org/jira/browse/JBREFLECT-65
https://jira.jboss.org/jira/browse/JBREFLECT-66
https://jira.jboss.org/jira/browse/JBREFLECT-67
https://jira.jboss.org/jira/browse/JBREFLECT-68
https://jira.jboss.org/jira/browse/JBREFLECT-69 (the bug responsible for the random failures)
Notice that, despite the wide coverage of the class pool tests Kabir wrote, most of the bugs were uncovered by the deployers-vfs tests. The reason for this is that those tests were reregistering stuff, which hid the bugs (JBREFLECT-67). Now, I don't know if we need to port the vfs tests to class pool or not. Maybe I should only port the "key" tests, that uncovered bugs.
With the refactoring performed for JBREFLECT-68, I'm thinking on getting rid of RegisterModuleCallback, incorporating its code to VFSCLDomainRegistry (which, BTW, I'll probably rename). -
43. Re: Testing Deployers with new Reflect + ClassPool
"flavia.rainone@jboss.com" wrote:
Now, I don't know if we need to port the vfs tests to class pool or not. Maybe I should only port the "key" tests, that uncovered bugs.
Something definitely needs porting, or any form of "duplication".
As these errors should already be spotted in the Classpool project itself. -
44. Re: Testing Deployers with new Reflect + ClassPool
"alesj" wrote:
"flavia.rainone@jboss.com" wrote:
Now, I don't know if we need to port the vfs tests to class pool or not. Maybe I should only port the "key" tests, that uncovered bugs.
Something definitely needs porting, or any form of "duplication".
As these errors should already be spotted in the Classpool project itself.
I started porting this tests and It looks like I've already found another bug :(
Apparently, bootstrap classes are being loaded by the wrong class pool.
Ales, you mentioned on a private chat that jboss-classpool should not use VFS because that would result in a circular dependency. Have you taken a look at the class pool tests? They already use VFS stuff, even though the pom doesn't declare any direct dependency to VFS. I wonder if those tests are ok or if I'll have to "fix" them, by removing the VFS part.