From 6c9ac509c4b515a15dbb790309bc8469c911d308 Mon Sep 17 00:00:00 2001 From: SeongEon Jo Date: Sat, 23 Dec 2023 06:25:32 +0900 Subject: [PATCH] SharedFileInputStream should comply with spec (#695) --- .../mail/util/SharedFileInputStream.java | 62 +++++------------- .../mail/util/SharedFileInputStreamTest.java | 65 +++++++++++++++++++ doc/release/CHANGES.txt | 5 ++ www/docs/COMPAT.txt | 9 +++ 4 files changed, 97 insertions(+), 44 deletions(-) create mode 100644 api/src/test/java/jakarta/mail/util/SharedFileInputStreamTest.java diff --git a/api/src/main/java/jakarta/mail/util/SharedFileInputStream.java b/api/src/main/java/jakarta/mail/util/SharedFileInputStream.java index 5ec38da1..e9c01589 100644 --- a/api/src/main/java/jakarta/mail/util/SharedFileInputStream.java +++ b/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 SharedFileInputStream is a @@ -34,15 +35,6 @@ * A RandomAccessFile object is used to * access the file data.

* - * Note that when the SharedFileInputStream is closed, - * all streams created with the newStream - * 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(); } diff --git a/api/src/test/java/jakarta/mail/util/SharedFileInputStreamTest.java b/api/src/test/java/jakarta/mail/util/SharedFileInputStreamTest.java new file mode 100644 index 00000000..a1f707bb --- /dev/null +++ b/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(); + } + } +} \ No newline at end of file diff --git a/doc/release/CHANGES.txt b/doc/release/CHANGES.txt index 961ddbd2..4e26a970 100644 --- a/doc/release/CHANGES.txt +++ b/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) diff --git a/www/docs/COMPAT.txt b/www/docs/COMPAT.txt index a7bbedef..e627ca96 100644 --- a/www/docs/COMPAT.txt +++ b/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