RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[Chris Hegarty]

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 ?

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