[12] RFR 8187522: sun/net/ftp/FtpURLConnectionLeak.Java timed out

[Chris Yin]

Hi, Vyom, Chris H.

> On 11 Oct 2018, at 7:08 PM, Chris Hegarty <chris.hegarty at oracle.com> wrote:
> 
> 
> 
>> On 11 Oct 2018, at 12:02, vyom tewari <vyom.tewari at oracle.com <mailto:vyom.tewari at oracle.com>> wrote:
>> On Thursday 11 October 2018 02:15 PM, Chris Yin wrote:
>>> Please review below small change to fix test sun/net/ftp/FtpURLConnectionLeak.Java, thanks
>>> 
>>> Besides the original timeout issue, looks like the test not working as expected even the results is pass. Per test description, we expect FileNotFoundException and then verify connection closed, but look through recent test run log, it’s actually got “sun.net.ftp.FtpLoginException: Invalid username/password” and this exception been caught in IOException block coincidentally, so the test result is pass, but it never hit FileNotFoundException. The fix change will remove IOException catch block and close server socket in close() of try-with-resouce to avoid possible invalid exception catch and connection issue, also ftp 220 response message was modified to allow FtpClient working correctly (I leave the corrupted message style as it is since looks like it will be used to test JDK-8151586)
>> you are right, corrupted message style is intentional. Your code changes looks good to me but i am not able to understand "FtpCommandHandler.java" changes.
>> 
>>  I can see that you inserted one "-" , is it intentional ?

Thanks for reviewing and checking this, yep, the ‘-‘ char is intentional, as Chris H. also explained below, we need it to make sure ftp communication not been broken, otherwise tests like FtpURLConnection will fall in “sun.net.ftp.FtpLoginException” unexpectedly

> 
> If the 4th char is a dash, then the implementation ( for
> what it’s worth ) thinks that it has seen multi-line reply
> `###-` sequence.
> 
> It needs a comment, or maybe revert prior to 8151586.
> And a separate specific test added for 8151586.

Thanks for your suggestion, currently, I just added a comment to explain the 220 response message and sent updated webrev in earlier thread, I may prefer to process in that way for now if no objection since separate specific test added for 8151586 is already out of scope of current bug :). We may have another bug/enhancement to track it if necessary. Thanks

Regards,
Chris Y.

> 
> -Chris.
> 
> 
> 
>> Thanks,
>> Vyom
>>> 
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8187522 <https://bugs.openjdk.java.net/browse/JDK-8187522>
>>> webrev: http://cr.openjdk.java.net/~xyin/8187522/webrev.00/ <http://cr.openjdk.java.net/~xyin/8187522/webrev.00/>
>>> 
>>> Regards,
>>> Chris
>> 
> 

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