Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
Neil Richards
neil.richards at ngmr.net
Sun Apr 10 17:28:02 UTC 2011
On Fri, 2011-04-08 at 20:53 -0700, Xueming Shen wrote:
> Regression test /test/java/util/zip/ZipFile/FinalizeInflater.java failed
> with this fix.
>
> The possible solution I can think of now is to do nothing but simply
> return the inflater back to the cache, but do an additional sanity
> check when "check out"
>
> private Inflater getInflater() {
>
> synchronized (inflaters) {
> int size = inflaters.size();
> while ((size = inflaters.size())> 0) {
> Inflater inf = inflaters.remove(size - 1);
> if (!inf.ended())
> return inf;
> }
> return new Inflater(true);
> }
> }
>
>
> http://cr.openjdk.java.net/~sherman/7031076/webrev/
>
> What do you think? have a better idea?
Whilst checking on "check out" might hope to reduce the timing window, I
fear that doing so will not entirely eliminate it.
(The timing window is essentially between the point the inflater object
is placed on the finalization queue for finalization and the point at
which that finalization has actually occurred. In that window, if any
implementation or user code can cause it to be added to the cache, it
could also be retrieved from the cache prior to the point the
Finalization thread gets round to calling Inflater.finalize()).
Instead, in order to safely cache and reuse inflater objects in ZipFile,
I believe it to be necessary for the ZipFile to prevent them from
becoming eligible for finalization/GC (until a decision is made not to
put them back in the inflater cache due to their having been explicitly
ended).
It can do this by holding references to the inflater objects that are in
use by its input streams, as well as those in the cache (ie. those
currently unused).
(NB: Previously, the Inflater objects happened to be preserved in this
way, due to the ZipFile keeping alive the InputStream objects, which
have references to the inflater objects. Thus the challenge is to
preserve the Inflater objects - so that they can be cached and reused -
without hitting the heap by prolonging the life of the InputStream
objects).
ZipFile must also only return an inflater to the inflater cache at the
point where its end() method can no longer be explicitly called
(directly or indirectly) by the user - such as i done in the
InflaterFinalization testscase.
In other words, it's addition to the cache should only be considered
once the input stream that was using it has been completely GC'd.
Please see below another formulation of the suggested change which
incorporates these features.
It adapts the 'streams' map to hold references to the in-use inflater
objects as values, and reverts to explicitly dealing with the cleanup of
stale entries in the map so as to use that point to drive
releaseInflater() with the stored value.
(I have chosen to hold these values 'softly', so that they may be
released by GC if they are not reused - by being given to the cache - in
a timely manner. If the input stream is not explicitly closed, an
inflater object may not be reset() until the call to releaseInflater()
from clearStaleStreams() - ie. it may continue to hold onto its old
buffer until then. As clearStaleStreams() is only driven when a new
InputStream is requested, this may be retained for some time. By holding
the inflater 'softly', the system can decide to release the inflater's
resources by GC'ing it, if heap demand is high, or the system has not
asked for new InputStreams from this ZipFile for a while).
With releaseInflater() being driven from the cleanup of stale 'streams'
entries, it no longer needs to be called from ZFIIS.close(), so,
instead, only Inflater.reset() is called from there (providing the
inflater object has not explicitly been ended) so that it releases the
buffer it has been holding.
This allows the synchronization to be simplified, as there's no longer a
reason to synchronize on the inflater cache independently of that on the
ZipFile itself.
(I've also changed the inflater cache to use a Deque, which allows its
code to be simplified and should be (marginally) more efficient,
according to the API doc).
Also, based on the previous observation that object's finalize() methods
should concentrate on freeing off their own associated system resources,
I've modified ZipFile's close() and finalize() methods such that they
both call closeFile(), which deals with closing its system resources).
I believe, with these changes, the handling of the caching & reuse of
ZipFile's Inflater objects is handled in a safe manner, whilst still
allowing its InputStreams to be GC'd in a timely manner.
Please get back to me with any comments, questions or suggestions on
this,
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.richards at ngmr.net>, <neil_richards at uk.ibm.com>
# Date 1300289208 0
# Branch zip-heap
# Node ID c7ed122ae9e3354e99c53ca191b467ef9d9c97ff
# Parent aa13e7702cd9d8aca9aa38f1227f966990866944
7031076: Retained ZipFile InputStreams increase heap demand
Summary: Allow unreferenced ZipFile InputStreams to be finalized, GC'd
Contributed-by: <neil.richards at ngmr.net>
diff -r aa13e7702cd9 -r c7ed122ae9e3 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
@@ -30,11 +30,16 @@
import java.io.IOException;
import java.io.EOFException;
import java.io.File;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.SoftReference;
+import java.lang.ref.WeakReference;
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.Iterator;
+import java.util.Map;
import java.util.NoSuchElementException;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
@@ -314,8 +319,21 @@
// 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<WeakReference<InputStream>, SoftReference<Inflater>>
+ streams = new HashMap<>();
+ private final ReferenceQueue<? super InputStream> staleStreamQueue =
+ new ReferenceQueue<>();
+ private static final SoftReference<Inflater> NULL_INFLATER_REF =
+ new SoftReference<>(null);
+
+ private synchronized void clearStaleStreams() {
+ Object staleStream;
+ while (null != (staleStream = staleStreamQueue.poll())) {
+ releaseInflater(streams.remove(staleStream).get());
+ }
+ }
/**
* Returns an input stream for reading the contents of the specified
@@ -339,6 +357,7 @@
ZipFileInputStream in = null;
synchronized (this) {
ensureOpen();
+ clearStaleStreams();
if (!zc.isUTF8() && (entry.flag & EFS) != 0) {
jzentry = getEntry(jzfile, zc.getBytesUTF8(entry.name), false);
} else {
@@ -351,51 +370,21 @@
switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ streams.put(
+ new WeakReference<InputStream>(in, staleStreamQueue),
+ NULL_INFLATER_REF);
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);
+ streams.put(
+ new WeakReference<InputStream>(is, staleStreamQueue),
+ new SoftReference<Inflater>(inf));
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,36 +392,77 @@
}
}
+ 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();
+ if (false == inf.ended()) {
+ inf.reset();
+ }
+ 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.
*/
- 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);
+ private synchronized Inflater getInflater() {
+ Inflater inf;
+ 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;
+ private synchronized void releaseInflater(Inflater inf) {
+ if ((null != inf) && (false == inf.ended())) {
inf.reset();
- inflaters.add(inf);
+ 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.
@@ -539,40 +569,48 @@
*
* @throws IOException if an I/O error has occurred
*/
- public void close() throws IOException {
- synchronized (this) {
- closeRequested = true;
+ public synchronized void close() throws IOException {
+ closeRequested = true;
- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
- is.close();
+ // Close streams, release their inflaters
+ Iterator<Map.Entry<WeakReference<InputStream>, SoftReference<Inflater>>>
+ streamEntryIterator = streams.entrySet().iterator();
+ while (streamEntryIterator.hasNext()) {
+ Map.Entry<WeakReference<InputStream>, SoftReference<Inflater>>
+ streamEntry = streamEntryIterator.next();
+ InputStream is = streamEntry.getKey().get();
+ if (null != is) {
+ is.close();
}
+ Inflater inf = streamEntry.getValue().get();
+ if (null != inf) {
+ inf.end();
+ }
+ streamEntryIterator.remove();
+ }
- if (jzfile != 0) {
- // Close the zip file
- long zf = this.jzfile;
- jzfile = 0;
+ // Release cached inflaters
+ Inflater inf;
+ while (null != (inf = inflaterCache.poll())) {
+ inf.end();
+ }
- close(zf);
+ closeFile();
+ }
- // Release inflaters
- synchronized (inflaters) {
- int size = inflaters.size();
- for (int i = 0; i < size; i++) {
- Inflater inf = (Inflater)inflaters.get(i);
- inf.end();
- }
- }
- }
+ private synchronized void closeFile() {
+ if (jzfile != 0) {
+ // Close the zip file
+ long zf = this.jzfile;
+ jzfile = 0;
+
+ close(zf);
}
}
-
/**
- * 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,
@@ -585,7 +623,7 @@
* @see java.util.zip.ZipFile#close()
*/
protected void finalize() throws IOException {
- close();
+ closeFile();
}
private static native void close(long jzfile);
@@ -684,9 +722,12 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}
diff -r aa13e7702cd9 -r c7ed122ae9e3 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.richards at ngmr.net>, <neil_richards at 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;
+ }
+}
More information about the core-libs-dev
mailing list