7031076: Retained ZipFile InputStreams increase heap demand
Summary: Allow unreferenced ZipFile InputStreams to be finalized, GC'd
Reviewed-by: sherman, dholmes
Contributed-by: neil.richards@ngmr.net
--- a/jdk/src/share/classes/java/util/zip/ZipFile.java Mon Apr 18 12:07:29 2011 -0400
+++ b/jdk/src/share/classes/java/util/zip/ZipFile.java Mon Apr 18 10:51:19 2011 -0700
@@ -31,11 +31,13 @@
import java.io.EOFException;
import java.io.File;
import java.nio.charset.Charset;
-import java.util.Vector;
+import java.util.ArrayDeque;
+import java.util.Deque;
import java.util.Enumeration;
-import java.util.Set;
-import java.util.HashSet;
+import java.util.HashMap;
+import java.util.Map;
import java.util.NoSuchElementException;
+import java.util.WeakHashMap;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
import static java.util.zip.ZipConstants64.*;
@@ -54,7 +56,7 @@
private long jzfile; // address of jzfile data
private String name; // zip file name
private int total; // total number of entries
- private boolean closeRequested;
+ private volatile boolean closeRequested = false;
private static final int STORED = ZipEntry.STORED;
private static final int DEFLATED = ZipEntry.DEFLATED;
@@ -314,8 +316,9 @@
// freeEntry releases the C jzentry struct.
private static native void freeEntry(long jzfile, long jzentry);
- // the outstanding inputstreams that need to be closed.
- private Set<InputStream> streams = new HashSet<>();
+ // the outstanding inputstreams that need to be closed,
+ // mapped to the inflater objects they use.
+ private final Map<InputStream, Inflater> streams = new WeakHashMap<>();
/**
* Returns an input stream for reading the contents of the specified
@@ -351,51 +354,21 @@
switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ synchronized (streams) {
+ streams.put(in, null);
+ }
return in;
case DEFLATED:
- final ZipFileInputStream zfin = in;
// MORE: Compute good size for inflater stream:
long size = getEntrySize(jzentry) + 2; // Inflater likes a bit of slack
if (size > 65536) size = 8192;
if (size <= 0) size = 4096;
- InputStream is = new InflaterInputStream(zfin, getInflater(), (int)size) {
- private boolean isClosed = false;
-
- public void close() throws IOException {
- if (!isClosed) {
- super.close();
- releaseInflater(inf);
- isClosed = true;
- }
- }
- // Override fill() method to provide an extra "dummy" byte
- // at the end of the input stream. This is required when
- // using the "nowrap" Inflater option.
- protected void fill() throws IOException {
- if (eof) {
- throw new EOFException(
- "Unexpected end of ZLIB input stream");
- }
- len = this.in.read(buf, 0, buf.length);
- if (len == -1) {
- buf[0] = 0;
- len = 1;
- eof = true;
- }
- inf.setInput(buf, 0, len);
- }
- private boolean eof;
-
- public int available() throws IOException {
- if (isClosed)
- return 0;
- long avail = zfin.size() - inf.getBytesWritten();
- return avail > (long) Integer.MAX_VALUE ?
- Integer.MAX_VALUE : (int) avail;
- }
- };
- streams.add(is);
+ Inflater inf = getInflater();
+ InputStream is =
+ new ZipFileInflaterInputStream(in, inf, (int)size);
+ synchronized (streams) {
+ streams.put(is, inf);
+ }
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,36 +376,91 @@
}
}
+ private class ZipFileInflaterInputStream extends InflaterInputStream {
+ private volatile boolean closeRequested = false;
+ private boolean eof = false;
+ private final ZipFileInputStream zfin;
+
+ ZipFileInflaterInputStream(ZipFileInputStream zfin, Inflater inf,
+ int size) {
+ super(zfin, inf, size);
+ this.zfin = zfin;
+ }
+
+ public void close() throws IOException {
+ if (closeRequested)
+ return;
+ closeRequested = true;
+
+ super.close();
+ Inflater inf;
+ synchronized (streams) {
+ inf = streams.remove(this);
+ }
+ if (inf != null) {
+ releaseInflater(inf);
+ }
+ }
+
+ // Override fill() method to provide an extra "dummy" byte
+ // at the end of the input stream. This is required when
+ // using the "nowrap" Inflater option.
+ protected void fill() throws IOException {
+ if (eof) {
+ throw new EOFException("Unexpected end of ZLIB input stream");
+ }
+ len = in.read(buf, 0, buf.length);
+ if (len == -1) {
+ buf[0] = 0;
+ len = 1;
+ eof = true;
+ }
+ inf.setInput(buf, 0, len);
+ }
+
+ public int available() throws IOException {
+ if (closeRequested)
+ return 0;
+ long avail = zfin.size() - inf.getBytesWritten();
+ return (avail > (long) Integer.MAX_VALUE ?
+ Integer.MAX_VALUE : (int) avail);
+ }
+
+ protected void finalize() throws Throwable {
+ close();
+ }
+ }
+
/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
*/
private Inflater getInflater() {
- synchronized (inflaters) {
- int size = inflaters.size();
- if (size > 0) {
- Inflater inf = (Inflater)inflaters.remove(size - 1);
- return inf;
- } else {
- return new Inflater(true);
+ Inflater inf;
+ synchronized (inflaterCache) {
+ while (null != (inf = inflaterCache.poll())) {
+ if (false == inf.ended()) {
+ return inf;
+ }
}
}
+ return new Inflater(true);
}
/*
* Releases the specified inflater to the list of available inflaters.
*/
private void releaseInflater(Inflater inf) {
- synchronized (inflaters) {
- if (inf.ended())
- return;
+ if (false == inf.ended()) {
inf.reset();
- inflaters.add(inf);
+ synchronized (inflaterCache) {
+ inflaterCache.add(inf);
+ }
}
}
// List of available Inflater objects for decompression
- private Vector inflaters = new Vector();
+ private Deque<Inflater> inflaterCache = new ArrayDeque<>();
/**
* Returns the path name of the ZIP file.
@@ -540,14 +568,32 @@
* @throws IOException if an I/O error has occurred
*/
public void close() throws IOException {
- synchronized (this) {
- closeRequested = true;
+ if (closeRequested)
+ return;
+ closeRequested = true;
- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
- is.close();
+ synchronized (this) {
+ // Close streams, release their inflaters
+ synchronized (streams) {
+ if (false == streams.isEmpty()) {
+ Map<InputStream, Inflater> copy = new HashMap<>(streams);
+ streams.clear();
+ for (Map.Entry<InputStream, Inflater> e : copy.entrySet()) {
+ e.getKey().close();
+ Inflater inf = e.getValue();
+ if (inf != null) {
+ inf.end();
+ }
+ }
+ }
+ }
+
+ // Release cached inflaters
+ Inflater inf;
+ synchronized (inflaterCache) {
+ while (null != (inf = inflaterCache.poll())) {
+ inf.end();
+ }
}
if (jzfile != 0) {
@@ -556,23 +602,13 @@
jzfile = 0;
close(zf);
-
- // Release inflaters
- synchronized (inflaters) {
- int size = inflaters.size();
- for (int i = 0; i < size; i++) {
- Inflater inf = (Inflater)inflaters.get(i);
- inf.end();
- }
- }
}
}
}
-
/**
- * Ensures that the <code>close</code> method of this ZIP file is
- * called when there are no more references to it.
+ * Ensures that the system resources held by this ZipFile object are
+ * released when there are no more references to it.
*
* <p>
* Since the time when GC would invoke this method is undetermined,
@@ -611,6 +647,7 @@
* (possibly compressed) zip file entry.
*/
private class ZipFileInputStream extends InputStream {
+ private volatile boolean closeRequested = false;
protected long jzentry; // address of jzentry data
private long pos; // current position within entry data
protected long rem; // number of remaining bytes within entry
@@ -678,15 +715,25 @@
}
public void close() {
+ if (closeRequested)
+ return;
+ closeRequested = true;
+
rem = 0;
synchronized (ZipFile.this) {
if (jzentry != 0 && ZipFile.this.jzfile != 0) {
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
+ }
+ synchronized (streams) {
streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java Mon Apr 18 10:51:19 2011 -0700
@@ -0,0 +1,148 @@
+/*
+ * Copyright (c) 2011 Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * Portions Copyright (c) 2011 IBM Corporation
+ */
+
+/*
+ * @test
+ * @bug 7031076
+ * @summary Allow stale InputStreams from ZipFiles to be GC'd
+ * @author Neil Richards <neil.richards@ngmr.net>, <neil_richards@uk.ibm.com>
+ */
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.InputStream;
+import java.util.Enumeration;
+import java.util.HashSet;
+import java.util.Random;
+import java.util.Set;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipFile;
+import java.util.zip.ZipOutputStream;
+
+public class ClearStaleZipFileInputStreams {
+ private static final int ZIP_ENTRY_NUM = 5;
+
+ private static final byte[][] data;
+
+ static {
+ data = new byte[ZIP_ENTRY_NUM][];
+ Random r = new Random();
+ for (int i = 0; i < ZIP_ENTRY_NUM; i++) {
+ data[i] = new byte[1000];
+ r.nextBytes(data[i]);
+ }
+ }
+
+ private static File createTestFile(int compression) throws Exception {
+ File tempZipFile =
+ File.createTempFile("test-data" + compression, ".zip");
+ tempZipFile.deleteOnExit();
+
+ ZipOutputStream zos =
+ new ZipOutputStream(new FileOutputStream(tempZipFile));
+ zos.setLevel(compression);
+
+ try {
+ for (int i = 0; i < ZIP_ENTRY_NUM; i++) {
+ String text = "Entry" + i;
+ ZipEntry entry = new ZipEntry(text);
+ zos.putNextEntry(entry);
+ try {
+ zos.write(data[i], 0, data[i].length);
+ } finally {
+ zos.closeEntry();
+ }
+ }
+ } finally {
+ zos.close();
+ }
+
+ return tempZipFile;
+ }
+
+ private static void startGcInducingThread(final int sleepMillis) {
+ final Thread gcInducingThread = new Thread() {
+ public void run() {
+ while (true) {
+ System.gc();
+ try {
+ Thread.sleep(sleepMillis);
+ } catch (InterruptedException e) { }
+ }
+ }
+ };
+
+ gcInducingThread.setDaemon(true);
+ gcInducingThread.start();
+ }
+
+ public static void main(String[] args) throws Exception {
+ startGcInducingThread(500);
+ runTest(ZipOutputStream.DEFLATED);
+ runTest(ZipOutputStream.STORED);
+ }
+
+ private static void runTest(int compression) throws Exception {
+ ReferenceQueue<InputStream> rq = new ReferenceQueue<>();
+
+ System.out.println("Testing with a zip file with compression level = "
+ + compression);
+ File f = createTestFile(compression);
+ try {
+ ZipFile zf = new ZipFile(f);
+ try {
+ Set<Object> refSet = createTransientInputStreams(zf, rq);
+
+ System.out.println("Waiting for 'stale' input streams from ZipFile to be GC'd ...");
+ System.out.println("(The test will hang on failure)");
+ while (false == refSet.isEmpty()) {
+ refSet.remove(rq.remove());
+ }
+ System.out.println("Test PASSED.");
+ System.out.println();
+ } finally {
+ zf.close();
+ }
+ } finally {
+ f.delete();
+ }
+ }
+
+ private static Set<Object> createTransientInputStreams(ZipFile zf,
+ ReferenceQueue<InputStream> rq) throws Exception {
+ Enumeration<? extends ZipEntry> zfe = zf.entries();
+ Set<Object> refSet = new HashSet<>();
+
+ while (zfe.hasMoreElements()) {
+ InputStream is = zf.getInputStream(zfe.nextElement());
+ refSet.add(new WeakReference<InputStream>(is, rq));
+ }
+
+ return refSet;
+ }
+}