Hi Xuelei,
Please see inline.
On 12/02/2016 09:48 PM, Xue-Lei Fan wrote:
- Exceptions are printed out in startServer/startClient methods, it
doesn't look necessary to use suppressed exceptions and initCause()
method. What was wrong with the code in runTest() method? The
code in
runTest() method looks shorter and cleaner to me.
The main issue of runTest() is that it does not throw the original
exception. Exception tacks have more information for debugging. I
want to keep the good side of Brad's previous hardness on this point.
It is not clear what's the original exception should be because you
have
client and server. If you print all exceptions in
startServer/startClient (right after they occurred), then you don't
hide
anything helpful for debugging.
The exception dump may have more information than the single exception
track, for example the nested cause stacks.
Can't it be printed out in startServer/startClient accordingly?
Throw the right exception is important even for test case. Having the
Java handling the exception is more nature. Reading the debug log
dump is not as straightforward as read the exception at the end of
each run. Wrapping into RuntimeException is very confusing sometimes.
Comparing the following two log:
java.net.XXException: this is an exception message.
cased by A
caused by B.
and
java.lang.RuntimeException: xxxx
caused by java.net.XXException: this is an exception message.
caused by A.
(cause by B may be swallowed)
I don't like the later one, which introduce unnecessary exception
stacks and take more time for the debugging.
I am not suggesting neither #2.
I am suggesting not to use initCause because it modifies the original
exception, and not to use addSupressed() because it's not clear where
the suppressed exception came from. Instead, I am suggesting to print
all original exceptions on that sides where they occurred. I am not
suggesting to wrap anything into a runtime exception. A runtime
exception can be used just to indicate a test failure.
The original exception handling was very simple, and a lot
improvements were made so that the log is easier for debugging.
IMO, suppressed exceptions may confuse here (that's just from my
experience looking at JSSE test logs). The logic in lines 739-763
doesn't make it cleaner what exception was thrown on what side. It
does
"local.initCause(remote)", but actually "remote" is not a cause of
"local". Finally, it does "exception.addSuppressed(startException)"
where "startException" can be thrown either on server or client side
which actually depends on test logic (an engineer needs to keep it in
mind).
Would it be better to have in logs something simple like the
following?
....
This is an unexpected exception on client side:
<full stack trace>
This is an unexpected exception on server side:
<full stack trace>
Test failed.
...
Doesn't it look simpler?
The purpose of suppressed exception is fully use of the exception for
debugging. The above suggestion still lose some exception debugging
information.
What kind of information is lost if you don't use suppressed exceptions?
Deep causes of the exception, and the nature about how an java command
dump the exception. We have to research what kind of information may
be lost here. I don't want that kind of research and using the nature
of the exception is easier to me.
I don't think that anything is going to be lost if you just print an
exception right away.
Artem
"startException" can be printed out right away when it occurs which I
think would be cleaner.
I dumped the exception message (line 808/818), hopefully it can help a
little bit if confusing.
I believe this is right place to dump everything out.
;-) I don't think so.
Thanks for the review.
Xuelei
It just needs to
make sure that it exceptions traces from server/client sides don't mix
up, so may be it's better to add some synchronization on System.out like
it was done in print() method.
It's good suggestion that we'd better to tell which side (the client
or server side) throws the exception. I will think more about it in
the next webrev update.
Sure, thank you.
Artem
