On 29 Mar 2016, at 17:38, vyom <vyom.tew...@oracle.com> wrote: > On Tuesday 29 March 2016 09:10 PM, Chris Hegarty wrote: >> On 29 Mar 2016, at 16:20, Roger Riggs <roger.ri...@oracle.com> wrote: >> >>> Looks good, >> +1 >> >> Does the test need to be run in othervm mode ? > I don't think othervm mode required, do you wants to me to generate another > webrev(0.5) ?
Not necessary. I will remove it ( build and test ) before committing your changes. Just checking that there was not some specific reason you added it. -Chris. >> >> -Chris. >> >>> Thanks, Roger >>> >>> >>> On 3/29/2016 11:00 AM, vyom wrote: >>>> Hi, >>>> >>>> Please find the updated webrev. >>>> http://cr.openjdk.java.net/~vtewari/7167293/webrev0.4/index.html >>>> <http://cr.openjdk.java.net/%7Evtewari/7167293/webrev0.4/index.html> >>>> >>>> Thanks, >>>> Vyom >>>> >>>> On Tuesday 29 March 2016 07:49 PM, Roger Riggs wrote: >>>>> Hi Vyom, >>>>> >>>>> A minor cleanup to reduce the size of the little used code. Create the >>>>> FNFE before checking and closing. >>>>> Then the form of the cleanup code will be consistent and there will be >>>>> less code. >>>>> For example, >>>>> >>>>> + FileNotFoundException fnfe = new >>>>> FileNotFoundException(fullpath); >>>>> + if (ftp != null) { >>>>> + try { >>>>> + ftp.close(); >>>>> + } catch (IOException ioe) { >>>>> + fnfe.addSuppressed(ioe); >>>>> + } >>>>> + } >>>>> + throw fnfe; >>>>> >>>>> Thanks, Roger >>>>> >>>>> >>>>> >>>>> On 3/29/2016 9:03 AM, vyom wrote: >>>>>> please find that updated >>>>>> webrev(http://cr.openjdk.java.net/~vtewari/7167293/webrev0.3 >>>>>> <http://cr.openjdk.java.net/%7Evtewari/7167293/webrev0.3>), I >>>>>> incorporated Roger's comments. >>>>>> Thanks, >>>>>> Vyom >>>>>> >>>>>> >>>>>> On Wednesday 23 March 2016 10:04 PM, Roger Riggs wrote: >>>>>>> Hi Vyom, >>>>>>> >>>>>>> I think it should be the case that the same exception (FrpProtocol, >>>>>>> IOException) should >>>>>>> be rethrown instead of the exception that resulted from the call to >>>>>>> close. >>>>>>> >>>>>>> In many case, the exceptions from a call to a cleanup close() are >>>>>>> simply ignored >>>>>>> but the alternative is to add them as suppressed exceptions to the >>>>>>> original fe or ex. >>>>>>> I would code the new blocks as ignoring the IOException on close and >>>>>>> falling through >>>>>>> to throw the same exception as previously. >>>>>>> For example, >>>>>>> >>>>>>> } catch (FtpProtocolException fe) { >>>>>>> + if (ftp != null) { >>>>>>> + try { >>>>>>> + ftp.close(); >>>>>>> + } catch (IOException ioe) { >>>>>>> + // ignore; alternatively fe.addSuppressed(ioe); >>>>>>> + } >>>>>>> + } >>>>>>> throw new IOException(fe); >>>>>>> >>>>>>> Roger >>>>>>> >>>>>>> >>>>>>> On 3/21/2016 5:19 AM, vyom wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> Please find the updated webrev, got some off line comment from Chris. >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~vtewari/7167293/webrev0.2/index.html >>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/7167293/webrev0.2/index.html> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Vyom >>>>>>>> >>>>>>>> On Thursday 17 March 2016 12:30 PM, vyom wrote: >>>>>>>>> please find the updated >>>>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/7167293/webrev0.1/index.html >>>>>>>>> >>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/7167293/webrev0.1/index.html>). >>>>>>>>> Thanks, >>>>>>>>> Vyom >>>>>>>>> >>>>>>>>> On Tuesday 15 March 2016 05:11 PM, Chris Hegarty wrote: >>>>>>>>>> Vyom, >>>>>>>>>> >>>>>>>>>> On 15/03/16 10:00, vyom wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> Please review the below fix. >>>>>>>>>>> >>>>>>>>>>> Bug: JDK-7167293 : FtpURLConnection connection leak on >>>>>>>>>>> FileNotFoundException >>>>>>>>>>> Webrev: http://cr.openjdk.java.net/~rgoel/~vyom/7167293/webrev0.0/ >>>>>>>>>>> <http://cr.openjdk.java.net/%7Ergoel/%7Evyom/7167293/webrev0.0/> >>>>>>>>>> You have the same lines of code in a number of places. Does >>>>>>>>>> a private static helper method make sense for this? >>>>>>>>>> >>>>>>>>>> Test tries to connect to an external resource, which is not >>>>>>>>>> reachable on my, and many, systems. Can the test setup a simple >>>>>>>>>> ServerSocket to do something minimal? >>>>>>>>>> >>>>>>>>>> -Chris. >
