RFR 8022580: sun.net.ftp.impl.FtpClient.nameList(null)

[Artem Smotrakov]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160706/12cc5133/attachment-0001.html>