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:[EMAIL PROTECTED]] > Sent: Saturday, November 23, 2002 10:23 PM > To: [EMAIL PROTECTED] > 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 <[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]> >
