Is it necessary to close each nested OutputStream and Writer separately?

Question

I am writing a piece of code:

OutputStream outputStream = new FileOutputStream(createdFile);
GZIPOutputStream gzipOutputStream = new GZIPOutputStream(outputStream);
BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(gzipOutputStream));

Do I need to close every stream or writer like the following?

gzipOutputStream.close();
bw.close();
outputStream.close();

Or will just closing the last stream be fine?

bw.close();

Solution 1

Assuming all the streams get created okay, yes, just closing bw is fine with those stream implementations; but that's a big assumption.

I'd use try-with-resources (tutorial) so that any issues constructing the subsequent streams that throw exceptions don't leave the previous streams hanging, and so you don't have to rely on the stream implementation having the call to close the underlying stream:

try (
    OutputStream outputStream = new FileOutputStream(createdFile);
    GZIPOutputStream gzipOutputStream = new GZIPOutputStream(outputStream);
    OutputStreamWriter osw = new OutputStreamWriter(gzipOutputStream);
    BufferedWriter bw = new BufferedWriter(osw)
    ) {
    // ...
}

Note you no longer call close at all.

Important note: To have try-with-resources close them, you must assign the streams to variables as you open them, you cannot use nesting. If you use nesting, an exception during construction of one of the later streams (say, GZIPOutputStream) will leave any stream constructed by the nested calls inside it open. From JLS §14.20.3:

A try-with-resources statement is parameterized with variables (known as resources) that are initialized before execution of the try block and closed automatically, in the reverse order from which they were initialized, after execution of the try block.

Note the word "variables" (my emphasis).

E.g., don't do this:

// DON'T DO THIS
try (BufferedWriter bw = new BufferedWriter(
        new OutputStreamWriter(
        new GZIPOutputStream(
        new FileOutputStream(createdFile))))) {
    // ...
}

...because an exception from the GZIPOutputStream(OutputStream) constructor (which says it may throw IOException, and writes a header to the underlying stream) would leave the FileOutputStream open. Since some resources have constructors that may throw and others don't, it's a good habit to just list them separately.

We can double-check our interpretation of that JLS section with this program:

public class Example {

    private static class InnerMost implements AutoCloseable {
        public InnerMost() throws Exception {
            System.out.println("Constructing " + this.getClass().getName());
        }

        @Override
        public void close() throws Exception {
            System.out.println(this.getClass().getName() + " closed");
        }
    }

    private static class Middle implements AutoCloseable {
        private AutoCloseable c;

        public Middle(AutoCloseable c) {
            System.out.println("Constructing " + this.getClass().getName());
            this.c = c;
        }

        @Override
        public void close() throws Exception {
            System.out.println(this.getClass().getName() + " closed");
            c.close();
        }
    }

    private static class OuterMost implements AutoCloseable {
        private AutoCloseable c;

        public OuterMost(AutoCloseable c) throws Exception {
            System.out.println("Constructing " + this.getClass().getName());
            throw new Exception(this.getClass().getName() + " failed");
        }

        @Override
        public void close() throws Exception {
            System.out.println(this.getClass().getName() + " closed");
            c.close();
        }
    }

    public static final void main(String[] args) {
        // DON'T DO THIS
        try (OuterMost om = new OuterMost(
                new Middle(
                    new InnerMost()
                    )
                )
            ) {
            System.out.println("In try block");
        }
        catch (Exception e) {
            System.out.println("In catch block");
        }
        finally {
            System.out.println("In finally block");
        }
        System.out.println("At end of main");
    }
}

...which has the output:

Constructing Example$InnerMost
Constructing Example$Middle
Constructing Example$OuterMost
In catch block
In finally block
At end of main

Note that there are no calls to close there.

If we fix main:

public static final void main(String[] args) {
    try (
        InnerMost im = new InnerMost();
        Middle m = new Middle(im);
        OuterMost om = new OuterMost(m)
        ) {
        System.out.println("In try block");
    }
    catch (Exception e) {
        System.out.println("In catch block");
    }
    finally {
        System.out.println("In finally block");
    }
    System.out.println("At end of main");
}

then we get the appropriate close calls:

Constructing Example$InnerMost
Constructing Example$Middle
Constructing Example$OuterMost
Example$Middle closed
Example$InnerMost closed
Example$InnerMost closed
In catch block
In finally block
At end of main

(Yes, two calls to InnerMost#close is correct; one is from Middle, the other from try-with-resources.)

Solution 2

You can close the outer most stream, in fact you don't need to retain all the streams wrapped and you can use Java 7 try-with-resources.

try (BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(
                     new GZIPOutputStream(new FileOutputStream(createdFile)))) {
     // write to the buffered writer
}

---

If you subscribe to YAGNI, or you-aint-gonna-need-it, you should be only adding code you actually need. You shouldn't be adding code you imagine you might need but in reality doesn't do anything useful.

Take this example and imagine what could possibly go wrong if you didn't do this and what the impact would be?

try (
    OutputStream outputStream = new FileOutputStream(createdFile);
    GZIPOutputStream gzipOutputStream = new GZIPOutputStream(outputStream);
    OutputStreamWriter osw = new OutputStreamWriter(gzipOutputStream);
    BufferedWriter bw = new BufferedWriter(osw)
    ) {
    // ...
}

Lets start with FileOutputStream which calls open to do all the real work.

/**
 * Opens a file, with the specified name, for overwriting or appending.
 * @param name name of file to be opened
 * @param append whether the file is to be opened in append mode
 */
private native void open(String name, boolean append)
    throws FileNotFoundException;

If the file is not found, there is no underlying resource to close, so closing it won't make any difference. If The file exists, it should be throwing a FileNotFoundException. So there is nothing to be gained by trying to close the resource from this line alone.

The reason you need to close the file is when the file is opened successfully, but you later get an error.

Lets look at the next stream GZIPOutputStream

There is code which can throw an exception

private void writeHeader() throws IOException {
    out.write(new byte[] {
                  (byte) GZIP_MAGIC,        // Magic number (short)
                  (byte)(GZIP_MAGIC >> 8),  // Magic number (short)
                  Deflater.DEFLATED,        // Compression method (CM)
                  0,                        // Flags (FLG)
                  0,                        // Modification time MTIME (int)
                  0,                        // Modification time MTIME (int)
                  0,                        // Modification time MTIME (int)
                  0,                        // Modification time MTIME (int)
                  0,                        // Extra flags (XFLG)
                  0                         // Operating system (OS)
              });
}

This writes the header of the file. Now it would be very unusual for you to be able to open a file for writing but not be able to write even 8 bytes to it, but lets imagine this could happen and we don't close the file afterwards. What does happen to a file if it is not closed?

You don't get any unflushed writes, they are discarded and in this case, there is no successfully written bytes to the stream which isn't buffered at this point anyway. But a file which is not closed doesn't live forever, instead FileOutputStream has

protected void finalize() throws IOException {
    if (fd != null) {
        if (fd == FileDescriptor.out || fd == FileDescriptor.err) {
            flush();
        } else {
            /* if fd is shared, the references in FileDescriptor
             * will ensure that finalizer is only called when
             * safe to do so. All references using the fd have
             * become unreachable. We can call close()
             */
            close();
        }
    }
}

If you don't close a file at all, it gets closed anyway, just not immediately (and like I said, data which is left in a buffer will be lost this way, but there is none at this point)

What is the consequence of not closing the file immediately? Under normal conditions, you potentially lose some data, and you potentially run out of file descriptors. But if you have a system where you can create files but you can't write anything to them, you have a bigger problem. i.e. it hard to imagine why you are repeatedly trying to create this file despite the fact you are failing.

Both OutputStreamWriter and BufferedWriter don't throw IOException in their constructors, so it not clear what problem they would cause. In The case of BufferedWriter, you could get an OutOfMemoryError. In this case it will immediately trigger a GC, which as we have seen will close the file anyway.

Solution 3

If all of the streams have been instantiated then closing only the outermost is just fine.

The documentation on Closeable interface states that close method:

Closes this stream and releases any system resources associated with it.

The releasing system resources includes closing streams.

It also states that:

If the stream is already closed then invoking this method has no effect.

So if you close them explicitly afterwards, nothing wrong will happen.

Solution 4

I'd rather use try(...) syntax (Java 7), e.g.

try (OutputStream outputStream = new FileOutputStream(createdFile)) {
      ...
}

Solution 5

It will be fine if you only close the last stream - the close call will be send to the underlying streams, too.

View more solutions

Comments

[MadProgrammer]

While I agree with you, you might want to highlight the benefit of this approach and answer the point if the OP needs to close the child/inner streams

[T.J. Crowder]

This assumes no errors constructing the streams, which may or may not be true for the ones listed, but is not reliably true in general.

[T.J. Crowder]

See comment on Grzegorz Żur's answer.

[Jules]

+1 for noting that exceptions may be thrown during construction of the streams, although I'll note that realistically you're either going to get an out-of-memory exception or something equally serious (at which point it really doesn't matter if you close your streams, because your application is about to exit), or it will be GZIPOutputStream that throws an IOException; the rest of the constructors have no checked exceptions, and there are no other circumstances that are likely to produce a runtime exception.

[T.J. Crowder]

@Jules: Yeah, for these specific streams, indeed. It's more about good habits.

[TimK]

See T.J. Crowder's answer for situations where this could fail.

[Vishy]

@TimK can you provide an example of where the file is created but the stream later fails and what the consequence is. The risk of failure is extremely low and the impact is trivial. Not need to make the more complicated than it needs to be.

[Vishy]

I strongly disagree with the idea of increasing code complexity for an imagined problem, when there is no actual need to do this. While you demonstrated that there is a difference for a theoretical case, you didn't actually show what difference it would make in this case the OP is asking about. See my answer.

[Vishy]

@TimK His answer doesn't actually say where it could fail, just that it is different for an different case.

[T.J. Crowder]

@PeterLawrey: I strongly disagree with using bad habits or not depending on stream implementation. :-) This isn't a YAGNI/no-YAGNI distinction, it's about patterns that make for reliable code.

[Vishy]

@T.J.Crowder true, I mean the java.io library which comes with the JVM.

[T.J. Crowder]

@PeterLawrey: There's nothing above about not trusting java.io, either. Some streams -- generalizing, some resources -- throw from constructors. So making sure multiple resources are opened individually so they can be reliably closed if a subsequent resource throws is just a good habit, in my view. You can choose not to do it if you disagree, that's fine.

[T.J. Crowder]

@PeterLawrey: Done. I've softened the "you must" with "if you want try-with-resources to close them," and switched the example of a constructor throwing from OutputStreamWriter (whose constructors don't document any exceptions) to GZIPOutputStream (whose constructors do).

[Vishy]

@T.J.Crowder A reasonable qualification. Given the code for these classes has been available long before OpenJDK was open source and they are not that complicated, it would take a lot creativity/courage for a vendor to produce a JVM which worked significantly differently. i.e. even Android's DVM is the same and I wouldn't be surprised to find the code for C# is pretty similar. These classes are a Java version of what you might do in C (but adding the use of finalize())

[T.J. Crowder]

@PeterLawrey: So you advocate taking the time to look at the source code of an implementation for something documenting an exception, on a case-by-case basis, and then saying "Oh, well, it doesn't actually throw, so..." and saving a few characters of typing? We part company there, big time. :-) Moreover, I just looked, and this ain't theoretical: GZIPOutputStream's constructor writes a header to the stream. And so it can throw. So now the position is whether I think it's worth bothering to try to close the stream after writing threw. Yeah: I opened it, I should at least try to close it.

[T.J. Crowder]

GZIPOutputStream(OutputStream) documents IOException and, looking at the source, actually writes a header. So it isn't theoretical, that constructor can throw. You may feel it's okay to leave the underlying FileOutputStream open after writing to it throws. I don't.

[Vishy]

@T.J.Crowder I already covered that in second half of my answer ;) dig a little deeper and you will see why this is highly unlikely to be some thing you should be worrying about.

[T.J. Crowder]

@PeterLawrey: I think I'd rather just write the separate line than stop what I'm doing each time, dig up the source code, rummage around looking at what it does, and make a probability assessment.

[Vishy]

@T.J.Crowder this is fair approach. At some point you have to get on with actually write the code to do whatever your program is supposed to be doing. For an average developer this is entirely reasonable and many of your readers will be average developers so it is not bad advice to people who don't want to know what the code actually does, but I don't think of yourself as an average developer ;)

[T.J. Crowder]

@PeterLawrey: :-) V. kind.

[Vishy]

@T.J.Crowder Any one who is an experienced professional JavaScript developer (and other languages besides) I take my hat off to. I couldn't do it. ;)

[user207421]

You don't have to 'rely on the implementation' for any stream that extends a Filtered stream/reader, for which closing the wrapped stream is specified behaviour.

[T.J. Crowder]

@EJP: The variables thing isn't about the close, it's about the failing to open, and so the stream you're talking about doesn't even exist or get the chance to close its underlying one.

[robert_difalco]

Just to revisit this, the other issue is that if you are using a GZIPOutputStream on a file and don't call finish explicitly it will be called in it's close implementation. This isn't in a try...finally so if the finish/flush throws an exception then the underlying file handle will never be closed.

[Vishy]

@robert_difalco it is closed after a GC, most likely but maybe not ;)

[Maarten Bodewes]

Pfft, if we trust the constructors not to throw then a large part of reason for try with resources : the checking for null would not be needed. Create the variables for all the streams please, brilliant piece of advice (which I actually sought out).

[heez]

This is a fantastic writeup

[hmijail]

Seeing YAGNI used as an excuse to leave bugs in code is even more scary than depressing. Even more so when it's a subtle bug that you might realize when writing the code but might no longer remember once the bug is triggered.