RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[vyom]

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