Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
- Original Message - On 8/11/2012 7:17 AM, Andrew Hughes wrote: - Original Message - The bug number is 8003120 Thanks. Pushed to tl: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f51943263267 I did not construe Lance's mail to indicate an approval to push. Ok, sorry, I did. I did explicitly say Ok for tl? If so, can I have a bug ID for this?. In future, it would be better if people were explicit about whether they expected the push to take place or not. As I said in both e-mails, I don't really want to change the well-tested original. We can make the syntactic changes Lance suggested in a separate fix, which then means the two fixes aren't muddled together. The turnaround on this was just a bit too quick in my opinion. At least there was public review. I still see patches just being committed without any public review. David Best Lance On Nov 7, 2012, at 3:30 PM, Andrew Hughes wrote: IcedTea bug: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1197 com.sun.naming.internal.ResourceManager.getApplicationResources() does not close the input streams it gets from helper.getResources() and helper.getJavaHomeLibStream(). This patch: http://cr.openjdk.java.net/~andrew/pr1197/webrev.01/ adds finally blocks to close these streams. As noted in the original bug report, this has been tested on a J2EE server (JBoss) without issue. I realise that there's a possibility that try-with-resources could be used here, but the patch as posted is the one that was tested and I don't want to negate that testing by changing it. Ok for tl? If so, can I have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
- Original Message - On 07/11/2012 20:51, Andrew Hughes wrote: : As you can see on the IcedTea bug, I've asked the same question. I'd have preferred it to use try-with-resources myself (easier to follow for one thing), but given the patch is as it is, I'm now wary about changing it and negating the existing testing that's already been done. I see you've pushed this already but I think we need to open a follow-on bug on this as it looks like there is additional work to do. Particularly the case where there are several streams and close fails (looks to me that it will not attempt to close all streams in that case). Also, given that the target is jdk8 then it seems reasonable to see if try-with-resources could be used for the second case, I assume the testing you mentioned was with a different release. The other thing is that we try to include a regression test with all fixes where possible, I don't know how feasible it for this case. The testing was on 7u as there isn't an 8 release yet. I'm not sure how you'd test this either. -Alan. -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
[PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
IcedTea bug: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1197 com.sun.naming.internal.ResourceManager.getApplicationResources() does not close the input streams it gets from helper.getResources() and helper.getJavaHomeLibStream(). This patch: http://cr.openjdk.java.net/~andrew/pr1197/webrev.01/ adds finally blocks to close these streams. As noted in the original bug report, this has been tested on a J2EE server (JBoss) without issue. I realise that there's a possibility that try-with-resources could be used here, but the patch as posted is the one that was tested and I don't want to negate that testing by changing it. Ok for tl? If so, can I have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
Is there a reason the patch was not created originally leveraging try-with-resoruces as it seems like the perfect candidate from the webrev? I can create a bug for it, but I think I would prefer to see the patch take advantage of try-with-resoruces Best Lance On Nov 7, 2012, at 3:30 PM, Andrew Hughes wrote: IcedTea bug: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1197 com.sun.naming.internal.ResourceManager.getApplicationResources() does not close the input streams it gets from helper.getResources() and helper.getJavaHomeLibStream(). This patch: http://cr.openjdk.java.net/~andrew/pr1197/webrev.01/ adds finally blocks to close these streams. As noted in the original bug report, this has been tested on a J2EE server (JBoss) without issue. I realise that there's a possibility that try-with-resources could be used here, but the patch as posted is the one that was tested and I don't want to negate that testing by changing it. Ok for tl? If so, can I have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
The bug number is 8003120 Best Lance On Nov 7, 2012, at 3:30 PM, Andrew Hughes wrote: IcedTea bug: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1197 com.sun.naming.internal.ResourceManager.getApplicationResources() does not close the input streams it gets from helper.getResources() and helper.getJavaHomeLibStream(). This patch: http://cr.openjdk.java.net/~andrew/pr1197/webrev.01/ adds finally blocks to close these streams. As noted in the original bug report, this has been tested on a J2EE server (JBoss) without issue. I realise that there's a possibility that try-with-resources could be used here, but the patch as posted is the one that was tested and I don't want to negate that testing by changing it. Ok for tl? If so, can I have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
- Original Message - Is there a reason the patch was not created originally leveraging try-with-resources as it seems like the perfect candidate from the webrev? I can create a bug for it, but I think I would prefer to see the patch take advantage of try-with-resoruces As you can see on the IcedTea bug, I've asked the same question. I'd have preferred it to use try-with-resources myself (easier to follow for one thing), but given the patch is as it is, I'm now wary about changing it and negating the existing testing that's already been done. Best Lance On Nov 7, 2012, at 3:30 PM, Andrew Hughes wrote: IcedTea bug: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1197 com.sun.naming.internal.ResourceManager.getApplicationResources() does not close the input streams it gets from helper.getResources() and helper.getJavaHomeLibStream(). This patch: http://cr.openjdk.java.net/~andrew/pr1197/webrev.01/ adds finally blocks to close these streams. As noted in the original bug report, this has been tested on a J2EE server (JBoss) without issue. I realise that there's a possibility that try-with-resources could be used here, but the patch as posted is the one that was tested and I don't want to negate that testing by changing it. Ok for tl? If so, can I have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07 [image/gif:oracle_sig_logo.gif] Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
- Original Message - The bug number is 8003120 Thanks. Pushed to tl: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f51943263267 Best Lance On Nov 7, 2012, at 3:30 PM, Andrew Hughes wrote: IcedTea bug: http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1197 com.sun.naming.internal.ResourceManager.getApplicationResources() does not close the input streams it gets from helper.getResources() and helper.getJavaHomeLibStream(). This patch: http://cr.openjdk.java.net/~andrew/pr1197/webrev.01/ adds finally blocks to close these streams. As noted in the original bug report, this has been tested on a J2EE server (JBoss) without issue. I realise that there's a possibility that try-with-resources could be used here, but the patch as posted is the one that was tested and I don't want to negate that testing by changing it. Ok for tl? If so, can I have a bug ID for this? Thanks, -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com -- Andrew :) Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) PGP Key: 248BDC07 (https://keys.indymedia.org/) Fingerprint = EC5A 1F5E C0AD 1D15 8F1F 8F91 3B96 A578 248B DC07
Re: [PATCH FOR REVIEW] ResourceManager.getApplicationResources() does not close InputStreams
On 07/11/2012 20:51, Andrew Hughes wrote: : As you can see on the IcedTea bug, I've asked the same question. I'd have preferred it to use try-with-resources myself (easier to follow for one thing), but given the patch is as it is, I'm now wary about changing it and negating the existing testing that's already been done. I see you've pushed this already but I think we need to open a follow-on bug on this as it looks like there is additional work to do. Particularly the case where there are several streams and close fails (looks to me that it will not attempt to close all streams in that case). Also, given that the target is jdk8 then it seems reasonable to see if try-with-resources could be used for the second case, I assume the testing you mentioned was with a different release. The other thing is that we try to include a regression test with all fixes where possible, I don't know how feasible it for this case. -Alan.
