Should I close a StringReader?

Question

I use a StringReader to turn a string into something I can upload to an SFTP server (it takes a stream). Is there any point in closing that StringReader afterwards? As far as I can see in the source it just sets the string to null...

I could just do it, but since the close method is marked as throwing an IOException and all I have to wrap it in a try catch and the code just ends up looking a lot more horrible than it perhaps needs to be.

Cameroncameroon

Answer 1

If you know you're dealing with a StringReader that you'll be throwing away, I don't see any reason to close it. I can't imagine any reason you'd be holding a reference to it after you'd close it, so there's no real benefit to the string being set to null for garbage collection. If you were creating a method that takes a Reader then it might make sense to close it since you don't know the underlying type.

Desiree

Yeah, if I didn't know what kind of Reader I was dealing with, I would definitely close it. – Cameroncameroon

Answer 2

It does more than that. If I may quote the JavaDoc:

/**
 * Closes the stream and releases any system resources associated with
 * it. Once the stream has been closed, further read(),
 * ready(), mark(), or reset() invocations will throw an IOException.
 * Closing a previously closed stream has no effect.
 */

So yes, you should close that reader. Not for the sake of resources but for the sake of good style and programmers that may follow you. You don't know where this instance will be passed to and what someone else will try to do with it. Someday you might also choose to change the interface and accept any Reader implementation in which case you might deal with a Reader that requires a call to close() to free resources.

So it is good style to prevent the further (possibly wrong) use of this instance once you're done with it. And since it doesn't hurt, it will only prevent possible errors in the future.

Edit: Since you say, that your close() method is declaring an exception it might throw I would say that you need to call close() since StringReader.close() does not throw an exception. However, Reader.close() does. So you already allow other implementations of Reader and so you must close it since you can't know what implementations of Reader you'll eventually get. If we are talking about three lines of code that never leave that scope, declare your variable StringReader and call close anyway (in that case without exception handling).

Cower

Given StringReader reads from a String what system reousrces would it be freeing if all it holds is a String ? – Vraisemblance

As I said, for a StringReader it's not a matter of resources. But other implementations of Reader might be different. Depending on your interface and possible future changes, it might get relevant some day and even if it's only a StringReader, it's still good style. – Cower

I know the doc says that, but when looking at the source it's only setting the string to null (which of course would make the other methods crash) – Cameroncameroon

Maybe my edit helps? Looks like you just declared your variable with the wrong type. StringReader.close() does not declare an exception in my copy of Java6u20. – Cower

Answer 3

Though strictly not necessary, because StringReader only holds onto a String, as a matter of good form its always a good idea to close all Readers anyway. Today your code might be using a StringReader but if you changed it to another Reader that really needs to be closed, your code w/out the close would be wrong while your w/ close would be fine.

Vraisemblance

Answer 4

You don't need to catch an exception if your variable has type StringReader, instead of Reader, since StringReader#close() doesn't throw an exception: only Reader#close() does. So you can use try-with-resources to automatically close the reader, without needing to have boilerplate to handle exceptions that won't occur. Reader#close() throwing IOException means subtypes can throw this type of exception, not that they must. This is one of the rare cases where you want to declare a variable with a subtype, not the supertype; see Use interface or type for variable definition in java? for more.

Thus, I'd suggest the following, which just requires one level of nesting, which is par for resources:

try (StringReader reader = new StringReader(string)) {
    // Do something with reader.
}

However, there's little value in closing a StringReader, since it doesn't hold an external resource (only Java-managed memory, not a file handle or native memory, say), so it's fine to omit it, though I'd recommend a comment stating why this is safe, since otherwise not closing a reader is surprising. As you note, close() just nulls out the field, per JDK 8 source: StringReader.java:198. If you want to avoid the nesting and close, you could just write this:

// Don't need to close StringReader, since no external resource.
StringReader reader = new StringReader(string);
// Do something with reader.

...or (using more general type for variable):

// Don't need to close StringReader, since no external resource.
Reader reader = new StringReader(string);
// Do something with reader.

Normal try-with-resources works here because StringReader#close() overrides Reader#close() and mercifully states that it does not throw IOException.

Beware that this is not the case for StringWriter: StringWriter#close() does declare that it throws IOException, despite being a nop! This is presumably for forward compatibility, so it could throw an exception in a future implementation, though this is unlikely. See my answer to Will not closing a stringwriter cause a leak?.

In such a case (if the method does not throw an exception, but the interface stated that it could), the tight way to write this, which you are presumably alluding to, is:

Reader reader = new StringReader(string);
try {
    // Do something with reader, which may or may not throw IOException.
} finally {
    try {
        reader.close();
    } catch (IOException e) {
        throw new AssertionError("StringReader#close() cannot throw IOException", e);
    }
}

This level of boilerplate is necessary because you can't just put a catch on the overall try block, as otherwise you might accidentally swallow an IOException thrown by the body of your code. Even if there isn't any currently, some might be added in future and you'd want to be warned of this by the compiler. Note also that the AssertionError, which documents the current behavior, would also mask an exception thrown by the body of the try statement, though this should never occur. If this were the alternative, you'd clearly be better off omitting the close() and commenting why.

This answer depends on the fact that you're creating the StringReader yourself; of course if you receive a Reader from somewhere else (as the return type of a factory, say), then you need to close it and handle the possible exception, since you don't know what resources it may hold, and it may throw an exception.

Beeswing

Answer 5

If you close the stream and release any system resources associated with it. Once the stream has been closed, further read(), ready(), mark(), or reset() invocations will throw an IOException. Closing a previously closed stream has no effect. Specified by: close in interface Closeable Specified by: close in class Reader

Unmanned