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

Daniel Fuchs daniel.fuchs at oracle.com
Wed Jul 13 13:40:29 UTC 2016 

On 13/07/16 13:14, Svetlana Nikandrova wrote:
> Chris, Daniel,
>
> thank you for your comments.
> Please see updated review:
> http://cr.openjdk.java.net/~snikandrova/8022580/webrev.04/
> <http://cr.openjdk.java.net/%7Esnikandrova/8022580/webrev.04/>

Hi Svetlana,

The test looks good to me know :-)

best regards,

-- daniel

>
> Thank you,
> Svetlana
>
> On 12.07.2016 17:25, Chris Hegarty wrote:
>> 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