On Thu, 2011-04-14 at 14:48 -0700, Xueming Shen wrote:
> Opinion? anything I overlooked/missed?
Hi Sherman,
Thanks once more for all your help and advice on this - I'm in favour of
almost all of what you suggest. :-)
I think it's worthwhile trying to clear 'streams' entries from a
finalize method in ZFIIS, to help ensure they are cleared in a timely
manner.
I don't think the Inflater objects need to be held softly in
'inflaterCache', as they will have been reset() at the point they are
put into that cache (in releaseInflater()).
(I was holding them softly due to a worry over any delay in clearing
their 'buf' reference, which is done when reset() is called. Now that
the 'streams' entries are being cleared from close() and finalize() -
ie. up front - I think this worry is adequately addressed.)
I'm worried about changing the object 'streams' refers to in
ZipFile.close(), as threads are using that object to synchronize
against.
I don't believe it is currently giving the guarantee against
ConcurrentModificationException being seen from the iterator that I
believe you intend it to be, as other threads, calling (for example)
ZFIIS.close() at the same time will not be guaranteed to see the update
to the value of 'streams'.
Instead, I've modified this code so that a real copy of 'streams' is
produced, by calling 'new HashMap<>(streams)'. It is this copy that is
iterated through to close() the InputStreams and end() the Inflaters.
Once the copy is obtained, 'streams' can be safely clear()'d.
Because it is not clear that a Collections.synchronizedMap will give
consistent results when fed into the constructor of HashMap, I've used
explicit synchronization on 'streams' instead, to ensure 'streams' isn't
modified during the construction of 'copy'.
As each of the calls to InputStream.close() will synchronize on
'streams' to call its remove() method, I've held that monitor whilst
those calls are made.
Other minor tweaks are changing 'isClosed' fields to
'closeRequested' (to better describe their use now), changing
'ZipFile.closeRequested' to be volatile (so it can be checked before
synchronization, and so that it conforms to the pattern now established
elsewhere in the file), and reducing the synchronization block in
releaseInflater() (only the call to 'inflateCache.add()' need be
protected by synchronization on 'inflaterCache').
Please find below the resultant changeset,
Let me know what you think,
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 c1c7b2dfa0091938bebc0727224690afc892a0b7
# 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 c1c7b2dfa009
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
@@ -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,93 @@
}
}
+ 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() {
+ synchronized (streams) {
+ streams.remove(this);
+ }
+ }
+ }
+
/*
* 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 +570,32 @@
* @throws IOException if an I/O error has occurred
*/
public void close() throws IOException {
+ if (closeRequested)
+ return;
+ closeRequested = true;
+
synchronized (this) {
- closeRequested = true;
+ // 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();
+ }
+ }
+ }
+ }
- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
- is.close();
+ // Release cached inflaters
+ Inflater inf;
+ synchronized (inflaterCache) {
+ while (null != (inf = inflaterCache.poll())) {
+ inf.end();
+ }
}
if (jzfile != 0) {
@@ -556,23 +604,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 +649,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 +717,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();
+ }
}
diff -r aa13e7702cd9 -r c1c7b2dfa009
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;
+ }
+}
