RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(String path) handle "null" incorrectly

Chris Hegarty chris.hegarty at oracle.com
Tue Jul 12 14:25:22 UTC 2016 

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.fuchs at 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
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>

More information about the net-dev mailing list