Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

Hi Xuelei,

Please see inline.

On 12/02/2016 03:53 PM, Xue-Lei Fan wrote:
>> Let's whether Sean or Weijun can have free cycle for the review of 
>> this part.
Yeah, that would be great.
>
>> - 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?
>
>> It also might make sense to make
>> createSSLContext() a part of public API which I think would make the
>> template more flexible.
> We have had the protected createClient/ServerSSLContext() methods.
Making createSSLContext() accessible doesn't seem to be worse to me. It 
may be final if you don't want anyone to override it for some reason.
>
>> - 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.

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?
>
>> - lines 114-123, this code is used quite often by tests, why don't we
>> keep it in SSLSocketTemplate?
>>
> Good point!  I don't like it in SSLSocketTemplate as it is used for 
> HTTP connections only, but it may be worthy to have a sub-template for 
> HTTPS client testing.  What do you think if we address this 
> enhancement in a new bug?
I am fine with it. My point is that SSLSocketTemplate may contain some 
useful static methods like this (to avoid duplicate code).

Artem