28

I have the following piece of code in a try/catch block

 InputStream inputstream = conn.getInputStream();
 InputStreamReader inputstreamreader = new  InputStreamReader(inputstream);
 BufferedReader bufferedreader = new BufferedReader(inputstreamreader);

My question is that when I have to close these streams in the finally block, do I have to close all the 3 streams or just closing the befferedreader will close all the other streams ?

comatose
  • 1,952
  • 7
  • 26
  • 42
  • 1
    Possible duplicate of [Do I need to close() both FileReader and BufferedReader?](http://stackoverflow.com/questions/1388602/do-i-need-to-close-both-filereader-and-bufferedreader) – River Mar 25 '16 at 18:09

6 Answers6

31

By convention, wrapper streams (which wrap existing streams) close the underlying stream when they are closed, so only have to close bufferedreader in your example. Also, it is usually harmless to close an already closed stream, so closing all 3 streams won't hurt.

casablanca
  • 69,683
  • 7
  • 133
  • 150
  • You cannot guarantee that an stream implementation will not throw an exception when closing it if it is already close. So it is way better to stick to the proper way and close just the BufferedReader – Soronthar Jun 29 '12 at 14:52
  • 2
    @Soronthar Then this stream implementation is not valid by contract which states `If the stream is already closed then invoking this method has no effect.` All valid implementations MUST behave like this. It is totally legit (and common) to close an already closed stream. Typically you close it first time in try-block and a second time in the finally block where closing in try-block gives you the chance to react on exception and closing in finally block is for ensuring the close. – Fabian Barney Jun 29 '12 at 16:09
  • @FabianBarney you're right, I forgot they retrofitted the streams with the Closeable interface in JSE 6. – Soronthar Jun 29 '12 at 20:04
  • 2
    It's not just by convention, it is specified. See for example FilerInputStream.close(). – user207421 Jul 01 '12 at 01:39
8

Normally it is ok to just close the most outer stream, because by convention it must trigger close on the underlying streams.

So normally code looks like this:

BufferedReader in = null;

try {
    in = new BufferedReader(new InputStreamReader(conn.getInputStream()));
    ...
    in.close(); // when you care about Exception-Handling in case when closing fails
}
finally {
    IOUtils.closeQuietly(in); // ensure closing; Apache Commons IO
}

Nevertheless there may be rare cases where an underlying stream constructor raises an exception where the stream is already opened. In that case the above code won't close the underlying stream because the outer constructor was never called and in is null. So the finally block does not close anything leaving the underlying stream opened.

Since Java 7 you can do this:

    try (OutputStream out1 = new ...; OutputStream out2 = new ...) {
        ...
        out1.close(); //if you want Exceptions-Handling; otherwise skip this
        out2.close(); //if you want Exceptions-Handling; otherwise skip this            
    } // out1 and out2 are auto-closed when leaving this block

In most cases you do not want Exception-Handling when raised while closing so skip these explicit close() calls.

Edit Here's some code for the non-believers where it is substantial to use this pattern. You may also like to read Apache Commons IOUtils javadoc about closeQuietly() method.

    OutputStream out1 = null;
    OutputStream out2 = null;

    try {
        out1 = new ...;
        out2 = new ...;

        ...

        out1.close(); // can be skipped if we do not care about exception-handling while closing
        out2.close(); // can be skipped if we ...
    }
    finally {
        /*
         * I've some custom methods in my projects overloading these
         * closeQuietly() methods with a 2nd param taking a logger instance, 
         * because usually I do not want to react on Exceptions during close 
         * but want to see it in the logs when it happened.
         */
        IOUtils.closeQuietly(out1);
        IOUtils.closeQuietly(out2);
    }

Using @Tom's "advice" will leave out1 opened when creation of out2 raises an exception. This advice is from someone talking about It's a continual source of errors for obvious reasons. Well, I may be blind, but it's not obvious to me. My pattern is idiot-safe in every use-case I can think of while Tom's pattern is error-prone.

Krishna Chaitanya
  • 7
  • 2
Fabian Barney
  • 14,219
  • 5
  • 40
  • 60
  • Please don't do that sort of `null` dance. It's a continual source of errors for obvious reasons. And what a mess. – Tom Hawtin - tackline Jun 29 '12 at 14:56
  • 1
    This is the normal pattern. What else you want to do? – Fabian Barney Jun 29 '12 at 14:57
  • `acquire(); try { use(); } finally { release(); }` – Tom Hawtin - tackline Jun 29 '12 at 14:59
  • @Bruno Right - that's what I meant in the last paragraph. – Fabian Barney Jun 29 '12 at 15:00
  • @TomHawtin-tackline So you do not realease when acquire raises an Exception? This is bad, especially when using special OutputStreams like TeeOutputStream where creation of one already proceeded and the other one fails. – Fabian Barney Jun 29 '12 at 15:05
  • I think what Tom Hawtin is trying to say is that this `null`-testing pattern isn't useful in this case. If you don't get past the initialisation, it will throw an `IOException`. Since you're not catching it anyway, it's not going to change anything. In addition, you'd have to catch it in an outer try/catch for `in.close()` in the `finally` block anyway. – Bruno Jun 29 '12 at 15:06
  • It is useful and totally standard pattern. It is so common that there exist Utility-Methods in Apache Commons `IOUtils` for it. Sorry, but he's simply wrong here. – Fabian Barney Jun 29 '12 at 15:07
  • Just saying, here, in your example, if `new BufferedReader(...)` fails, an `IOException` will be thrown outside the scope of these lines (and `in.close()` will not be executed since `in` will be `null`). If you put the initialisation with the declaration `BufferedReader in = new (...)` above, the exact same thing will happen if an exception is thrown anyway. – Bruno Jun 29 '12 at 15:09
  • Right, in that case you must declare a second variable outside the try-block and close it separatly in the finally block. But this is not possible with @TomHawtin-tackline advice, because with his advice it is _generally_ impossible to release when error while acquire occurs. – Fabian Barney Jun 29 '12 at 15:11
  • Well, I think the `acquire(); try { use(); } finally { release(); }` block must be within a within a `try`/`catch (IOException ...)` block (or method with `throws`), not necessarily with a `finally` block or extra variables, simply because both `close` and the constructor can throw it. In this case, having the `new BufferedReader` statement in or outside your block doesn't change anything. It wouldn't reach the finally statement if an exception was thrown during the `acquire()` statement anyway. – Bruno Jun 29 '12 at 15:14
  • The pattern you describe is indeed useful when you are using multiple streams. – Bruno Jun 29 '12 at 16:33
  • *bangs head against wall* How can you release something that has not been acquired? Do not conflate `try-finally` and `try-catch` (like the Java language does). One (genuine) resource pre `try-finally`. Using `null`s here not only complicates the code, but frequently introduces (sad case) bugs. Even when pointed out, it often gets incorrectly fixed. Keep it simple. – Tom Hawtin - tackline Jul 02 '12 at 01:49
  • Well, I posted some code that clearly shows why this pattern is right and should be used. And it isn't my personal bullshit mind but a very common and accepted pattern. Headbanging isn't my profession but Albert Einstein comes into my mind: "Make things as simple as possible, but not simpler." Don't forget the last part ... – Fabian Barney Jul 02 '12 at 09:02
3

Closing the outermost one is sufficient (i.e. the BufferedReader). Reading the source code of BufferedReader we can see that it closes the inner Reader when its own close method is called:

513       public void close() throws IOException {
514           synchronized (lock) {
515               if (in == null)
516                   return;
517               in.close();
518               in = null;
519               cb = null;
520           }
521       }
522   }
Tudor
  • 61,523
  • 12
  • 102
  • 142
0

As a rule of thumb, you should close everything in the reverse order that you opened them.

Andres Olarte
  • 4,380
  • 3
  • 24
  • 45
0

I would close all of them in the inverse order from which you have opened them, as if when opening them would push the reader to a stack and closing would pop the reader from the stack.

In the end, after closing all, the "reader stack" must be empty.

João Rocha da Silva
  • 4,259
  • 4
  • 26
  • 35
  • What is your reason for this recommendation? When filtered input streams and readers are specified to close the nested stream on close()? – user207421 Jul 01 '12 at 01:37
  • Just common sense, I guess. There are better answers in this thread, as the votes show. Some have even shown the source code for the library. – João Rocha da Silva Jul 01 '12 at 13:22
0

You only need to close the actual resource. You should close the resource even if constructing decorators fails. For output, you should flush the most decorator object in the happy case.

Some complications:

  • Sometimes the decorators are different resources (some compression implementations use the C heap).
  • Closing decorators in sad cases actually causes flushes, with ensuing confusion such as not actually closing the underlying resource.
  • It looks like you underlying resource is a URLConnection, which doesn't have a disconnect/close method as such.

You may wish to consider using the Execute Around idiom so you don't have to duplicate this sort of thing.

Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305