RE: xalan-J: org.apache.xalan.extensions.ExtensionHandler: racecondition, exception supression [low priority]

[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