Svetlana, >> <http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.03/>
The source code changes look fine. On the test... > On 11 Jul 2016, at 14:20, Daniel Fuchs <daniel.fu...@oracle.com> wrote: > > Hi Svetlana, > > Have you thought of using a CountDownLatch (or some other > synchronization mechanism like e.g. Semaphore) to wait for > the server to be ready instead of: > 55 int port = 0; > 56 while (port == 0) { > 57 Thread.sleep(500); > 58 port = server.getPort(); > 59 } > Also serverSocket needs to be volatile. Please remove this. It is unnecessary. > Or better yet, you could bind the server socket in > the constructor of FtpServer - which would allow you > to make serverSocket final instead of volatile, Exactly. That is the preferred style for these kind of tests. > and then > you would no longer need to make FtpServer extend Thread. You still need to execute the server handling code in another thread, but FtpServer could be just a Runnable. Additionally, - No need to catch any Exceptions in main, just declare that main throws Exception - with the changes above, ‘client' may be able to move into the try-with-resources block. -Chris. > best regards, > > -- daniel > > On 07/07/16 19:06, Svetlana Nikandrova wrote: >> Artem, >> >> please see updated review: >> http://cr.openjdk.java.net/~snikandrova/8022580/webrev.03/ >> <http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.03/> >> >> Thanks, >> Svetlana >> >> On 07.07.2016 19:41, Artem Smotrakov wrote: >>> Hi Svetlana, >>> >>> Thank you for addressing the comments below. The test looks fine to >>> me. Just a couple of minor comments. >>> >>> 1. You don't really need fields in lines 77-79. >>> >>> 2. try-catch block in line 86 is not really necessary as well. >>> >>> It would be better to update bug subject to something more meaningful. >>> >>> Artem >>> >>> On 07/07/2016 08:31 AM, Svetlana Nikandrova wrote: >>>> Artem, >>>> >>>> thank you for suggested test improvements. Here is updated webrev: >>>> http://cr.openjdk.java.net/~snikandrova/8022580/webrev.02/ >>>> <http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.02/> >>>> >>>> Chris, >>>> >>>> seem like you have already looked into that issue before. May be you >>>> can find some time to review this fix? >>>> >>>> Thank you, >>>> Svetlana >>>> >>>> On 06.07.2016 20:35, Artem Smotrakov wrote: >>>>> Hi Svetlana, >>>>> >>>>> Thanks for addressing my comments. Could you please take a look at a >>>>> couple of comments about TestFtpClientNameListWithNull.java below? >>>>> >>>>> 1. lines 64-70: should "client" be closed in "finally" block? I also >>>>> think it might be better to re-throw IOExceptions instead of >>>>> ignoring them (it may hide some problems). You can just add "throws" >>>>> to main() method. >>>>> >>>>> 2. line 107: you close "out" in a couple of places in handelClient() >>>>> method, it might be better to add a "finally" block, and shutdown >>>>> everything there. >>>>> >>>>> 3. lines 176-182: you can close a client socket in handelClient(). >>>>> Since it's a test for JDK 9, you can use try-with-resources like the >>>>> following: >>>>> >>>>> public void handelClient(Socket cl) { >>>>> ... >>>>> try (cl) { >>>>> ... >>>>> } >>>>> .... >>>>> >>>>> (please note that it doesn't work for JDK 8) >>>>> >>>>> It would reduce the code and simplify the test (you would avoid some >>>>> "try" blocks). >>>>> >>>>> I am not sure that you need to close "out" if you close the socket. >>>>> >>>>> 4. Typo: handelClient -> handleClient >>>>> >>>>> Artem >>>>> >>>>> On 07/06/2016 09:11 AM, Svetlana Nikandrova wrote: >>>>>> Hi Artem, >>>>>> >>>>>> thank you for your replay. I believe I addressed all your comments. >>>>>> Please see updated diff: >>>>>> http://cr.openjdk.java.net/~snikandrova/8022580/webrev.01/ >>>>>> <http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.01/> >>>>>> >>>>>> Thank you, >>>>>> Svetlana >>>>>> >>>>>> On 05.07.2016 20:54, Artem Smotrakov wrote: >>>>>>> Hi Svetlana, >>>>>>> >>>>>>> I am not an official reviewer, but I have a couple comments below. >>>>>>> >>>>>>> TestFtpClientNameListWithNull.java: >>>>>>> >>>>>>> 1. Shouldn't "commandHasArgs" be volatile? Looks like it may cause >>>>>>> intermittent failures. As a sanity check, you may want to run the >>>>>>> test in a loop to make sure it is stable. >>>>>>> >>>>>>> 2. You may want to make FtpServer implement AutoCloseable >>>>>>> (terminate() method just becomes close()). Then, you can use it in >>>>>>> try-with-resource block which would simplify the code (you can >>>>>>> avoid a couple of nested try-catch blocks). >>>>>>> >>>>>>> By the way, it might be good to make sun.net.ftp.FtpClient >>>>>>> implement AutoCloseable as well, but probably it should be a >>>>>>> separate enhancement. >>>>>>> >>>>>>> 3. line 62-63, 66: should it be in "finally" block? >>>>>>> >>>>>>> 4. How many connection is FtpServer supposed to handle in this >>>>>>> test? If only one, it might be better to remove the "while" loop >>>>>>> and "done" variable to simplify the code. >>>>>>> >>>>>>> 5. Is it necessary to handle a client connections in a separate >>>>>>> thread? Avoiding too many threads would simplify the test. >>>>>>> >>>>>>> 6. The test ignores a couple of IOException, it might be better to >>>>>>> fail it they occur. >>>>>>> >>>>>>> 7. Why is it necessary to use daemons? >>>>>>> >>>>>>> 8. Please use braces for "if" statements, see Java Coding Conventions. >>>>>>> >>>>>>> FtpClient.java: please update copyright year. >>>>>>> >>>>>>> Artem >>>>>>> >>>>>>> On 07/05/2016 07:40 AM, Svetlana Nikandrova wrote: >>>>>>>> Hello, >>>>>>>> >>>>>>>> please review this fix for bug >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8022580 >>>>>>>> Webrev: >>>>>>>> http://cr.openjdk.java.net/~msolovie/8022580/webrev.00/ >>>>>>>> <http://cr.openjdk.java.net/%7Emsolovie/8022580/webrev.00/> >>>>>>>> >>>>>>>> Description: >>>>>>>> There is no handling for null path parameter in the method >>>>>>>> sun.net.ftp.impl.FtpClient#nameList(), while javadoc says that >>>>>>>> "@param path a String containing the pathname of the directory to >>>>>>>> list or null for the current working directory". Changeset adds >>>>>>>> check that if param is null NLST will be sent without parameter >>>>>>>> (no-parameter default is current directory). >>>>>>>> >>>>>>>> JPRT tested. >>>>>>>> >>>>>>>> Thank you, >>>>>>>> Svetlana >>>>>>> >>>>>> >>>>> >>>> >>> >> >