Code Review Request JDK-8161106 Improve SSLSocket test template

[Artem Smotrakov]

Hi Xuelei,

The webrev looks good to me. Please see inline.

On 07/12/2016 10:36 PM, Xuelei Fan wrote:
> 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.
Right, I didn't get it when I read it first time.
>> 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.
Agree. Using try-with resources would remove one try-finally block, but 
probably it would introduce nested "try" blocks.
>> 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.
Sure.
>
>> 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.
Okay. I am not an expert in Java I/O, so I am not sure. Agree, 
File.separator wouldn't make it nicer.
>> 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.
Thanks. It might be better to print whole stack trace in 
srartServer/startClient
>> 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.
Sure.

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