RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[Roger Riggs]

Looks good,

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