Replace CloseShield* constructors with factory methods

[robtimus]

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.

[robtimus]

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.

[coveralls]

Coverage decreased (-0.05%) to 88.581% when pulling 8bdc513 on robtimus:CloseShield.dontClose into 167effd on apache:master.

[garydgregory]

Hi @robtimus
Please rebase on git master which should give you a green set of builds.

[garydgregory]

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?

[robtimus]

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.

[robtimus]

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
}

[garydgregory]

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.

[robtimus]

I've renamed the methods to wrap, and removed the static imports in the test classes.

[garydgregory]

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.

[robtimus]

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.

[garydgregory]

I will review all of this tomorrow (Thursday).

[garydgregory]

@robtimus Merged to git master. Thank you for the PR :-)

[robtimus]

You're welcome :)