You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@xalan.apache.org by Sc...@lotus.com on 2001/03/28 06:49:55 UTC
Re: XalanJ2 BUG - quiet swallowing of exceptions - with proposed fix
attached
Patrick, once again sorry for the long lag time in responding to this. My
comments here are a bit rambling as I tried to work through the issues.
You might try doing a refresh from the CVS in the morning and trying your
test case again to see how it works now. The bottom line is I may have
found a better solution than you proposed in your patch. (though I'm not
totally confident...)
All: feel free to give good ideas about more robust and coherent exception
handling mechanisms and strategies. Or flame me on my bad ideas.
> What is also odd about this is that in every place in
> TransformerImpl, m_reportInPostExceptionFromThread is never true. Is this
> old code that can be discarded?
In my code all m_reportInPostExceptionFromThread references are commented,
so I guess I already did this.
> Then there is the later comment " // should have already been reported
via
> the error handler?".
I think I meant javax.xml.transform.ErrorListener, the default of which is
org.apache.xml.utils.DefaultErrorHandler, which will print the line and
file to System.out (should be System.err... not sure why it isn't). But
not the error message, interestingly.
> Finally, to really add to the confusion, there is the
> way the exception is treated in the synchronized block where if the
> exception's message is null then the exception is printed.
Not sure what that's about. Looks like an act of debugging desperation on
my part. Nice of me to leave a comment, huh? I'm awfully tempted to
remove it altogether (i.e. the whole synch block).
I commented out everything in that block except notifyAll(), which seems
harmless and probably safer to leave in, I guess.
I did some work with error handling a couple of weeks ago also in relation
to some errors that were happening from Cocoon, which may make some error
throws more robust.
> if ( this.m_exceptionThrown instanceof RuntimeException
> || this.m_exceptionThrown instanceof Error ) {
> // these exceptions are not printed until the very top-level run
method
> // in TransformerImpl.run()
> e.printStackTrace();
> } else {
> // all others should have already been reported via the error
handler?
> }
I'm a little confused because your patch is somewhat old compared to the
source I'm using (I'm too lazy to go dig up the version that it was made
against), but I'm not sure what this is about. Printing the stack trace
from this method really is probably a bad thing, yes? As long as it is
reported from the main thread? hmm...
As far as I can tell, any exception thrown from a result ContentHandler
should be caught in the run method, and caught in transform(Source source).
If this isn't happening, I need to understand why.
Things get nastier if the transform is being driven by input ContentHandler
events... i.e. transform(Source source) isn't involved. This may be what
you are experienceing???
In that case org.apache.xalan.stree.Child#throwParseError(Exception e)
*may* throw a WrappedRuntimeException. Or, the main thread may already be
in endDocument() in the transformThread.join(); statement, in which case,
the error will be dropped. hmmm.... added:
// This should wait until the transformThread is considered not
alive.
transformThread.join();
if(!m_transformer.hasTransformThreadErrorCatcher())
{
Exception e = m_transformer.getExceptionThrown();
if(null != e)
throw new org.xml.sax.SAXException(e);
}
m_transformer.setTransformThread(null);
...which should help a lot. Don't know why I've never done this before.
We really need to work on error handling tests. Nasty business that.
transform-method --> main-parser-thread --> transform-thread -->
ContentHandler <-- error!
ContentHandler --> main-parser-thread --> transform-thread -->
ContentHandler <-- error!
ContentHandler --> main-parser-thread --> transform-thread -->
ContentHandler --> secondary-parser-thread --> transform-thread -->
ContentHandler <-- error!
ContentHandler --> main-parser-thread --> transform-thread -->
user-extension <-- error!
Creating meaningful regression tests for these is quite a challenge.
> On a minor note: I removed some catches that simply rethrew the exception
Thanks. I don't know what these were about (probably debugging leftovers),
but they're removed.
-scott
Patrick Moore
<patrickm@rio To: "'xalan-dev@xml.apache.org'" <xa...@xml.apache.org>
port.com> cc: (bcc: Scott Boag/CAM/Lotus)
Subject: XalanJ2 BUG - quiet swallowing of exceptions - with proposed
02/23/2001 fix attached
09:17 PM
Please
respond to
xalan-dev
Hi there --
I have found some bad behavior.
Repro:
1. Throw a RuntimeException from a ContentHandler method (I noticed this
problem while in startElement() ).
2. Make sure this exception has a message.
3. This exception will never get printed and the program will quietly die.
Now here is the puzzling part of the bug. The code in
TransformerImpl.postExceptionFromThread(Exception e) [lines2829:2871] is
very puzzling. First, there is the "Nicola Brown" comment about there being
some sort of problem. What is also odd about this is that in every place in
TransformerImpl, m_reportInPostExceptionFromThread is never true. Is this
old code that can be discarded?
Then there is the later comment " // should have already been reported via
the error handler?". Finally, to really add to the confusion, there is the
way the exception is treated in the synchronized block where if the
exception's message is null then the exception is printed.
I have included a proposed fix. But someone more in the know needs to
examine this and see 1) if the "Nicola Brown" reference is still valid;
and/or 2) should all exception be printed. Note too that this change means
that a checked exception with no message will no longer be printed which I
am not at all certain is correct.
On a minor note: I removed some catches that simply rethrew the exception
-Patrick Moore-
excerpt:
void postExceptionFromThread(Exception e)
{
// Commented out in response to problem reported by Nicola Brown
<Ni...@jacobsrimell.com>
// if(m_reportInPostExceptionFromThread)
// {
// // Consider re-throwing the exception if this flag is set.
// e.printStackTrace();
// }
if (m_inputContentHandler instanceof SourceTreeHandler)
{
SourceTreeHandler sth = (SourceTreeHandler)
m_inputContentHandler;
sth.setExceptionThrown(e);
}
ContentHandler ch = getContentHandler();
if(ch instanceof SourceTreeHandler)
{
SourceTreeHandler sth = (SourceTreeHandler) ch;
((TransformerImpl)(sth.getTransformer())).postExceptionFromThread(e);
}
m_isTransformDone = true;
m_exceptionThrown = e;
; // should have already been reported via the error handler?
synchronized (this)
{
String msg = e.getMessage();
// System.out.println(e.getMessage());
notifyAll();
if (null == msg)
{
// m_throwNewError = false;
e.printStackTrace();
}
// throw new org.apache.xml.utils.WrappedRuntimeException(e);
}
}
(See attached file: TransformerImpl.java.fix)(See attached file:
ElemForEach.java.fix)(See attached file: ElemLiteralResult.java.fix)