RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[Chris Hegarty]

On 29 Mar 2016, at 17:38, vyom <vyom.tewari at oracle.com> wrote:

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

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