commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simon Kitching <skitch...@apache.org>
Subject Re: [logging] r375631 - systemClassloaderTried patch
Date Tue, 14 Feb 2006 01:29:45 GMT
On Mon, 2006-02-13 at 22:33 +0000, robert burrell donkin wrote:
> On Mon, 2006-02-13 at 14:21 +1300, Simon Kitching wrote:
> > Hi Robert,
> > 
> > I've had a look at this patch and it doesn't seem quite right to me.
> 
> i had the same feeling :)
> 
> but that might just be the specifications being unintuitive again...
> 
> > Presumably this is to handle the case where the system classloader is
> > not in the chain leading from base classloader to boot classloader. This
> > is a rather odd situation, but I guess that in the new principle of "try
> > everything before failing" we could have a go with that when all else
> > fails.
> 
> not quite
> 
> this patch is intended to address allowed variability in the classloader
> class implementation noted in the javadocs for ClassLoader#getParent. it
> is possible that some JRE implementations "may use null to represent the
> bootstrap class loader. This method will return null in such
> implementations if this class loader's parent is the bootstrap class
> loader."  
> 
> the term bootstrap classloader is difficult. i suspect that this javadoc
> is prior to the introduction of boot, extension and system classloaders.
> i elected to assume that using the system classloader would usually be
> good enough (providing that recursive loops are avoided) since i don't
> know of any cross-JRE way of accessing the boot classloader. (if anyone
> knows, please jump in :)

I would interpret this differently. I think "bootstrap class loader" is
the same thing as "boot classloader", which is indeed represented by
"null" in the Sun JVM. A classloader for which getParent returns null is
a child of the boot classloader.

Classes in the boot classloader can be accessed via
  ClassLoader loader = null;
  Class.forName(className, true, loader);
By passing null as the loader to use, the boot classloader is used. This
is exactly what the code in method createLogFromClass is already doing;
because null is tested for only at the end of the loop, an attempt is
made to use the null (boot) classloader already.

In early JVMs, I believe ClassLoader.getSystemClassLoader returns null,
ie the "system" and "boot" classloaders are the same. I believe that
some embedded JVMs do the same thing, even if they support java > 1.1

Am I mistaken in thinking "bootstrap classloader" and "boot classloader"
are the same thing?

> 
> but it seems like a lot of complex code to address a risk something this
> obscure...
> 
> > However this is currently hooked in to fire when parentCL is null, ie
> > *before* we try the bootloader. I believe that in simple JVMs the
> > bootloader IS also the system classloader. So I think it would be better
> > as:
> > 
> > final ClassLoader systemCL = ClassLoader.getSystemClassLoader();
> > boolean systemCLTried = false;
> > for(;;) {
> >   // checks as currently done
> > 
> >   if (currentCL == systemCL) {
> >     // the CL we just tested was the systemCL
> >     systemCLTried = true;
> >   }
> > 
> >   if (currentCL == null) {
> >     if (systemCLTried == true) {
> >       break;
> >     }
> > 
> >     // We've walked up the CL tree, but never visited the systemCL.
> >     // Before giving up, let's try walking up from the system CL too..
> >     currentCL = systemCL;
> >   } else {
> >     currentCL = currentCL.getParent();
> >   }
> > }
> > 
> > Thoughts?
> 
> this seems like a better formulation with equivalent effect.

Almost the same. However the above code effectively "inserts" the system
classloader only *after* trying the logic with the null classloader. The
recently committed patch inserts it *before* trying the null loader, and
never tries the null loader at all.

> 
> i'd be equally happy to revert and go with just a dianostic message for
> now, though.

Lets do that then. The situation where the system classloader is not in
the chain from TCCL to boot is possible, but very weird. 

Cheers,

Simon



---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message