Hi, Chris H. Thanks for your review and comments, I just added a comment as you suggested and update new webrev as below, please have a review.
http://cr.openjdk.java.net/~xyin/8187522/webrev.01/ Thanks, Chris Y. > On 11 Oct 2018, at 7:00 PM, Chris Hegarty <chris.hega...@oracle.com> wrote: > > Chris Y, > >> On 11 Oct 2018, at 09:45, Chris Yin <xu.y....@oracle.com >> <mailto:xu.y....@oracle.com>> 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) >> >> 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/> > > I think this is good. > > The `dash` character in FtpCommandHandler is subtle. I kinda > regret allowing that change, rather than creating a separate > standalone test for 8151586. It just confusing and overloads > the test infra. It might ok at this point, but a comment would > be good. > > -Chris. >
