You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Simon Kitching <sk...@apache.org> on 2006/02/13 02:21:42 UTC
[logging] r375631 - systemClassloaderTried patch
Hi Robert,
I've had a look at this patch and it doesn't seem quite right to me.
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.
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?
Regards,
Simon
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [logging] r375631 - systemClassloaderTried patch
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Tue, 2006-02-14 at 14:29 +1300, Simon Kitching wrote:
> 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?
the javadocs for Class#forName use the term 'bootstrap' as well. getting
hold of the boot classloader is platform dependent but passing null to
Class#forName shouldn't be.
> > 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.
sounds like reverting the patch is the right thing to do, so that's what
i'll do...
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [logging] r375631 - systemClassloaderTried patch
Posted by Simon Kitching <sk...@apache.org>.
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
Re: [logging] r375631 - systemClassloaderTried patch
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
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 :)
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.
i'd be equally happy to revert and go with just a dianostic message for
now, though.
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org