[GitHub] brooklyn-server pull request #196: Close streams after usage

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/brooklyn-server/pull/196


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/196#discussion_r67138867
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/crypto/SecureKeys.java ---
@@ -78,11 +78,11 @@ public static X509Principal 
getX509PrincipalWithCommonName(String commonName) {
 return new X509Principal("" + "C=None," + "L=None," + "O=None," + 
"OU=None," + "CN=" + commonName);
 }
 
-/** reads RSA or DSA / pem style private key files (viz {@link 
#toPem(KeyPair)}), extracting also the public key if possible
+/** reads RSA or DSA / pem style private key files (viz {@link 
#toPem(KeyPair)}), extracting also the public key if possible. Closes the 
stream.
  * @throws IllegalStateException on errors, in particular {@link 
PassphraseProblem} if that is the problem */
 public static KeyPair readPem(InputStream input, final String 
passphrase) {
 // TODO cache is only for fallback "reader" strategy (2015-01); 
delete when Parser confirmed working
-byte[] cache = Streams.readFully(input);
+byte[] cache = Streams.readFullyAndClose(input);
--- End diff --

@neykov How about overloading `readPem` to take a byte array? It looks like 
that is the way it is used anyway.

And for the tests that call it, I wonder if we should add `byte[] 
ResourceUtils.getResourceAsBytes(String url)`. Again it feels like many callers 
of the existing `getResourceFromUrl` either want it as a string or as bytes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/196#discussion_r67031965
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBomScanner.java
 ---
@@ -268,7 +268,7 @@ private void addLibraryDetails(Bundle bundle, 
Map catalog) {
 
 private String readBom(URL bom) {
 try (final InputStream ins = bom.openStream()) {
-return Streams.readFullyString(ins);
+return Streams.readFullyStringAndClose(ins);
--- End diff --

Haven't noticed that, will revert.
Yes, my thinking was that it doesn't really make sense to keep the stream 
open after reading it fully. I've updated the javadoc to encourage the usage of 
xxxAndClose alternatives. Only very specific cases will need to keep it 
unclosed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/196#discussion_r67030921
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/crypto/SecureKeys.java ---
@@ -78,11 +78,11 @@ public static X509Principal 
getX509PrincipalWithCommonName(String commonName) {
 return new X509Principal("" + "C=None," + "L=None," + "O=None," + 
"OU=None," + "CN=" + commonName);
 }
 
-/** reads RSA or DSA / pem style private key files (viz {@link 
#toPem(KeyPair)}), extracting also the public key if possible
+/** reads RSA or DSA / pem style private key files (viz {@link 
#toPem(KeyPair)}), extracting also the public key if possible. Closes the 
stream.
  * @throws IllegalStateException on errors, in particular {@link 
PassphraseProblem} if that is the problem */
 public static KeyPair readPem(InputStream input, final String 
passphrase) {
 // TODO cache is only for fallback "reader" strategy (2015-01); 
delete when Parser confirmed working
-byte[] cache = Streams.readFully(input);
+byte[] cache = Streams.readFullyAndClose(input);
--- End diff --

Agree in general. It's the way we use readPem that prompted me to do the 
change - we are not closing the streams in any of the callers. Will have 
another look on how it can be improved.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/196#discussion_r67019434
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/util/core/crypto/SecureKeys.java ---
@@ -78,11 +78,11 @@ public static X509Principal 
getX509PrincipalWithCommonName(String commonName) {
 return new X509Principal("" + "C=None," + "L=None," + "O=None," + 
"OU=None," + "CN=" + commonName);
 }
 
-/** reads RSA or DSA / pem style private key files (viz {@link 
#toPem(KeyPair)}), extracting also the public key if possible
+/** reads RSA or DSA / pem style private key files (viz {@link 
#toPem(KeyPair)}), extracting also the public key if possible. Closes the 
stream.
  * @throws IllegalStateException on errors, in particular {@link 
PassphraseProblem} if that is the problem */
 public static KeyPair readPem(InputStream input, final String 
passphrase) {
 // TODO cache is only for fallback "reader" strategy (2015-01); 
delete when Parser confirmed working
-byte[] cache = Streams.readFully(input);
+byte[] cache = Streams.readFullyAndClose(input);
--- End diff --

Feels like the responsibility of the caller to close the stream, rather 
than this method.

Looking at things like Guava's `ByteStreams`, they've deprecated all the 
methods that closed the stream that was passed in (and switched to a completely 
different pattern that uses `ByteSource`).

That makes me think it is the wrong thing to close the stream we are given.

Are there any good examples of good modern APIs that close the stream that 
is passed in?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/196#discussion_r67018475
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogBomScanner.java
 ---
@@ -268,7 +268,7 @@ private void addLibraryDetails(Bundle bundle, 
Map catalog) {
 
 private String readBom(URL bom) {
 try (final InputStream ins = bom.openStream()) {
-return Streams.readFullyString(ins);
+return Streams.readFullyStringAndClose(ins);
--- End diff --

The stream is created in a try-with-resources [1], so no need to close. Is 
your thinking that if we call `readFullyStringAndClose()` everywhere, then it 
will be easier to make sure we're closing things everywhere? (e.g. we can 
search for uses of readFullyString, and not have many usages to review).

[1] 
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/196#discussion_r67017851
  
--- Diff: 
camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/AbstractYamlTest.java
 ---
@@ -125,7 +125,7 @@ protected Entity createAndStartApplication(String... 
multiLineYaml) throws Excep
 }
 
 protected Entity createAndStartApplication(Reader input) throws 
Exception {
-return createAndStartApplication(Streams.readFully(input));
+return createAndStartApplication(Streams.readFullyAndClose(input));
--- End diff --

Overall, I'm fine with this because it's just in tests. But it feels like 
the responsibility of the caller to close the Reader that they created, rather 
than this method.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #196: Close streams after usage

[neykov]

GitHub user neykov opened a pull request:

https://github.com/apache/brooklyn-server/pull/196

Close streams after usage

Fixes https://builds.apache.org/job/brooklyn-master-windows/151/console

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/neykov/brooklyn-server close-streams

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/brooklyn-server/pull/196.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #196


commit 2bd3abe1ae50f91b7bbd7406f1b237744280c430
Author: Svetoslav Neykov 
Date:   2016-06-13T14:41:15Z

Close streams after usage




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---