CODEWITHSUNDEEP
javasyntaxinputstream
SyntaxRules
SyntaxRules

Reputation: 2206

Do you need to close streams?

This question comes in two parts:

What is the difference between InputStream i1 = new InputStream() and new InputStream()?
and
Is it worth it to create all local variables just to close them?

I know the simple answers with the first you can keep the variable, continue to use the variable and even close the input stream like a good programmer. The second you have lost its reference, but it looks more concise. Is there any memory difference between the two? How about speed difference (in destruction)?

Now to the examples that spurred my thoughts. First we use `'new Object()` with no regards to throwing it away.

 public void getLongStrings() throws IOException {
        try {
            foo = FileCopyUtils.copyToString(new InputStreamReader(aBook.getInputStream()));
            bar = FileCopyUtils.copyToString(new InputStreamReader(aNovel.getInputStream()));
        }
        catch (IOException ioe) {
            //do something appropriate here;
        }
    }

Now for the more verbose method

public void getLongStrings() throws IOException {
        InputStream i1 = null;
        InputStream i2 = null;
        InputStreamReader isr1 = null;
        InputStreamReader isr2 = null;
        try {
            i1 = aBook.getInputStream();
            i2 = aNovel.getInputStream();
            isr1 = new InputStreamReader(i1);
            isr2 = new InputStreamReader(i2);
            foo = FileCopyUtils.copyToString(isr1);
            bar = FileCopyUtils.copyToString(isr2);
        }
        catch (IOException ioe) {
            //do something appropriate here
        } finally {
            if (i1 != null) i1.close();
            if (i2 != null) i2.close();
            if (isr1 != null) isr1.close();
            if (isr2 != null) isr2.close();
        }
    }

Is the first or second better? Is one faster over the other? Is it smart to close all of my streams even though it doesn't look at pretty?

Thanks for any insights (or edits).

Upvotes: 5

Views: 1972

Answers (3)

Adam Siemion
Adam Siemion

Reputation: 16029

Assuming FileCopyUtils.copyToString() does not close the provided stream then the second approach is better.

close() has to be called to release any resources associated with the stream. So it is not a matter of code prettiness or performance, but correctness.

The second approach has a bug, when any of the close calls throws an exception all the remaining close() calls will not be invoked.

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1499860

The second you have lost its reference, but it looks more concise.

Yes, it is more concise. You're doing less with the object - importantly, you're not doing things you should be doing. Taking a shortcut will often produce less code, but by leaving file handles open pending the finalizer closing things, you'll find that you'll get exceptions which are hard to reproduce reliably.

Is there any memory difference between the two?

Maybe, but that's not relevant. The important resource here isn't the memory, but the file handle.

How about speed difference (in destruction)?

Again, likely to be irrelevant. Correctness is far more important.

(Note that your current second code is still not completely reliable - if one of the early close() calls throws an exception, you won't close the rest.)

If you're using Java 7, you should look at the try-with-resources statement, which makes all of this a lot simpler.

EDIT: As noted in JB Nizet's answer, the code you're using may already close the underlying stream - it depends on what FileCopyUtils is. If it's the Spring class, you're okay. If it's something else, you may not be. You should read the documentation. The general principle is as above though: you should make sure that something closes the streams. Don't assume that just because in this case you don't need to close things explicitly, that that's true in the general case.

Note that Guava's InputSupplier and OutputSupplier interfaces are useful here - it allows you to pass around the suppliers knowing that the input/output haven't been opened yet... which means that other code can perform the open/copy/close (or whatever is required) and handle it all for you... you don't end up using any objects in your code which could be closed.

Upvotes: 7

JB Nizet
JB Nizet

Reputation: 691635

Assuming this is Spring's FileCopyUtils, its javadoc says:

All copy methods use a block size of 4096 bytes, and close all affected streams when done.

(emphasis mine)

So your inital approach is perfectly fine in this case: Spring handles the closing of the Reader, and closing a Reader closes the wrapped stream.

If that wasn't the case, you would have to close the Reader, preferrably using Java 7's try with resources.

Note that your second example, closes the stream but doesn't have to: closing the reader automatically closes it. Also note that if the first reader can't be closed and throws an IOException, the second won't be closed correctly. Another good reason to use try with resources, whic is guaranteed to close everything.

Upvotes: 4

Related Questions