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
