Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/
Gianny, Can you provide some more information on how you might accomplish what you are proposing? GeronimoTldLocationsCache is a specialization of the Jasper TldLocationsCache functionality and we configure Jasper to use our version. So, much of the fundamental logic involved in resolving the tld resources is embedded in Jasper. See http://svn.apache.org/viewvc?view=rev&rev=517712 for the original change and the JIRA https://issues.apache.org/jira/browse/GERONIMO-2955 . So, if we revert back to the jasper TldLocationsCache our classloader implementations of getResource() will never be invoked because we will not have traversed the parent classloaders correctly to add them to the cache. Also, given the differences between MultiParentClassLoader and ChildrenConfigurationClassLoader it is not clear to me how we would walk the appropriate parentage without some type of instance check. We do honor much of the classloading logic there today in walking the parentage and eventually when jasper goes about loading the resources. Joe Gianny Damour wrote: Hi Joe, Thanks for your investigations. This is a very good catch. Based on a cursory review of GeronimoTldLocationsCache, I believe that the implementation can be improved to remove such a weird instanceof identification mechanism, which is fragile and increases coupling to Geronimo class loading internals. Furthermore, the implementation simply skips any logic, e.g. class loading logic, which is encapsulated within a class loader. An integration analogy would be to directly retrieve stuff from a database instead of going through the application layer providing relevant business logic to retrieve the stuff you are looking for. My understanding is that ClassLoader.getResources() can be used to implement GeronimoTldLocationsCache.scanJars without requiring any dependency on MCCL and CCCL. Thanks, Gianny On 21/11/2008, at 6:39 AM, Joe Bohn wrote: I finally just checked in a fix for this. Turned out it wasn't related to the resource processing in the classloader ... but rather to the jasper navigation of the classloader hierarchy that had to be extended for the new ChildrenConfigurationClassLoader. See http://svn.apache.org/viewvc?rev=719329&view=rev Joe Joe Bohn wrote: I think I'm getting it narrowed down a bit. It seems that when ChildrenConfigurationClassLoader.getResource(name) is called it never actually resulting in invoking MultiParentClassLoader.getResource(name) (at least not in this case). The MPCL should have been passed in when the CCCL was constructed. CCCL.getResource calls super.getResource (super being SecureClassLoader) which should ultimately result in MPCL.getResource being invoked - but that doesn't seem to be happening. Perhaps we should update CCCL.getResource to directly invoke MPCL.getResource rather than super.getResource? I guess the other possibility is that the parent isn't what I think it should be ... still investigating. Joe Joe Bohn wrote: Gianny Damour wrote: On 19/11/2008, at 5:19 AM, Joe Bohn wrote: Joe Bohn wrote: Just a heads up that I *think* there are still some issues with this change. It appears that the hiddenResource processing from the MultiParentClassloader was removed. Correction ... it was not removed but rather changed and relocated. I'm still looking to understand why this is causing a problem. Hi, Indeed, I changed the implementation to properly encapsulate class loading rules. The new implementation is way cleaner this way; when my frustration coming from reported issues will reduce, I may use this better encapsulation to add import and export class loading rules. I agree that what you added for the ClassLoadingRules is cleaner. However, I think it might be the ChildrenConfigurationClassLoader and it's replacement of MultiParentClassLoader in the Configuration that might be causing us problems. The testsuite failures that I mentioned are failing as well as nearly all of the jstl tck tests since this change. Perhaps it is working as designed, but it seems strange to me that as we process the "parent" classloaders they are all of type "ChildrenConfigurationClassLoader" when they used to be "MultiParentClassLoaders". BTW, I've verified that these tests were not failing prior to this change and begin to fail with just the change a few other necessary changes (the reverting of the xsd changes and fixing the dependency history files). I think this has resulted in some testsuite failures involving tld processing. There are failures in the web-testsuite/test-2.1-jsps and web-testsuite/test-myfaces tests. I'm looking at what would be necessary to add in the hiddenResource logic again hoping that will resolve the issue. This is a really nice feature and it is great to have the capability. However could you please run the testsuite in the future to avoid problems like this (especially when introducing fundamenta
Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/
Hi Joe, Thanks for your investigations. This is a very good catch. Based on a cursory review of GeronimoTldLocationsCache, I believe that the implementation can be improved to remove such a weird instanceof identification mechanism, which is fragile and increases coupling to Geronimo class loading internals. Furthermore, the implementation simply skips any logic, e.g. class loading logic, which is encapsulated within a class loader. An integration analogy would be to directly retrieve stuff from a database instead of going through the application layer providing relevant business logic to retrieve the stuff you are looking for. My understanding is that ClassLoader.getResources() can be used to implement GeronimoTldLocationsCache.scanJars without requiring any dependency on MCCL and CCCL. Thanks, Gianny On 21/11/2008, at 6:39 AM, Joe Bohn wrote: I finally just checked in a fix for this. Turned out it wasn't related to the resource processing in the classloader ... but rather to the jasper navigation of the classloader hierarchy that had to be extended for the new ChildrenConfigurationClassLoader. See http://svn.apache.org/viewvc?rev=719329&view=rev Joe Joe Bohn wrote: I think I'm getting it narrowed down a bit. It seems that when ChildrenConfigurationClassLoader.getResource(name) is called it never actually resulting in invoking MultiParentClassLoader.getResource(name) (at least not in this case). The MPCL should have been passed in when the CCCL was constructed. CCCL.getResource calls super.getResource (super being SecureClassLoader) which should ultimately result in MPCL.getResource being invoked - but that doesn't seem to be happening. Perhaps we should update CCCL.getResource to directly invoke MPCL.getResource rather than super.getResource? I guess the other possibility is that the parent isn't what I think it should be ... still investigating. Joe Joe Bohn wrote: Gianny Damour wrote: On 19/11/2008, at 5:19 AM, Joe Bohn wrote: Joe Bohn wrote: Just a heads up that I *think* there are still some issues with this change. It appears that the hiddenResource processing from the MultiParentClassloader was removed. Correction ... it was not removed but rather changed and relocated. I'm still looking to understand why this is causing a problem. Hi, Indeed, I changed the implementation to properly encapsulate class loading rules. The new implementation is way cleaner this way; when my frustration coming from reported issues will reduce, I may use this better encapsulation to add import and export class loading rules. I agree that what you added for the ClassLoadingRules is cleaner. However, I think it might be the ChildrenConfigurationClassLoader and it's replacement of MultiParentClassLoader in the Configuration that might be causing us problems. The testsuite failures that I mentioned are failing as well as nearly all of the jstl tck tests since this change. Perhaps it is working as designed, but it seems strange to me that as we process the "parent" classloaders they are all of type "ChildrenConfigurationClassLoader" when they used to be "MultiParentClassLoaders". BTW, I've verified that these tests were not failing prior to this change and begin to fail with just the change a few other necessary changes (the reverting of the xsd changes and fixing the dependency history files). I think this has resulted in some testsuite failures involving tld processing. There are failures in the web-testsuite/test-2.1-jsps and web-testsuite/ test-myfaces tests. I'm looking at what would be necessary to add in the hiddenResource logic again hoping that will resolve the issue. This is a really nice feature and it is great to have the capability. However could you please run the testsuite in the future to avoid problems like this (especially when introducing fundamental changes like this)? If this is indeed a bug related to this change, then let's ensure that we have a proper unit test to prevent regression. Let's also ensure that this test is collocated with the proper component to improve cohesion. I have been observing various changes, many of them do not have proper unit tests and this causes problems further down the path as other developers can not safely change code. I'm fine with that, but I'm sure there are probably a number of more obscure situations that aren't covered by the unit tests ... they can only do so much. That's one of the reasons for the testsuite. Regarding private-classes, the current Geronimo <-> OpenEJB DD coupling is unfortunate. Does the OpenEJB deployer needs to know about the Geronimo environment? If it does not need to know about it, then let's strip from the DD the environment element and pass to OpenEJB the stripped version. This will reduce a little bit coupling. Another approach is to transform the Geronimo DD
Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/
I finally just checked in a fix for this. Turned out it wasn't related to the resource processing in the classloader ... but rather to the jasper navigation of the classloader hierarchy that had to be extended for the new ChildrenConfigurationClassLoader. See http://svn.apache.org/viewvc?rev=719329&view=rev Joe Joe Bohn wrote: I think I'm getting it narrowed down a bit. It seems that when ChildrenConfigurationClassLoader.getResource(name) is called it never actually resulting in invoking MultiParentClassLoader.getResource(name) (at least not in this case). The MPCL should have been passed in when the CCCL was constructed. CCCL.getResource calls super.getResource (super being SecureClassLoader) which should ultimately result in MPCL.getResource being invoked - but that doesn't seem to be happening. Perhaps we should update CCCL.getResource to directly invoke MPCL.getResource rather than super.getResource? I guess the other possibility is that the parent isn't what I think it should be ... still investigating. Joe Joe Bohn wrote: Gianny Damour wrote: On 19/11/2008, at 5:19 AM, Joe Bohn wrote: Joe Bohn wrote: Just a heads up that I *think* there are still some issues with this change. It appears that the hiddenResource processing from the MultiParentClassloader was removed. Correction ... it was not removed but rather changed and relocated. I'm still looking to understand why this is causing a problem. Hi, Indeed, I changed the implementation to properly encapsulate class loading rules. The new implementation is way cleaner this way; when my frustration coming from reported issues will reduce, I may use this better encapsulation to add import and export class loading rules. I agree that what you added for the ClassLoadingRules is cleaner. However, I think it might be the ChildrenConfigurationClassLoader and it's replacement of MultiParentClassLoader in the Configuration that might be causing us problems. The testsuite failures that I mentioned are failing as well as nearly all of the jstl tck tests since this change. Perhaps it is working as designed, but it seems strange to me that as we process the "parent" classloaders they are all of type "ChildrenConfigurationClassLoader" when they used to be "MultiParentClassLoaders". BTW, I've verified that these tests were not failing prior to this change and begin to fail with just the change a few other necessary changes (the reverting of the xsd changes and fixing the dependency history files). I think this has resulted in some testsuite failures involving tld processing. There are failures in the web-testsuite/test-2.1-jsps and web-testsuite/test-myfaces tests. I'm looking at what would be necessary to add in the hiddenResource logic again hoping that will resolve the issue. This is a really nice feature and it is great to have the capability. However could you please run the testsuite in the future to avoid problems like this (especially when introducing fundamental changes like this)? If this is indeed a bug related to this change, then let's ensure that we have a proper unit test to prevent regression. Let's also ensure that this test is collocated with the proper component to improve cohesion. I have been observing various changes, many of them do not have proper unit tests and this causes problems further down the path as other developers can not safely change code. I'm fine with that, but I'm sure there are probably a number of more obscure situations that aren't covered by the unit tests ... they can only do so much. That's one of the reasons for the testsuite. Regarding private-classes, the current Geronimo <-> OpenEJB DD coupling is unfortunate. Does the OpenEJB deployer needs to know about the Geronimo environment? If it does not need to know about it, then let's strip from the DD the environment element and pass to OpenEJB the stripped version. This will reduce a little bit coupling. Another approach is to transform the Geronimo DD into an OpenEJB supported DD when it is handed over to OpenEJB (in this case, we simply remove the private-classes). The creation of a canonical DD representation between Geronimo and OpenEJB will reduce coupling. I let you re-revert the private-classes change. I can't unrevert these changes since I'm not a committer on OpenEJB ... but perhaps we should wait on that anyway until we resolve the JSTL problems. Thanks, Gianny BTW, I'd personally like to see the plan changes re-introduced for private-classes if it turns out that we need an OpenEJB release anyway (and at this point in time I think that is the case). I think users are more accustomed to using declarative plans for this type of thing at the moment and would find this helpful. Thanks, Joe removed content of original change from this discussion thread.
Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/
I think I'm getting it narrowed down a bit. It seems that when ChildrenConfigurationClassLoader.getResource(name) is called it never actually resulting in invoking MultiParentClassLoader.getResource(name) (at least not in this case). The MPCL should have been passed in when the CCCL was constructed. CCCL.getResource calls super.getResource (super being SecureClassLoader) which should ultimately result in MPCL.getResource being invoked - but that doesn't seem to be happening. Perhaps we should update CCCL.getResource to directly invoke MPCL.getResource rather than super.getResource? I guess the other possibility is that the parent isn't what I think it should be ... still investigating. Joe Joe Bohn wrote: Gianny Damour wrote: On 19/11/2008, at 5:19 AM, Joe Bohn wrote: Joe Bohn wrote: Just a heads up that I *think* there are still some issues with this change. It appears that the hiddenResource processing from the MultiParentClassloader was removed. Correction ... it was not removed but rather changed and relocated. I'm still looking to understand why this is causing a problem. Hi, Indeed, I changed the implementation to properly encapsulate class loading rules. The new implementation is way cleaner this way; when my frustration coming from reported issues will reduce, I may use this better encapsulation to add import and export class loading rules. I agree that what you added for the ClassLoadingRules is cleaner. However, I think it might be the ChildrenConfigurationClassLoader and it's replacement of MultiParentClassLoader in the Configuration that might be causing us problems. The testsuite failures that I mentioned are failing as well as nearly all of the jstl tck tests since this change. Perhaps it is working as designed, but it seems strange to me that as we process the "parent" classloaders they are all of type "ChildrenConfigurationClassLoader" when they used to be "MultiParentClassLoaders". BTW, I've verified that these tests were not failing prior to this change and begin to fail with just the change a few other necessary changes (the reverting of the xsd changes and fixing the dependency history files). I think this has resulted in some testsuite failures involving tld processing. There are failures in the web-testsuite/test-2.1-jsps and web-testsuite/test-myfaces tests. I'm looking at what would be necessary to add in the hiddenResource logic again hoping that will resolve the issue. This is a really nice feature and it is great to have the capability. However could you please run the testsuite in the future to avoid problems like this (especially when introducing fundamental changes like this)? If this is indeed a bug related to this change, then let's ensure that we have a proper unit test to prevent regression. Let's also ensure that this test is collocated with the proper component to improve cohesion. I have been observing various changes, many of them do not have proper unit tests and this causes problems further down the path as other developers can not safely change code. I'm fine with that, but I'm sure there are probably a number of more obscure situations that aren't covered by the unit tests ... they can only do so much. That's one of the reasons for the testsuite. Regarding private-classes, the current Geronimo <-> OpenEJB DD coupling is unfortunate. Does the OpenEJB deployer needs to know about the Geronimo environment? If it does not need to know about it, then let's strip from the DD the environment element and pass to OpenEJB the stripped version. This will reduce a little bit coupling. Another approach is to transform the Geronimo DD into an OpenEJB supported DD when it is handed over to OpenEJB (in this case, we simply remove the private-classes). The creation of a canonical DD representation between Geronimo and OpenEJB will reduce coupling. I let you re-revert the private-classes change. I can't unrevert these changes since I'm not a committer on OpenEJB ... but perhaps we should wait on that anyway until we resolve the JSTL problems. Thanks, Gianny BTW, I'd personally like to see the plan changes re-introduced for private-classes if it turns out that we need an OpenEJB release anyway (and at this point in time I think that is the case). I think users are more accustomed to using declarative plans for this type of thing at the moment and would find this helpful. Thanks, Joe removed content of original change from this discussion thread.
Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/
Gianny Damour wrote: On 19/11/2008, at 5:19 AM, Joe Bohn wrote: Joe Bohn wrote: Just a heads up that I *think* there are still some issues with this change. It appears that the hiddenResource processing from the MultiParentClassloader was removed. Correction ... it was not removed but rather changed and relocated. I'm still looking to understand why this is causing a problem. Hi, Indeed, I changed the implementation to properly encapsulate class loading rules. The new implementation is way cleaner this way; when my frustration coming from reported issues will reduce, I may use this better encapsulation to add import and export class loading rules. I agree that what you added for the ClassLoadingRules is cleaner. However, I think it might be the ChildrenConfigurationClassLoader and it's replacement of MultiParentClassLoader in the Configuration that might be causing us problems. The testsuite failures that I mentioned are failing as well as nearly all of the jstl tck tests since this change. Perhaps it is working as designed, but it seems strange to me that as we process the "parent" classloaders they are all of type "ChildrenConfigurationClassLoader" when they used to be "MultiParentClassLoaders". BTW, I've verified that these tests were not failing prior to this change and begin to fail with just the change a few other necessary changes (the reverting of the xsd changes and fixing the dependency history files). I think this has resulted in some testsuite failures involving tld processing. There are failures in the web-testsuite/test-2.1-jsps and web-testsuite/test-myfaces tests. I'm looking at what would be necessary to add in the hiddenResource logic again hoping that will resolve the issue. This is a really nice feature and it is great to have the capability. However could you please run the testsuite in the future to avoid problems like this (especially when introducing fundamental changes like this)? If this is indeed a bug related to this change, then let's ensure that we have a proper unit test to prevent regression. Let's also ensure that this test is collocated with the proper component to improve cohesion. I have been observing various changes, many of them do not have proper unit tests and this causes problems further down the path as other developers can not safely change code. I'm fine with that, but I'm sure there are probably a number of more obscure situations that aren't covered by the unit tests ... they can only do so much. That's one of the reasons for the testsuite. Regarding private-classes, the current Geronimo <-> OpenEJB DD coupling is unfortunate. Does the OpenEJB deployer needs to know about the Geronimo environment? If it does not need to know about it, then let's strip from the DD the environment element and pass to OpenEJB the stripped version. This will reduce a little bit coupling. Another approach is to transform the Geronimo DD into an OpenEJB supported DD when it is handed over to OpenEJB (in this case, we simply remove the private-classes). The creation of a canonical DD representation between Geronimo and OpenEJB will reduce coupling. I let you re-revert the private-classes change. I can't unrevert these changes since I'm not a committer on OpenEJB ... but perhaps we should wait on that anyway until we resolve the JSTL problems. Thanks, Gianny BTW, I'd personally like to see the plan changes re-introduced for private-classes if it turns out that we need an OpenEJB release anyway (and at this point in time I think that is the case). I think users are more accustomed to using declarative plans for this type of thing at the moment and would find this helpful. Thanks, Joe removed content of original change from this discussion thread.
Re: svn commit: r712326 - in /geronimo/server/trunk: framework/modules/geronimo-kernel/src/main/java/org/apache/geronimo/kernel/classloader/ framework/modules/geronimo-kernel/src/main/java/org/apache/
On Nov 18, 2008, at 12:39 PM, Gianny Damour wrote: On 19/11/2008, at 5:19 AM, Joe Bohn wrote: Joe Bohn wrote: Just a heads up that I *think* there are still some issues with this change. It appears that the hiddenResource processing from the MultiParentClassloader was removed. Correction ... it was not removed but rather changed and relocated. I'm still looking to understand why this is causing a problem. Hi, Indeed, I changed the implementation to properly encapsulate class loading rules. The new implementation is way cleaner this way; when my frustration coming from reported issues will reduce, I may use this better encapsulation to add import and export class loading rules. I think this has resulted in some testsuite failures involving tld processing. There are failures in the web-testsuite/test-2.1-jsps and web-testsuite/test-myfaces tests. I'm looking at what would be necessary to add in the hiddenResource logic again hoping that will resolve the issue. This is a really nice feature and it is great to have the capability. However could you please run the testsuite in the future to avoid problems like this (especially when introducing fundamental changes like this)? If this is indeed a bug related to this change, then let's ensure that we have a proper unit test to prevent regression. Let's also ensure that this test is collocated with the proper component to improve cohesion. I have been observing various changes, many of them do not have proper unit tests and this causes problems further down the path as other developers can not safely change code. Regarding private-classes, the current Geronimo <-> OpenEJB DD coupling is unfortunate. Does the OpenEJB deployer needs to know about the Geronimo environment? If it does not need to know about it, then let's strip from the DD the environment element and pass to OpenEJB the stripped version. This will reduce a little bit coupling. Another approach is to transform the Geronimo DD into an OpenEJB supported DD when it is handed over to OpenEJB (in this case, we simply remove the private-classes). The creation of a canonical DD representation between Geronimo and OpenEJB will reduce coupling. I think the problem is caused by mismatch between our xmlbeans plan handling and the openejb jaxb plan handling. Maybe we should consider moving the jaxb classes to a more neutral location? This will probably take some work though. thanks david jencks I let you re-revert the private-classes change. Thanks, Gianny
