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 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.
