SharedFileInputStream should comply with spec (#695)

api/src/main/java/jakarta/mail/util/SharedFileInputStream.java

@@ -23,6 +23,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.RandomAccessFile;
+import java.util.Objects;
 
 /**
  * A <code>SharedFileInputStream</code> is a
@@ -34,15 +35,6 @@
  * A <code>RandomAccessFile</code> object is used to
  * access the file data. <p>
  *
- * Note that when the SharedFileInputStream is closed,
- * all streams created with the <code>newStream</code>
- * method are also closed.  This allows the creator of the
- * SharedFileInputStream object to control access to the
- * underlying file and ensure that it is closed when
- * needed, to avoid leaking file descriptors.  Note also
- * that this behavior contradicts the requirements of
- * SharedInputStream and may change in a future release.
- *
  * @author Bill Shannon
  * @since JavaMail 1.4
  */
@@ -78,12 +70,6 @@ public class SharedFileInputStream extends BufferedInputStream
      */
     protected long datalen;
 
-    /**
-     * True if this is a top level stream created directly by "new".
-     * False if this is a derived stream created by newStream.
-     */
-    private boolean master = true;
-
     /**
      * A shared class that keeps track of the references
      * to a particular file so it can be closed when the
@@ -111,22 +97,8 @@ public synchronized void close() throws IOException {
                 in.close();
         }
 
-        public synchronized void forceClose() throws IOException {
-            if (cnt > 0) {
-                // normal case, close exceptions propagated
-                cnt = 0;
-                in.close();
-            } else {
-                // should already be closed, ignore exception
-                try {
-                    in.close();
-                } catch (IOException ioex) {
-                }
-            }
-        }
-
         @Override
-        protected void finalize() throws Throwable {
+        protected synchronized void finalize() throws Throwable {
             try {
                 in.close();
             } finally {
@@ -214,7 +186,6 @@ private void init(SharedFile sf, int size) throws IOException {
     private SharedFileInputStream(SharedFile sf, long start, long len,
                                   int bufsize) {
         super(null);
-        this.master = false;
         this.sf = sf;
         this.in = sf.open();
         this.start = start;
@@ -465,14 +436,12 @@ public void close() throws IOException {
         if (in == null)
             return;
         try {
-            if (master)
-                sf.forceClose();
-            else
-                sf.close();
+            sf.close();
         } finally {
             sf = null;
             in = null;
             buf = null;
+            Objects.requireNonNull(this); //TODO: replace with Reference.reachabilityFence
         }
     }
 
@@ -505,14 +474,19 @@ public long getPosition() {
      */
     @Override
     public synchronized InputStream newStream(long start, long end) {
-        if (in == null)
-            throw new RuntimeException("Stream closed");
-        if (start < 0)
-            throw new IllegalArgumentException("start < 0");
-        if (end == -1)
-            end = datalen;
-        return new SharedFileInputStream(sf,
-                this.start + start, end - start, bufsize);
+        try {
+            if (in == null)
+                throw new RuntimeException("Stream closed");
+            if (start < 0)
+                throw new IllegalArgumentException("start < 0");
+            if (end == -1)
+                end = datalen;
+
+            return new SharedFileInputStream(sf,
+                    this.start + start, end - start, bufsize);
+        } finally {
+            Objects.requireNonNull(this); //TODO: replace with Reference.reachabilityFence
+        }
     }
 
     // for testing...
@@ -537,7 +511,7 @@ public static void main(String[] argv) throws Exception {
      * Force this stream to close.
      */
     @Override
-    protected void finalize() throws Throwable {
+    protected synchronized void finalize() throws Throwable {
         super.finalize();
         close();
     }

api/src/test/java/jakarta/mail/util/SharedFileInputStreamTest.java

@@ -0,0 +1,65 @@
+/*
+ * Copyright (c) 2021, 2023 Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v. 2.0, which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * This Source Code may also be made available under the following Secondary
+ * Licenses when the conditions for such availability set forth in the
+ * Eclipse Public License v. 2.0 are satisfied: GNU General Public License,
+ * version 2 with the GNU Classpath Exception, which is available at
+ * https://www.gnu.org/software/classpath/license.html.
+ *
+ * SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
+ */
+
+package jakarta.mail.util;
+
+import org.junit.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+
+import static org.junit.Assert.fail;
+
+/**
+ * Please note:
+ * In version 2.1.2 Final Release, a difference in test results was observed based on the choice of assertion method.
+ * Invoking stream.read() directly yielded distinct outcomes compared to using Assertions.assertDoesNotThrow() from JUnit 5.
+ * This divergence is likely attributed to scope of the code.
+ * After the patch, this issue did not reoccur. However, it remains essential to be attentive
+ * If any changes are deemed necessary, please ensure a comprehensive review and thorough testing to uphold the original behavior.
+ */
+public class SharedFileInputStreamTest {
+
+    @Test
+    public void testChild() throws Exception {
+        File file = File.createTempFile(SharedFileInputStreamTest.class.getName(), "testChild");
+
+        try (InputStream childStream = new SharedFileInputStream(file).newStream(0, -1)) {
+            System.gc();
+            childStream.read();
+        } catch (IOException e) {
+            fail("IOException is not expected");
+        } finally {
+            file.delete();
+        }
+    }
+
+
+    @Test
+    public void testGrandChild() throws Exception {
+        File file = File.createTempFile(SharedFileInputStreamTest.class.getName(), "testGrandChild");
+
+        try (InputStream grandChild = ((SharedFileInputStream) new SharedFileInputStream(file).newStream(0, -1)).newStream(0, -1)) {
+            System.gc();
+            grandChild.read();
+        } catch (IOException e) {
+            fail("IOException is not expected");
+        } finally {
+            file.delete();
+        }
+    }
+}

doc/release/CHANGES.txt

@@ -19,6 +19,11 @@ Bug IDs that start with "G" can be found in the GlassFish Issue Tracker
 Seven digit bug numbers are from the old Sun bug database, which is no
 longer available.
 
+          CHANGES IN THE 2.1.3 RELEASE
+          ----------------------------
+E  695  SharedFileInputStream should comply with spec
+
+
 		  CHANGES IN THE 2.1.2 RELEASE
           ----------------------------
 E  629  jakarta.mail-api-2.1.0.jar does not work in OSGi environment (hk2servicelocator)

www/docs/COMPAT.txt

@@ -4,6 +4,15 @@
 		    Jakarta Mail API 2.0.1 release
 		    ------------------------------
 
+-- Jakarta Mail 2.1.3 --
+
+- SharedFileInputStream should comply with spec
+
+    The root SharedFileInputStream no longer closes all streams
+    created with the newStream. This behavior was not compliant with
+    the contract specified in the SharedInputStream interface
+    which specifies that all streams must be closed before the shared resource is closed.
+
 -- Jakarta Mail 2.0.0 --
 
 The Jakarta Mail 2.0 specification is the successor of the Jakarta