Code Review Request JDK-8170329 New SSLSocket testing template

[Xue-Lei Fan]

On 12/2/2016 5:53 PM, Artem Smotrakov wrote:
> Please see inline
>
>
> On 12/02/2016 05:25 PM, Xue-Lei Fan wrote:
>>>>
>>>>> - Why did you remove Peer and Application interfaces? I think those
>>>>> interfaces make SSLSocketTemplate more flexible since it allows
>>>>> override
>>>>> doServerSide/doClientSide logic if necessary - it doesn't seem to be
>>>>> worse. If there is no strong reason to remove those interfaces, I
>>>>> would
>>>>> prefer to keep them with default static/stateless
>>>>> doServerSide/doClientSide versions.
>>>> I agree with you that The Peer and Application interfaces are more
>>>> flexible.  I have no strong reason to remove them.  For this update, I
>>>> focus more on simple and minimal sub-classes.  The Peer and
>>>> Application interfaces need some complicated coding (condition,
>>>> threading, etc) in sub-classes, so the design does not fall into the
>>>> scope.  I may prefer to remove them at this update and see what
>>>> happens if we moving forward with more test update. We can add them
>>>> back or create a more flexible one if we need the flexibility in the
>>>> future.
>>> We have a lot of JSSE tests which may need to do something else in
>>> doServerSide() and doClientSide() methods (that's what I learned from my
>>> experience working with JSSE tests). Peer and Application interfaces
>>> basically allow to override doServerSide() and doClientSide() methods.
>>> Your changes make doServerSide() and doClientSide() methods private, so
>>> that test can't add something inside it.
>>>
>>> If you think those interfaces are helpful, I think it's better to keep
>>> Peer and Application interfaces instead of removing/adding them back and
>>> forth.
>>>
>>> Another option may be providing methods which are called in
>>> doServerSide() and doClientSide(). Those methods may be overridden by a
>>> test if it needs to do extra stuff inside doServerSide() and
>>> doClientSide().
>>>
>>> I am fine with both ways, so it's up to you. But I would like to make it
>>> clear what way we want to follow.
>>>
>>> What do you think?
>> You points above are good.  When I made the update, I struggled a lot
>> about the balance of flexible and simplicity.  I was inclined to
>> simplicity at last as we can update the template if any new feature is
>> required in the future.  I'm not sure how much this template can be used,
> I suspect that all JSSE tests should be updated to use this template to
> avoid intermittent failures which we have been seeing.
IHMO, I was not that optimistic.

>> exposing as less methods as possible will give us more flexible to
>> make further update.  So I made most of the methods private, and
>> removed a lot of methods.  Let's update the access scope if we really
>> need them in the future.  A small public footprint is easier to start.
> We already started a while ago :) Those methods which you removed were
> actually used by other tests which you updated.
No, none of them are used any more.

> My point is that we'd
> better understand how we want the tests to be organized to avoid
> adding/removing stuff back and forth.
Sure, it's the job of the update.  I see no need of those methods any 
more unless there are really needed in the future.

> Again, I believe that lots of
> tests should be updated to use this template. I am not suggesting to
> predict everything that the tests may need right away. If you think that
> Peer and Application interfaces are not good enough, it's fine to me,
> but we need to understand what would be a replacement for them
I think we don't need those methods at present.  We may need some of 
them in the future, but I see no strong reason to keep them at present.

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

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

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