Hi,
While checking all uses of static member variables in Xalan for race
conditions, I came across the following rather unusual code. While
*theoretically* an access to a class member variable without appropriate
locking, I don't think this is a problem in practice. However, I think
that there is some inadvisable exception suppression going on here
[which will almost certainly never be triggered; on the other hand, the
point of exceptions is to point out stuff that should never happen :-]
This code is pretty deep class-loader jujitsu [at least for me], so
please take my analysis with several grains of salt. I haven't raised a
bug report for this, as I'm not entirely sure my understanding of this
is correct.
And please also note that this is probably a pretty low-priority issue.
I don't *think* this is causing problems in "real life".
=======================================
Class: org.apache.xalan.extensions.ExtensionHandler
Member: private static Method getCCL
Method: getClassForName
On class load, member getCCL is set up to reference the
Thread.getContextClassLoader method associated with the Thread class
loaded by this class loader or an ancestor class loader. If this method
doesn't exist (eg java version is < 1.2) then it is set to null.
In method getClassForName:
* if getCCL is null, then the fallback "Class.forName" is invoked.
* otherwise, getCCL is invoked. If for some reason invocation fails, it
is set to null and
then the fallback Class.forName is used.
Issues:
(a)
The *theoretical* race condition occurs where class-member getCCL is set
to null, without any synchronisation. In practice, I don't think this is
an issue, because I can't see how the code which resets this variable
can ever be run [see points below].
(b)
Why, having successfully located the Method object for
getContextClassLoader, would invocation fail?
(c)
Why, if invocation has failed, should this issue be silently suppressed?
It certainly makes sense for a fallback to Class.forName to occur if the
Method cannot be found, as this allows support for Java < 1.2. This is
done in the static initialiser.
However, I suggest that the proper behaviour for failure to *invoke* the
method should be to throw a fatal java.lang.Error, or at least log a
very serious error report via the logging system.
I can't think of any situation where such a problem doesn't represent a
really screwed-up setup that isn't safe to continue running with. And
throwing an exception on invocation failure *instead of* setting
getCCL=null would also solve the theoretical race condition.
I do see, however, that a patch was deliberately applied (see details
later) to change the behaviour when ClassNotFoundException was thrown
during method invocation. So maybe there is some condition when this
gets triggered? [the comments don't say why the patch was applied]. I
can't find any bugzilla entry related to this, though.
=======================================
CVS History:
(a)
This method was added to this class in version 1.8, by "garyp".
In its initial version, it had the code to silently fall back to
Class.forName if invocation of the Thread.getContextClassLoader method
threw any kind of exception [except ClassNotFoundExeption, which was
allowed to propagate out of the method].
----------------------------
revision 1.8
date: 2000/12/03 21:12:35; author: garyp; state: Exp; lines: +53 -0
Load extension using thread's ContextClassLoader, if available.
Patch in XalanJ1 from Martin Klang <[EMAIL PROTECTED]>.
----------------------------
(b)
In version 1.8, an invocation of Thread.getContextClassLoader resulting
in ClassNotFoundException now is silently suppressed, falling back to
Class.forName:
----------------------------
revision 1.9
date: 2000/12/07 17:57:59; author: garyp; state: Exp; lines: +1 -1
Load extension function via system ClassLoader if ContextClassLoader
receives ClassNotFoundException.
----------------------------
I suspect this change was to prevent the invocation failure being
interpreted as a failure to find the original class specified in the
parameter. I personally think a better patch would have been to wrap the
ClassNotFoundException in a java.lang.Error subclass..
[NB: No criticism intended to "garyp". I just list the info above so
that this issue might be directed to the people with the best knowledge
of this area].
=======================================
The relevant code:
-----------
if (getCCL != null)
{
try {
ClassLoader contextClassLoader =
(ClassLoader)
getCCL.invoke(Thread.currentThread(), NO_OBJS);
result = contextClassLoader.loadClass(className);
}
catch (ClassNotFoundException cnfe)
{
result = Class.forName(className);
}
catch (Exception e)
{
getCCL = null;
result = Class.forName(className);
}
}
else
result = Class.forName(className);
-----------
Regards,
--
Simon Kitching <[EMAIL PROTECTED]>
