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

Chris Hegarty chris.hegarty at oracle.com
Wed Jul 13 15:17:33 UTC 2016 

+1

-Chris.

On 13/07/16 14:40, Daniel Fuchs wrote:
> 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