Hi David,
Thanks for this report, I'm impressed on your deep understanding of the
subject. Class loading is one of the most difficult areas, and until Nacho
implemented most of the new loader scheme we had lots of problems. ( that
happened few months ago ).
> Here's the list of things that I consider to be broken, based on the
> understandings that:
>
>
> A) the hierarchy should go:
>
> SystemCL
> | <-- LAYER 1
> lib/common CL
> / \ <-- LAYER 2
> lib/container lib/apps CL
> | <-- LAYER 3
> WEB-INF/lib << same CL, but with URLs
> WEB-INF/classes from both dirs, in this
> order...
You are absolutely right, that's the intended hierarchy.
What's important is that lib/container and the webapps are sibling, and
standard parent delegation is used ( with all jaxp-like problems avoided
by the fact that the webapps no longer 'see' libs used by container ).
> List of broken behaviours:
>
> 1) LAYER 1 and LAYER 2 are broken ( do not delegate to parent )
>
>
> org.apache.tomcat.modules.config.ProfileLoader [ -r ]
ProfileLoader is not actively used right now, it needs more testing.
It's goal is to go one step forward and allow:
- another layer for webapps, allowing "groups" ( subsets ) of webapps to
share some libs ( that are not shared by all webapps ).
Webapps need to share libs in order to be able to communicate (
internal include, forward, etc - the attributes must be loaded by the same
loader). But the "global" webapps classloader is not good as it'll
again create versioning problems ( all apps on the server will share it,
but on a big server you may have multiple groups of cooperating apps )
- for container, allow use of a mechanism similar with webapps, where
different modules can be inserted/reloaded at runtime, without restarting
the server. What have to be common ( and fixed ) is the core ( which
shouldn't change anyway ), the config modules and the context
mapper. All other modules ( auth, session, etc ) can be loaded as context
modules - and as a consequence it is possible to reload them, add, etc.
( I'm trying to start this with jasper34 - package it as a
"trusted"/special webapp, and allow people to upgrade it easily - next
step will be hot upgrade )
Now, regarding the bugs you found - thanks for reviewing and taking the
time to find them.
> line 320: commonLoader is set to be its own parent, if there
> was previously a commonLoader, or the System
> CL if not.
This is the commonLoader for the _profile_. Each "profile" represents a
refinement of the "main" class loaders.
A profile is a group of webapps, sharing common jars - for example they
may all use jasper34 ( while other groups will keep using jasper33 ). In
this case, jasper34 will be part of the container/jasper34 class
loader ( child of container class loader ).
The real problem is that we can't have multiple inheritance for the class
loader hierarchy - I'm still trying to figure out an intuitive way to
represent that. It's easy for container profile ( i.e. jasper34, modules
that are used only for some apps and could be reloaded without restarting
the server ), it's easy for webapps profiles ( like groups of webapps
sharing a jaxp impl. ). It's not easy for "common", and it's not easy to
put them togheter.
That's why ProfileLoader is not enabled ( or used ) right now.
> this is also a potential memory leak after a while,
> if initClassLoaders() is called more than once.
Initialization happens only at startup ( or soft server reload - but
that part is not even started )
> 2) LAYER 3 bypasses straight to the System CL:
> if( useAL && !context.isTrusted() ) {
> if( debug > 0 ) log( "Using webapp loader " + context.isTrusted());
> parent=cm.getParentLoader();
> } else if( useNoParent ) {
> if( debug > 0 ) log( "Using no parent loader ");
> parent=null;
> } else {
> if( debug > 0 ) log( "Using container loader ");
> parent=this.getClass().getClassLoader();
> }
>
>
> I am not really sure why this piece of code is as it is.
>
> note:
>
> - useAL is set by setUseApplicationLoader( boolean ).
> I could find no callers of this method in the source,
> but the method is still public.
The config mechansim will call this ( using introspection ) if the user
add an attribute useApplicationLoader="true" in <LoaderIntercepotor >
element in server.xml.
> - same story for useNoParent, which is set by
> setUseNoParent( boolean )
Same - it's an user option that defines how the webapp should behave.
Both 1 and 2 are triggered by user settings ( for example the /admin
app will have "trusted" attribute set, i.e. case 3).
The default setting is useApplicationLoader="true", as part of server.xml
( I'll fix it in LoaderInterceptor to make it default here too ).
> - so it must always default to if(3), and this line
> does NOT gurarantee delegation to the
> container loader! ( expected
> Context.getContextManager().getContainerLoader()
> instead )
I'll fix that, but should work with the current code too, since
LoaderInterceptor is loaded with the container loader.
A simpler form for the if():
- normal apps are in case 1 ( useApplicationLoader=true, not trusted )
- if ! useApplicationLoader:
-- noParent option means the apps will bypass even the common loader,
have the system loader as parent
-- otherwise they'll use the container class loader ( well, this is a
potential bug - it it's not trusted it shouldn't use the container loader)
- if trusted, it'll use the container loader.
> 3) Inconsistent directory-scanning behaviours
That's result of not-finished refactoring, it happens quite often to find
a piece of code duplicated in 2 classes - and the normal action is to move
the common code in a util and share it. That's the intention, if you look
in cvs history you'll see the code from ProfileLoader and Main was moved
in IntrospectionUtils ( where it is now shared ). I forgot about
LoaderInterceptor using the same pattern.
> Here's a list of questions I need answered to make some improvements:
>
> 0) Are there good historical reasons why the above behaviours are as
> they are?
There are historical reasons, I'm not sure they are good.
But I'm very sure the common/container/apps hierarchy and normal
delegation are the right thing.
ProfileLoader is open for any sugestions ( well, everything is - but
ProfileLoader really need some help )
The actual implementation probably have some bugs, like any other code -
I didn't see any real problem in class loading since we implemented the
loader hierarchy, but your review shows some potential ones.
> 1) is it OK to not use the IntrospectionUtils when constructing
> classpaths? I find its API a little awkward. ( Would factor out
> classpath-ish utilities into their own Class, deprecate existing
> classpath-ish methods on IntrospectionUtils ).
> 1b) assuming yes, what package should that class be placed into?
Well, speaking of historical reasons - that's the reason I put the code
that was duplicated in Main and ProfileLoader in IntrospectionUtil.
I couldn't figure out what package and class name should I use :-)
( and IntrospectionUtil was already used for all the magic that sets the
class loaders and runs Main ).
But I totally agree, if you have a good package and class name (
org.apache.tomcat.util.???.ClassLoader??? ), we should make the move.
> 2) is it OK to introduce the following new behaviour that allows the
> user to specify order in directories where classloading occurs?
I understand the problem you are trying to solve, but:
- you should _not_ rely on classloader ordering ( you never know if the
internal class loader impl is using a hashtable to store the paths )
- for webapps, the code will not be portable ( but it would be ok for the
container classes for this matter ).
- you can already use a system property and the system loader ( and
CLASSPATH ) for the common loader
- the right solution ( if I understood the problem ) will be
ProfileLoader.
But:
- since ProfileLoader is not ready, I guess your patch would be ok if it's
reasonably simple.
( I don't like playing with class loaders, and this should be seriously
reviewed/aproved by others - especially Nacho, my vote would be a cautious
+0 )
> IF there is a file called 'classpath.properties' ( call it what you
> like ), in the root directory of a classpath location, or in any of its
> descended directories,
...
> 3) where is the appropriate place to put user-documentation about
> classloading behaviour?
Anywhere you like ! ( but most likely under docs/ ). Just write a good
documentation for classloading behavior - you can use any tab size you
want in it ( except 13 and other prime numbers ). :-)
> 4) is there a test ( eg. JUnit test? ) or test-suite (
> eg. WatchDog? ) or test web-app, that verifies contracts about
> classloading behaviour with respect to the Servlet 2.2 specification?
The servlet2.2 doesn't specify too much. We do try to implement the
behavior specified in 2.3 ( where 2.2 leaves it open ) to easy
migration/compatibility.
For this reason the apps default with the common dir, and the common dir
contains only servlet.jar and classes that should not be overriden ( well,
arguably some of them should be moved to container - but right now it's
much cleaner to keep jasper_runtime and few others in common ).
So far the best sanity test for class loader was jasper ( and it's
watchdog/sanity tests ). ( i.e. if something brakes in class loader,
jasper is most likely to notice ).
It's quite difficult to even describe the behavior, writing special tests
would be a challenge ( I think ) - but if you can create a test it will
certainly be usefull. Right now wee are not using JUnit or equivalent in
tomcat3.x, you'll have to write some server-side code - a webapp or add
something to the /test webapp, and a small client using Gtest ( it can
send requests and do matches on the result from your server side code ).
> "The key ingredients of success are a crystal-clear goal, a realistic
> attack plan to achieve that goal, and consistent, daily action to reach
> that goal."
>
> Steve Maguire, "Debugging the Development Process".
Nice .signature
Costin
