You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/12/06 13:57:18 UTC
[GitHub] [commons-io] robtimus opened a new pull request #173: Replace CloseShield* constructors with factory methods
robtimus opened a new pull request #173:
URL: https://github.com/apache/commons-io/pull/173
Using `CloseShieldInputStream`, `CloseShieldOutputStream`, `CloseShieldReader` or `CloseShieldWriter` currently makes it easier to let resource leaks go undetected. That's because most IDEs are smart when it comes to wrapping an `InputStream` into another `InputStream`, and likewise for `OutputStream`, `Reader` and `Writer` - it will assume that the `close` method delegates and therefore assumes that closing the wrapper is safe. For instance, the following will not produce an IDE warning that the `FileInputStream` is never closed:
try (InputStream inputStream = new CloseShieldInputStream(new FileInputStream(file))) {
// use inputStream
}
This PR should make it easier for IDEs to detect such resource leaks by two simple changes:
* Add a static method to do the wrapping. IDEs are not smart enough to detect that these static methods simply wrap the given resource. (In fact, if I'd make a copy of `Objects.requireNonNull`, my IDE would complain about that too.)
* Deprecate the constructors so people are encouraged to use the static methods. In version 3.x these can be made private.
Using a static import for the wrapper method, the above code snippet becomes the following, and there is an IDE warning that the `FileInputStream` is never closed:
try (InputStream inputStream = dontClose(new FileInputStream(file))) {
// use inputStream
}
I've tried to import and use all of the separate `dontClose` methods in one class, and the compiler has no problems with that, so there will be no ambiguity errors if you statically import more than one `dontClose` method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-740209618
I've renamed the methods to `wrap`, and removed the static imports in the test classes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-740189244
> I've come to like code that almost reads as if it's English. I don't get a feeling about how to interpret the first one: "a CloseShieldInputStream on a new FileInputStream". The `with` option reads better, but it still sounds a bit off.
>
> How do you feel about simply calling the methods "wrap" or "wrapping"?
>
> ```
> try (InputStream inputStream = CloseShieldInputStream.wrap(new FileInputStream(file))) {
> // use inputStream
> }
> try (InputStream inputStream = CloseShieldInputStream.wrapping(new FileInputStream(file))) {
> // use inputStream
> }
> ```
>
> If you don't like those options (and can't think of a better one), I think "with" is the best of the other two alternatives: "a CloseShieldInputStream with a new FileInputStream" does say what it means.
+1 to wrap, it really captures what is going on here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-740121762
I thought of another alternative that I think works both with and without static imports:
try (InputStream inputStream = CloseShieldInputStream.preventFromClosing(new FileInputStream(file))) {
// use inputStream
}
try (InputStream inputStream = preventFromClosing(new FileInputStream(file))) {
// use inputStream
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739508273
These failed tests are unrelated to this PR. They are caused by test `FileUtilsTestCase.testForceDeleteReadOnlyFile` that fails to delete the file on Windows. The same occurs on my local machine, it's the only failing test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-740032701
I've come to like code that almost reads as if it's English. I don't get a feeling about how to interpret the first one: "a CloseShieldInputStream on a new FileInputStream". The `with` option reads better, but it still sounds a bit off.
How do you feel about simply calling the methods "wrap" or "wrapping"?
try (InputStream inputStream = CloseShieldInputStream.wrap(new FileInputStream(file))) {
// use inputStream
}
try (InputStream inputStream = CloseShieldInputStream.wrapping(new FileInputStream(file))) {
// use inputStream
}
If you don't like those options (and can't think of a better one), I think "with" is the best of the other two alternatives: "a CloseShieldInputStream with a new FileInputStream" does say what it means.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a change in pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #173:
URL: https://github.com/apache/commons-io/pull/173#discussion_r538433221
##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
##########
@@ -50,4 +54,16 @@ public void close() {
in = ClosedInputStream.CLOSED_INPUT_STREAM;
}
+ /**
+ * Creates a proxy that shields the given input stream from being
+ * closed.
+ *
+ * @param in underlying input stream
Review comment:
`@param in underlying input stream` -> `@param inputStream the input stream to wrap`
##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldReader.java
##########
@@ -50,4 +54,16 @@ public void close() {
in = ClosedReader.CLOSED_READER;
}
+ /**
+ * Creates a proxy that shields the given reader from being
+ * closed.
+ *
+ * @param in underlying reader
+ * @return the created proxy
+ * @since 2.9.0
+ */
+ public static CloseShieldReader wrap(final Reader in) {
Review comment:
as see other Javadoc comments, in this case `in` -> `reader`
##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
##########
@@ -35,7 +35,11 @@
* closed.
*
* @param in underlying input stream
+ * @deprecated Using this constructor prevents IDEs from warning if
+ * the underlying input stream is never closed.
+ * Use {@link #wrap(InputStream)} instead.
*/
+ @Deprecated
Review comment:
I don't agree with deprecating these APIs, different versions of different IDEs raise different warnings depending on your preferred configuration of said IDE. Let's keep this PR simpler and lighter, we can always add deprecation later if it is truly warranted.
##########
File path: src/test/java/org/apache/commons/io/output/CloseShieldWriterTest.java
##########
@@ -40,19 +40,14 @@
@BeforeEach
public void setUp() {
original = spy(new StringBuilderWriter());
- shielded = new CloseShieldWriter(original);
+ shielded = CloseShieldWriter.wrap(original);
}
@Test
public void testClose() throws IOException {
shielded.close();
verify(original, never()).close();
- try {
- shielded.write('x');
- fail("write(c)");
- } catch (final IOException ignore) {
- // expected
- }
+ assertThrows(IOException.class, () -> shielded.write('x'), "write(c)");
Review comment:
Nice use of a new JUnit 5 API! 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739551169
Hi @robtimus
Please rebase on git master which should give you a green set of builds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739508551
[](https://coveralls.io/builds/35480423)
Coverage increased (+0.07%) to 88.593% when pulling **32ffd80b6c3393ca771831d73374d3457df74ea7 on robtimus:CloseShield.dontClose** into **1dc7834e639b2245ada901bc011f651a09a98aaf on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on a change in pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on a change in pull request #173:
URL: https://github.com/apache/commons-io/pull/173#discussion_r538746587
##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
##########
@@ -50,4 +54,16 @@ public void close() {
in = ClosedInputStream.CLOSED_INPUT_STREAM;
}
+ /**
+ * Creates a proxy that shields the given input stream from being
+ * closed.
+ *
+ * @param in underlying input stream
Review comment:
Changed as suggested
##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldReader.java
##########
@@ -50,4 +54,16 @@ public void close() {
in = ClosedReader.CLOSED_READER;
}
+ /**
+ * Creates a proxy that shields the given reader from being
+ * closed.
+ *
+ * @param in underlying reader
+ * @return the created proxy
+ * @since 2.9.0
+ */
+ public static CloseShieldReader wrap(final Reader in) {
Review comment:
Changed as suggested
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739508551
[](https://coveralls.io/builds/35518510)
Coverage decreased (-0.05%) to 88.581% when pulling **d89454e6f78aea8550c74d405e600355ec76f28f on robtimus:CloseShield.dontClose** into **167effda26f5fabfef36a9878f0a0ce395855d10 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739508551
[](https://coveralls.io/builds/35509073)
Coverage increased (+0.007%) to 88.641% when pulling **56af9bec67e01255ff1ea26e194c1918e2bcbb59 on robtimus:CloseShield.dontClose** into **167effda26f5fabfef36a9878f0a0ce395855d10 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-741818970
I will review all of this tomorrow (Thursday).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739577418
Hi All:
I'm not a fan of the factory method name `dontClose()` as I usually only use static imports for JUnit methods, and this it's also weird to me to read code that says "don't do something", so `CloseShieldInputStream.dontClose(...)` is particularly odd to my ear, YMMV of course.
The following reads better to me:
```
try (InputStream inputStream = CloseShieldInputStream.on(new FileInputStream(file))) {
// use inputStream
}
```
or:
```
try (InputStream inputStream = CloseShieldInputStream.with(new FileInputStream(file))) {
// use inputStream
}
```
WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-743363861
You're welcome :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-742738847
@robtimus Merged to git master. Thank you for the PR :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] coveralls edited a comment on pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #173:
URL: https://github.com/apache/commons-io/pull/173#issuecomment-739508551
[](https://coveralls.io/builds/35552261)
Coverage decreased (-0.05%) to 88.581% when pulling **8bdc5130c4e39db4524e657bda09260d6d9fcc01 on robtimus:CloseShield.dontClose** into **167effda26f5fabfef36a9878f0a0ce395855d10 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory merged pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #173:
URL: https://github.com/apache/commons-io/pull/173
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] robtimus commented on a change in pull request #173: Replace CloseShield* constructors with factory methods
Posted by GitBox <gi...@apache.org>.
robtimus commented on a change in pull request #173:
URL: https://github.com/apache/commons-io/pull/173#discussion_r538752019
##########
File path: src/main/java/org/apache/commons/io/input/CloseShieldInputStream.java
##########
@@ -35,7 +35,11 @@
* closed.
*
* @param in underlying input stream
+ * @deprecated Using this constructor prevents IDEs from warning if
+ * the underlying input stream is never closed.
+ * Use {@link #wrap(InputStream)} instead.
*/
+ @Deprecated
Review comment:
I feel that without deprecating the constructors, this PR adds no real value. People that currently use the constructors will keep doing so because they see no reason to change their code, and new users of the classes may only get confused if there are two ways to do the same thing. They'll probably end up using the constructors too because that's a character shorter (`new` vs `.wrap`).
My thought was that deprecating the constructors will make current users think about whether or not they have closed the underlying streams, even if their IDEs don't warn them. New users will chose the wrapper method because the alternative is deprecated.
The only downside to deprecating the constructors that I can see is that current users that don't have their IDEs setup to warn about unclosed streams will get warnings where there were none first, but as I said, perhaps they will think about whether or not their streams are properly closed somewhere else.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org