RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[vyom]

On Tuesday 29 March 2016 09:10 PM, Chris Hegarty wrote:
> On 29 Mar 2016, at 16:20, Roger Riggs <Roger.Riggs at 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) ?
>
> -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.