Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

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
>>