8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

[Pavel Rappo]

> On 25 Jul 2019, at 21:01, Brian Burkhalter <brian.burkhalter at oracle.com> wrote:

<snip>

> This concern would be fixed by changing the proposed patch as
> 
> --- a/src/java.base/share/classes/java/io/SequenceInputStream.java
> +++ b/src/java.base/share/classes/java/io/SequenceInputStream.java
> @@ -91,13 +91,10 @@
>       * Continues reading in the next stream if an EOF is reached.
>       */
>      final void nextStream() throws IOException {
> -        try {
> -            if (in != null) {
> -                in.close();
> -            }
> -        } finally {
> -            peekNextStream();
> +        if (in != null) {
> +            in.close();
>          }
> +        peekNextStream();
>      }
>  
>      private void peekNextStream() {
> @@ -233,6 +230,7 @@
>                  } else {
>                      ioe.addSuppressed(e);
>                  }
> +                peekNextStream();
>              }
>          } while (in != null);
>          if (ioe != null) {
> 
> That is to say reverting the nextStream() change and adding peekNextStream() in the catch branch of close(). I actually had it this way in a version I did not post.
> 
> The scenario you suggest however would seem to be somewhat of a coding error by the caller. I would think that once a sequence stream were created the component streams should not be used.

When I filed this bug I was mainly concerned with the fact that `close` may end
up closing only some of the component streams. Both your original and amended
patches seem to fix this problem. However, I prefer the amended version. As it
both does the job and preserves the original behavior when consuming data from
the current component stream.

The scenario I mentioned in my previous email was about continuous attempts to
read from the SequenceInputStream, not about reading component streams
"out-of-band" (bypassing the aggregating SequenceInputStream). Sorry if I
didn't make myself clear.

This scenario doesn't seem to be a programming error, albeit a very strange use
case. I must admit I haven't seen any code in the wild doing so. Yet the spec
does not seem to prohibit this.

A nit on the accompanying test. Please consider this change:

diff -r 5a65ed97e38b test/jdk/java/io/SequenceInputStream/Close.java
--- a/test/jdk/java/io/SequenceInputStream/Close.java	Fri Jul 26 12:10:08 2019 +0100
+++ b/test/jdk/java/io/SequenceInputStream/Close.java	Fri Jul 26 12:12:29 2019 +0100
@@ -27,17 +27,15 @@
  */
 
 import java.io.ByteArrayInputStream;
-import java.io.InputStream;
 import java.io.IOException;
 import java.io.SequenceInputStream;
-import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Enumeration;
 import java.util.List;
 
 public class Close {
 
-    public static void main(String[] argv) throws IOException {
+    public static void main(String[] argv) {
         byte[] buf = new byte[42];
 
         List<CloseableBAOS> list = List.of(new CloseableBAOS(buf),
@@ -74,12 +72,12 @@
 
     static class CloseableBAOS extends ByteArrayInputStream{
         private boolean closed;
-        private int numCloses = 0;
 
         CloseableBAOS(byte[] buf) {
             super(buf);
         }
 
+        @Override
         public void close() throws IOException {
             closed = true;
             throw new IOException();