ZipOutputStream.close() should always close the wrapped stream

ADDITIONAL SYSTEM INFORMATION :
Checked against the latest source as available on github - https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java - and various older java versions. This problem has existed as far as I can tell forever.

A DESCRIPTION OF THE PROBLEM :
filterstreams (such as PrintWriter, Scanner, BufferedInputStream, and ZipOutputStream) all are always `AutoClosable`, and universally, their `close()` methods are defined as always closing their underlying stream.

This raises a tricky question which specs do not cover: HOW should a filter stream go about it?

OpenJDK's own implementations are inconsistent. Some thought indicates that ZipOutputStream is dangerously wrong and BufferedOutputStream is right. The issue is:

When calling `close()` on a filterstream, _and the act of cleaning up and flushing any buffered data from the filterstream down to the underlying stream throws something_, should the filterstream _still_ close the underlying stream?

All main filterstreams say YES. That's because they extend `FilterOutputStream`, and its close method uses a `try/finally` construct to ensure the underlying stream is closed _even if_ its `flush()` operation throws. as per this snippet from its close() implementation (see https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/io/FilterOutputStream.java#L171).

This is sensible behaviour.

ZipOutputStream, however, says NO. And it certainly does a lot of work (plenty of opportunity for an exception to occur here): It closes the current file which flushes data down, and will then write the zip's "contents" (the metadata about the file names, which deflate algorithm is used, and so on - these are at the end of a zip file, not at the beginning). Only then does it close the underlying stream, but, crucially, all that cleanup stuff _is not_ guarded with try/finally.

Hence, if you `close()` a ZipOutputStream and the writing of the metadata causes an exception to occur, the underlying stream remains open. This runs counter to expectation, counter to common code style, and counter to how just about all other places in the java core libraries do it.

In particular, this is somewhat common, certainly in many tutorials:

try (var out = new BufferedOutputStream(new FileOutputStream(...)) {
}

This code is a resource leak __unless__ the filter guarantees that it cannot throw any exceptions if handed a live stream in its constructor, and the close() method runs the underlying stream's close method regardless of any exceptions caused by its flush() operation. Fortunately, that's how BufferedOutputStream really does work so the above code is no resource leak. But, use ZipOutputStream instead, and it is.

I suggest the following actions are taken:

1. ZipOutputStream is fixed - the work is actually done by superclass DeflaterOutputStream's close() method. It should add a try/finally around its clean up work and call the underlying stream's `close()` method in the finally block. Or use a construct such as try (underlying) { ... } to rely on the try-with-resources lang feature to do it.

2. InputStream, OutputStream, Reader, and Writer's close() method javadoc is updated to include the notion that filterstreams are a thing, and that these should safely close all underlying resources.



STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Reading the source code of BufferedOutputStream and ZipOutputStream's close() methods is a much simpler way to verify this bug exists. However, if code is required that shows it, create file 'Test.java' and paste the content from the 'source code for an executable test case' into it. Then run `javac Test.java; java Test` - the source code is entirely self contained and compiles and runs on any JDK without requiring any dependencies and on every version from java 1.6 and up, including the v21 early release.

This prints:

Test 1: BufferedOutputStream
OKAY: Close has been called
Test 2: ZipOutputStream
Tests complete

Showing how the ZipOutputStream has failed to close the underlying stream, whereas BufferedOutputStream does do it.


EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
The self-contained test case in the 'steps to reproduce' section should print:

Test 1: BufferedOutputStream
OKAY: Close has been called
Test 2: ZipOutputStream
OKAY: Close has been called
Tests complete

ACTUAL -
The self-contained test case in the 'steps to reproduce' section should print:

Test 1: BufferedOutputStream
OKAY: Close has been called
Test 2: ZipOutputStream
Tests complete

i.e.: the underlying stream isn't closed when it should have been.

---------- BEGIN SOURCE ----------
import java.io.*;
import java.util.zip.*;
import java.util.concurrent.atomic.*;

class Test {
  public static void main(String[] args) throws Exception {
    var crash = new AtomicBoolean();
    OutputStream raw = new ByteArrayOutputStream() {
      public void write(int b) {
        if (crash.get()) throw new Error();
        super.write(b);
      }

      public void write(byte[] b, int off, int len) {
        if (crash.get()) throw new Error();
        super.write(b, off, len);
      }

      public void close() {
        System.out.println("OKAY: Close has been called");
      }
    };

    System.out.println("Test 1: BufferedOutputStream");
    var buffered = new BufferedOutputStream(raw);
    buffered.write(10);
    crash.set(true);
    try {
      buffered.close(); // does print!
    } catch (Throwable ignore) {}

    crash.set(false);
    System.out.println("Test 2: ZipOutputStream");
    var zip = new ZipOutputStream(raw);
    zip.putNextEntry(new ZipEntry("test.txt"));
    zip.write(10);
    crash.set(true);
    try {
      zip.close();
    } catch (Throwable ignore) {}
    System.out.println("Tests complete");
  }
}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
The common style is to write something like:

try (
  var raw = new FileOutputStream(path);
  var zip = new ZipOutputStream(raw);
  ) {
    code here
}

and that style means the underlying resource is closed. Whilst this style is common, it isn't quite ubiquitous; this style shows up in lots of code, and various tutorials of the JDK itself do this instead:

try (var zip = new ZipOutputStream(new FileOutputStream(path)) {
}

and that code is broken (resource leak). The workaround is to update all your linter tools and to stop writing it that way (write it like the first snippet) until this bug is fixed.


FREQUENCY : always