Hi, Vyom, Chris H. > On 11 Oct 2018, at 7:08 PM, Chris Hegarty <chris.hega...@oracle.com> wrote: > > > >> On 11 Oct 2018, at 12:02, vyom tewari <vyom.tew...@oracle.com >> <mailto:vyom.tew...@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 >> >
