Code Review Request JDK-8161106 Improve SSLSocket test template

[Xuelei Fan]

Thanks for the feedback, Artem.  Here is the updated webrev per your
suggestions:

    http://cr.openjdk.java.net/~xuelei/8161106/webrev.02/

On 7/13/2016 1:03 AM, Artem Smotrakov wrote:
> Hi Xuelei,
> 
> I am not an official reviewer, but I have a couple of comments.
> 
> 1. line 149: would it be better to check this condition in a loop?
> 
clientCondition.await() will wait for 30 seconds.  Need no loop any
more.  If adding a loop, it may become a potential deadloop and cause
timeout issue.

> 2. Using try-with-resources blocks might simplify doServerSide() a
> little bit (no need to call close() on sockets, and a couple of "try"
> blocks might be probably removed)
> 
I considered to use try-with-resources.  But as the lock is introduced.
and we want to support socket accept timeout, it more convenient to me
to use the traditional try-finally clauses.

> 3. Minor: it might be better to define timeout values as constants with
> meaningful names.
> 
Maybe.  Normally, if a number is used only once, I may not define a
field for it.

> 4. Minor: lines 268-273 seem to work, but I am wondering if it would be
> better to use File.separator instead of "/" (someone may run the test on
> Windows, but without Cygwin).
> 
I think JDK file I/O would take care of this file separator
interoperability.  It's good to use File.separator, but as the path may
contains a few separator in test case following this template, for example:

    pathToStores = "../../../../../javax/net/ssl";

Using File.separator would not make nice line.

> 5. SSLSocketSample() constructor, startServer() and startClient() methods:
> 
> Recently we've had a couple of test issue where no exception was printed
> on client/server sides. It happened because exceptions were caught and
> stored, but then tests waited infinitely for another thread to finish.
> Then, jtreg just killed the tests. As a result, no exception was printed
> out. It might be better to simplify this code to print out an exception
> right after it's caught, and then throw a runtime exception if
> serverException or clientException are not null. Probably there is a
> side effect that logs may be a little messy sometimes because of lack of
> synchronization, but maybe something like synchronized(System.out) may
> help.
> 
This update should be able to avoid the infinitely waiting.  Updated to
print the exception message right after it is caught.

> 6. Minor: it might be better to make class constants uppercase
> 
> http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-135099.html#367
> 
Good to use uppercase.  I plan to modify this template to a overridable
one so that test case can extend this template.  By then, those fields
will not be declared as constants any more.  Let's use the current names
for a while.

Thanks,
Xuelei

> 
> Artem
> 
> On 07/10/2016 10:16 PM, Xuelei Fan wrote:
>> There is a nice catch of the timeout miss-match during the handling of
>> serverIsReady in doClientSide().   Here is the updated webrev:
>>
>>      http://cr.openjdk.java.net/~xuelei/8161106/webrev.01/
>>
>> Thanks,
>> Xuelei
>>
>> On 7/11/2016 11:44 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Please review this enhancement of SSLSocket test template:
>>>      http://cr.openjdk.java.net/~xuelei/8161106/webrev.00/
>>>
>>> There are some known issues with the current SSLSocket test template
>>> (test/javax/net/ssl/templates/SSLSocketTemplate.java) in the automation
>>> testing environment:
>>> 1. the client or server can be blocked if the peer run into problems, as
>>> may result in intermittent timeout failure.
>>> 2. the server accepted connection may be not linked to the expected
>>> client. For example, some other test case may try to use and connect to
>>> a free server socket port, unfortunately this port can be actually
>>> opened by the server of SSLSocketTemplate because there is no
>>> synchronization about the free socket port between test cases.  It's OK
>>> to run the test singly, but may result in weird intermittent failure in
>>> the automation testing environment.
>>>
>>> The new test template in this update considers the noises above.
>>>
>>> Thanks,
>>> Xuelei
>>>
>