RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[Roger Riggs]

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