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