On Thu, 2011-04-07 at 16:02 -0700, Xueming Shen wrote:
> It appears it might not be necessary to do the finalize() in
> ZipFileInflaterInputStream. The ZipFileInflaterInputStream itself does
> not directly hold any native resource by itself that needs to be
> released at the end of its life circle, if not closed explicitly. The
> native resource/memory that need to be taken care of are held by its
> fields "inf" and "zfin", which should be finalized by the
> corresponding finalize() of their own classes (again, if not closed
> explicitly), when their outer ZFIIS object is unreachable. The Inflater
> class has its own finalize() implemented already to invoke its cleanup
> method end(), so the only thing need to be addressed is to add the
> finalize() into ZipFileInputStream class to call its close(), strictly
> speaking this issue is not the regression caused by #6735255, we have
> this "leak" before #6735255.
>
> Also, would you like to consider to use WeakHeapMap<InputStream, Void>
> instead of handling all the weak reference impl by yourself, the bonus
> would be that the stalled entries might be cleaned up more frequently.
>
Hi Sherman,
Thanks for your continuing analysis of this change.
I concur with your assessment above, and agree that making the suggested
modifications to the changeset results in the code being simpler and
clearer.
Please find below an updated changeset incorporating these suggestions
(and rebased off jdk7-b136),
Let me know if you need anything else to progress this fix forward,
Thanks,
Neil
--
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
# HG changeset patch
# User Neil Richards <neil.richa...@ngmr.net>, <neil_richa...@uk.ibm.com>
# Date 1300289208 0
# Branch zip-heap
# Node ID 6e5ae64dd0437327f9d20f72c55bfdef6649bb7d
# Parent aa13e7702cd9d8aca9aa38f1227f966990866944
7031076: Retained ZipFile InputStreams increase heap demand
Summary: Allow unreferenced ZipFile InputStreams to be finalized, GC'd
Contributed-by: <neil.richa...@ngmr.net>
diff -r aa13e7702cd9 -r 6e5ae64dd043
src/share/classes/java/util/zip/ZipFile.java
--- a/src/share/classes/java/util/zip/ZipFile.java Tue Mar 29 20:19:55
2011 -0700
+++ b/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:26:48
2011 +0000
@@ -33,9 +33,11 @@
import java.nio.charset.Charset;
import java.util.Vector;
import java.util.Enumeration;
+import java.util.Map;
import java.util.Set;
-import java.util.HashSet;
+import java.util.Iterator;
import java.util.NoSuchElementException;
+import java.util.WeakHashMap;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
import static java.util.zip.ZipConstants64.*;
@@ -315,7 +317,7 @@
private static native void freeEntry(long jzfile, long jzentry);
// the outstanding inputstreams that need to be closed.
- private Set<InputStream> streams = new HashSet<>();
+ private final Map<InputStream, Void> streams = new WeakHashMap<>();
/**
* Returns an input stream for reading the contents of the specified
@@ -351,51 +353,17 @@
switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ 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);
+ InputStream is =
+ new ZipFileInflaterInputStream(in, getInflater(),
+ (int)size);
+ streams.put(is, null);
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,6 +371,49 @@
}
}
+ private class ZipFileInflaterInputStream extends InflaterInputStream {
+ private boolean isClosed = 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 (!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 = 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 (isClosed)
+ return 0;
+ long avail = zfin.size() - inf.getBytesWritten();
+ return (avail > (long) Integer.MAX_VALUE ?
+ Integer.MAX_VALUE : (int) avail);
+ }
+ }
+
/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
@@ -543,11 +554,14 @@
synchronized (this) {
closeRequested = true;
- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
+ Iterator<InputStream> streamsIterator =
+ streams.keySet().iterator();
+ while (streamsIterator.hasNext()) {
+ InputStream is = streamsIterator.next();
+ if (null != is) {
is.close();
+ }
+ streamsIterator.remove();
}
if (jzfile != 0) {
@@ -684,9 +698,12 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}
diff -r aa13e7702cd9 -r 6e5ae64dd043
test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java Wed Mar
16 15:26:48 2011 +0000
@@ -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.richa...@ngmr.net>, <neil_richa...@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;
+ }
+}
