Hi Weijun,
Please review this update. Per you suggestion, I updated to use
CountDownLatch for the synchronization between client and server.
CountDownLatch is more simple than ReentrantLock in the context.
http://cr.openjdk.java.net/~xuelei/8161106/webrev.03/
Thanks,
Xuelei
On 7/14/2016 1:44 AM, Artem Smotrakov wrote:
> 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
>>>>>
>
