Re: RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[Chris Hegarty]

On 29 Mar 2016, at 17:38, vyom  wrote:

> On Tuesday 29 March 2016 09:10 PM, Chris Hegarty wrote:
>> On 29 Mar 2016, at 16:20, Roger Riggs  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 
 
 
 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 
>> ), 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 
 
 
 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
>  
> ).
> 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/
>>> 
>> 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.
> 

Re: RFR JDK-8087113: Websocket API and implementation

[Andrej Golovnin]

Hi Pavel,

>> 215 .append(" 
>> rem=").append(b.remaining()).append("]").toString();
>> 
>> Please use ']' instead of "]”.
>> 
>> 222 
>> .append("[len=").append(s.length()).append("]").toString();
>> 
>> Please use ']' instead of "]”.
> 
> I believe since JEP 280 [1] is already here, we don't need to bother about 
> this
> kind of things any more (generally speaking).

Static code analyzers produce a warning for code like this.

Btw. in line 212

 212 return new StringBuffer()

you use StringBuffer instead of StringBuilder.

JEP-280 does not apply in this case. But when you rewrite this code to
use the plain String concatenation, then JEP-280 would apply:

 211 static String toString(Buffer b) {
 212 return toStringSimple(b)
 214+ "[pos=“ + b.position()
 215+ " lim=“ + b.limit()
 216+ " cap=“ + b.capacity()
 217+ " rem=“ + b.remaining() + "]";
 218 }

This code is easier to read than the one with StringBuilder, at least for me.

>> 177 throw new 
>> IllegalArgumentException(String.valueOf(n));
>> 
>> Could you please provide a better message for exception here?
> 
> What would the purpose be? It's an internal implementation detail. If this
> exception is thrown, it means it's a bug in the implementation. And I highly
> doubt this error message would be of any help to an end-user. It's not 
> something
> they can influence/change (most likely).
> What's important here is the stack-trace and te value. Maybe I will include 
> more
> state to be printed.

A good error message is gold worth. It may save you hours of debugging.
Therefore if you can add more information to the exception message, then please
do it as it will help you to find and to fix bugs faster.

> 
>> src/java.httpclient/share/classes/java/net/http/SignalHandler.java
>> 
>> Maybe the lines 33-46 should be converted to a JavaDocs comment.
>> 
>> 112 switch (s) {
>> 113 case RUNNING:
>> 114 return RERUN;
>> 115 case DONE:
>> 116 return RUNNING;
>> 117 case RERUN:
>> 118 return RERUN;
>> 119 default:
>> 120 throw new InternalError(String.valueOf(s));
>> 121 }
>> 
>> Please fix the indentation.
> 
> I've tried to adhere to this:
> 
>   http://cr.openjdk.java.net/~alundblad/styleguide/#toc-indentation
> 
> Might revisit later. I will wait for the status change of this document.

Btw. have you considered to use an array instead of switch-statement?
The array may look like this:

private static final int[] NEXT_STATE = new int[] {
RUNNING, // DONE -> RUNNING
RERUN, // RUNNING -> RERUN
RERUN, // RERUN -> RERUN
};

And then the line 111 can be written as:

111 int prev = state.getAndUpdate(s -> NEXT_STATE[s]);

The only advantage is that it produces less byte codes.
But maybe it is still worth.

> 
>> src/java.httpclient/share/classes/java/net/http/WebSocket.java
>> 
>> Could you please add an #of-method to CloseCode which also takes
>> a description as parameter, e.g.:
> 
> I thought about it. But then we might have an ambiguity. One might create
> different versions of the object with the same numerical code, but different
> descriptions. Should they be equal?

Yes.

> If they are gonna be cached, then which one?

CloseCodes created by #of-method should never be cached by JDK.

> Overall, it might kill this class' property of being value-based.
> 
> If a non-standard Close Code would be created, it would be created by the
> client, rather than by the server. So a client already knows what the code
> means.

My main motivation to have a description for custom CloseCodes is
only to simplify debugging. So when you think it is overkill and not needed,
then it is OK for me to leave the code for now as is.

I have already started to port our code to use the new API and I have one 
comment.
I think you should introduce a new class CloseReason as it was done in JSR 356
(http://docs.oracle.com/javaee/7/api/javax/websocket/CloseReason.html)
and use it in #sendClose and #onClose-methods. The #onClose-method in the
WebSocket.Listener interface looks really ugly:

onClose(WebSocket webSocket, Optional code, String reason)

The parameter “reason” makes only sense when code.isPresent() returns true.
The code and the reason belong together. But the one is wrapped in Optional and 
the other not.
It doesn’t feel right. But when you introduce the new class CloseReason, then
you can change this method to:

onClose(WebSocket webSocket, Optional closeReason)

I think it makes the API much better and it is clear that the reason phrase is 
only
available when you have a close code too.

And I think there should be a way to configure a timeout for sending messages.

One minor thing off-topic:
While changing my code I noticed that the class 
ProxySelector$StaticPro

Re: RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[vyom]

On Tuesday 29 March 2016 09:10 PM, Chris Hegarty wrote:

On 29 Mar 2016, at 16:20, Roger Riggs  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) ?


-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 


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


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


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.

Re: RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[Chris Hegarty]

On 29 Mar 2016, at 16:20, Roger Riggs  wrote:

> Looks good,

+1 

Does the test need to be run in othervm mode ?

-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 
>> 
>> 
>> 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 
 ), 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 
>> 
>> 
>> 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 
>>> ). 
>>> 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/
> 
 
 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.
>>> 
>> 
> 
 
>>> 
>> 
> 

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 



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



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


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/



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.

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



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



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


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/



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.

8150234: Windows 10 App Containers disallow access to ICMP calls

[Rob McKenna]

Hi folks,

Looking for a review for this change.

Basically https://bugs.openjdk.java.net/browse/JDK-8135305 abandoned the old 
TCP echo isReachable check in favour of Windows' ICMP calls on supported 
platforms. Unfortunately it turns out that Windows 10's new App Containers 
don't actually allow access to these calls. This fix adds a check to see if 
access to those calls is allowed on those supported platforms and if not, falls 
back to the old TCP method.

http://cr.openjdk.java.net/~robm/8150234/webrev.01/

-Rob

Re: 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 
), 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 



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

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/



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.

Re: RFR 7167293:FtpURLConnection connection leak on FileNotFoundException

[vyom]

please find that updated 
webrev(http://cr.openjdk.java.net/~vtewari/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 



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

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/



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.