You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@xalan.apache.org by Simon Kitching <si...@ecnetwork.co.nz> on 2002/11/24 07:23:06 UTC
xalan-J: org.apache.xalan.extensions.ExtensionHandler: race
condition, exception supression [low priority]
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 <ma...@pingdynasty.com>.
----------------------------
(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 <si...@ecnetwork.co.nz>
RE: xalan-J: org.apache.xalan.extensions.ExtensionHandler:
racecondition, exception supression [low priority]
Posted by Simon Kitching <si...@ecnetwork.co.nz>.
Hi Gary,
On Mon, 2002-11-25 at 07:52, Gary L Peskin wrote:
[..]
> If, on the other hand, there is some other error, then we think it means
> that the context class loader is just not a suitable classloader. In
> that case, we tell XalanJ that it just shouldn't use the context class
> loader anymore because it's not suitable for loading extension classes.
> It doesn't mean we should report an error and stop processing. It just
> means that that's not the classloader for us.
Doh.. I didn't look clearly enough at the code.
I thought the catch clauses were for the line
getCCL.invoke(....)
whereas actually the code assumes that the invoke works (as I say in my
email, it really is expected to) and is instead (as you point out)
intended to handle exceptions from the following line
contextClassLoader.loadClass(...)
Would you believe I didn't even *see* that line? I was focussed too much
on tracking the use of the static member getCCL..
>
> While I agree that there is a possible race condition here, it seems
> harmless to me. The worst that will happen is multiple threads could
> set getCCL to null at different times, each having discovered that that
> context class loader is unsuitable. It doesn't seem worth it to
> synchronize this method for this case.
Consider two threads inside getClassForName:
* Thread 1 checks that getCCL != null, and enters the if-statement then
suspends.
* Thread 2 checks that getCCL != null, enters the if-statement, invokes
the method, gets an exception, sets getCCL to null.
* Thread 1 resumes, tries to invoke the method referenced by getCCL and
gets a NullPointerException.
The window of opportunity for this is, however, pretty damn small, as
this situation can only exist once (once getCCL is set to null, it is
null until the JVM exits). I agree that it isn't worth synchronizing for
this.
> Unless this is causing someone
> problems, I think we should leave this part the way it is.
Yep. If you work on this file sometime in the future, then maybe adding
a comment saying that this issue is known and is deliberately being
ignored would be nice..
Regards,
Simon
RE: xalan-J: org.apache.xalan.extensions.ExtensionHandler: racecondition, exception supression [low priority]
Posted by Gary L Peskin <ga...@firstech.com>.
Simon --
The idea here is that we use the context class loader to load extension
classes if it is available. Assuming it is available, the failure of
the context class loader to load a particular class
(ClassNotFoundException) probably means that the class was just not
found by the context class loader. In that case, we try the system
classloader as a fallback to see if it can locate the class. This is
necessary when you have a context classloader that may not attempt to
delegate to the system classloader. So, we're basically trying all of
the reasonable classloaders that we know about to try to load an
extension class.
If, on the other hand, there is some other error, then we think it means
that the context class loader is just not a suitable classloader. In
that case, we tell XalanJ that it just shouldn't use the context class
loader anymore because it's not suitable for loading extension classes.
It doesn't mean we should report an error and stop processing. It just
means that that's not the classloader for us.
While I agree that there is a possible race condition here, it seems
harmless to me. The worst that will happen is multiple threads could
set getCCL to null at different times, each having discovered that that
context class loader is unsuitable. It doesn't seem worth it to
synchronize this method for this case.
I do agree that we should issue a better error message when we can't
load an extension class. ExtensionHandler.getClassForName() should
probably return an Object[] which indicates the classloaders that were
tried in the event that the class could not be found. This would allow
its caller to issue a more informative diagnostic. I keep trying to
find the time to do this but that just hasn't been possible lately.
Another issue is whether it's appropriate to use a single
ContextClassLoader for all extensions. I can envision a situation where
XalanJ is located in an AppServer's extension directory but the various
XalanJ extensions are located in different WAR files. I'm not sure why
you'd want to do this since I'm not sure there is a real use case for
having extensions with the same name but different functions stored in
different WAR files. If there was such a use case, it would argue
against caching a ContextClassLoader at all and would indicate that the
CCL should be looked up on every extension call. However, this is a
particularly expensive call, given that we're doing it with reflection
in order to remain JDK 1.x compatible. Unless this is causing someone
problems, I think we should leave this part the way it is.
Gary
> -----Original Message-----
> From: Simon Kitching [mailto:simon@ecnetwork.co.nz]
> Sent: Saturday, November 23, 2002 10:23 PM
> To: xalan-dev@xml.apache.org
> Subject: xalan-J:
> org.apache.xalan.extensions.ExtensionHandler: racecondition,
> exception supression [low priority]
>
>
> 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 <ma...@pingdynasty.com>.
> ----------------------------
>
> (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 <si...@ecnetwork.co.nz>
>