CODEWITHSUNDEEP
java
Evgeniy Dorofeev
Evgeniy Dorofeev

Reputation: 136042

Is java.io.BufferedOutputStream safe to use?

At first glance this code seems completely OK

BufferedOutputStream bout = new BufferedOutputStream(new FileOutputStream("1.txt"));
byte[] bytes = new byte[4096];
bout.write(bytes);
bout.close();

but if we take a closer look we will see that close() is implemented as follows

public void close() throws IOException {
    try {
      flush();
    } catch (IOException ignored) {
    }
    out.close();
}

Is it possible that due to flush() errors are ignored data may be lost and the program will not notice it? There is no mentioning of any danger in FilterOutputStream.close (where BufferedOutputStream inherits close() from) API.

UPDATE: To simulate IO error during close() I changed the test to write to a Flash memory, added a 5 secs sleep before bout.close() and while the test was sleeping I removed the Flash from USB. The test finished without exceptions, but when I inserted the Flash and checked it - 1.txt was not there.

Then I overrode close() 

    BufferedOutputStream bout = new BufferedOutputStream(new FileOutputStream("g:/1.txt")) {
        @Override
        public void close() throws IOException {
            flush();
            super.close();
        }
    };

and ran the test again and got

Exception in thread "main" java.io.FileNotFoundException: g:\1.txt (The system cannot the specified path)
    at java.io.FileOutputStream.open(Native Method)
    at java.io.FileOutputStream.<init>(FileOutputStream.java:212)
    at java.io.FileOutputStream.<init>(FileOutputStream.java:104)
    at test.Test1.main(Test1.java:10)

Upvotes: 19

Views: 3996

Answers (2)

OrangeDog
OrangeDog

Reputation: 38777

In OpenJDK 11, the FilterOutputStream#close() method (which BufferedOutputStream inherits) is a lot more complex:

public void close() throws IOException {
    if (closed) {
        return;
    }
    synchronized (closeLock) {
        if (closed) {
            return;
        }
        closed = true;
    }

    Throwable flushException = null;
    try {
        flush();
    } catch (Throwable e) {
        flushException = e;
        throw e;
    } finally {
        if (flushException == null) {
            out.close();
        } else {
            try {
                out.close();
            } catch (Throwable closeException) {
               // evaluate possible precedence of flushException over closeException
               if ((flushException instanceof ThreadDeath) &&
                   !(closeException instanceof ThreadDeath)) {
                   flushException.addSuppressed(closeException);
                   throw (ThreadDeath) flushException;
               }

                if (flushException != closeException) {
                    closeException.addSuppressed(flushException);
                }

                throw closeException;
            }
        }
    }
}

As you can see, if the flush() method throws then the exception is now correctly propagated to the caller.

Together with the added locking, BufferedOutputStream should now be as safe as possible.

Upvotes: 0

pcalcao
pcalcao

Reputation: 15990

As it is, I would reason that calling close can indeed make you lose data, since that potential IOException is being silently ignored (who on earth knows what went through the developers minds to do that...).

A decent alternative, although it does put the effort on the side of the programmer, is to call flush explicitly before close (handling the potential IOException correctly), like mentioned in a comment by @Tom, particularly in a try/finally block.

This problem can be further exacerbated in Java7, due to AutoCloseable objects, since you won't explicitly call the close() method and this kind of work-around is even easier to slip by.

Upvotes: 6

Related Questions