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