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