Discussion:
Suspected regression: fix for 6735255 causes delay in GC of ZipFile InputStreams, increase in heap demand
Neil Richards
2011-03-23 21:46:26 UTC
Permalink
Hi,
I've noticed that the fix introduced to address bug 6735255 [1] [2] had
an unfortunate side-effect.

That fix stores references to the InputStream objects returned from
ZipFile.getInputStream(ZipEntry) in a HashSet (within ZipFile), so that
the close() method may be called upon those objects when ZipFile.close()
is called.

It does this to conform to the existing API description, and to avoid a
native memory leak which would occur if the InputStream is GC'd without
its close() being called.

However, by holding these InputStreams in a set within their ZipFile
object, their lifecycle is now prolonged until the ZipFile object either
has its close() method called, or is finalized (prior to GC).

I've found scenarios (in user code) were ZipFile objects are held onto
for a long time (eg. the entire lifetime of the process), but where
InputStream objects from that ZipFile (for individual entries in the zip
file) are obtained, used, then abandoned on a large number of occasions.

(An example here might be a user-defined, long-lasting class loader,
which might retain a ZipFile instance for each jar file in its search
path, but which will create transitory input streams to read out
particular files from those (large) jar files).

I see that the InputStream objects returned from
ZipFile.getInputStream(ZipEntry) tend to hold references to a couple of
sizeable byte arrays, one 512 bytes in size, the other 1002 bytes.

So by prolonging the life span of these objects, the fix for 6735255 can
inadvertently cause a significant increase in the demand/usage on the
heap (in practice, running into many Mb's).

(This is not so if the user code is good-mannered enough to explicitly
always call close() on the InputStreams in question.
However, experience shows that user code cannot be relied upon to behave
so benignly).

I believe this introduced problem can be addressed by:
1. Holding references to the InputStreams in the 'streams' Set
within ZipFile via WeakReferences, so they may be GC'd as soon
as they are no longer externally (strongly) referenced.
2. Adding finalization logic to the InputStream implementation
classes, which ensures their close() method is called prior to
GC.
3. Prevent Inflater objects from being returned to the pool (in the
ZipFile object) if close() has been called from finalize().

To that end, please find below an 'hg export' of a proposed change which
implements these aspects, together with a testcase to demonstrate the
problem/fix.

Any comments / suggestions on this gratefully received,
Thanks,
Neil

[1] http://bugs.sun.com/view_bug.do?bug_id=6735255
[2] http://hg.openjdk.java.net/jdk7/jdk7/jdk/rev/49478a651a28
--
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 34642c56c6febfd845c6c4ffd0344c3257f0b848
# Parent 554adcfb615e63e62af530b1c10fcf7813a75b26
Summary: Allow unreferenced ZipFile InputStreams to be finalized, GC'd
Contributed-by: <neil.richards at ngmr.net>

diff -r 554adcfb615e -r 34642c56c6fe src/share/classes/java/util/zip/ZipFile.java
--- a/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:01:07 2011 -0700
+++ b/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:26:48 2011 +0000
@@ -30,11 +30,14 @@
import java.io.IOException;
import java.io.EOFException;
import java.io.File;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
import java.nio.charset.Charset;
import java.util.Vector;
import java.util.Enumeration;
import java.util.Set;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.NoSuchElementException;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
@@ -315,7 +318,16 @@
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 Set<WeakReference<InputStream>> streams = new HashSet<>();
+ private final ReferenceQueue<? super InputStream> staleStreamQueue =
+ new ReferenceQueue<>();
+
+ private void clearStaleStreams() {
+ Object staleStream;
+ while (null != (staleStream = staleStreamQueue.poll())) {
+ streams.remove(staleStream);
+ }
+ }

/**
* Returns an input stream for reading the contents of the specified
@@ -339,6 +351,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 +364,19 @@

switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ streams.add(
+ new WeakReference<InputStream>(in, staleStreamQueue));
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.add(
+ new WeakReference<InputStream>(is, staleStreamQueue));
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,6 +384,58 @@
}
}

+ private class ZipFileInflaterInputStream extends InflaterInputStream {
+ private boolean isClosed = false;
+ private boolean eof = false;
+ private final ZipFileInputStream zfin;
+ private boolean inFinalizer = false;
+
+ ZipFileInflaterInputStream(ZipFileInputStream zfin, Inflater inf,
+ int size) {
+ super(zfin, inf, size);
+ this.zfin = zfin;
+ }
+
+ public void close() throws IOException {
+ synchronized (ZipFile.this) {
+ if (!isClosed) {
+ super.close();
+ if (false == inFinalizer)
+ 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);
+ }
+
+ protected void finalize() throws IOException {
+ inFinalizer = true;
+ close();
+ }
+ }
+
/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
@@ -543,11 +576,14 @@
synchronized (this) {
closeRequested = true;

- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
+ Iterator<WeakReference<InputStream>> streamsIterator =
+ streams.iterator();
+ while (streamsIterator.hasNext()) {
+ InputStream is = streamsIterator.next().get();
+ if (null != is) {
is.close();
+ }
+ streamsIterator.remove();
}

if (jzfile != 0) {
@@ -684,9 +720,12 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}


diff -r 554adcfb615e -r 34642c56c6fe 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 %BUG%
+ * @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.close();
+ }
+ }
+
+ 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;
+ }
+}
Alan Bateman
2011-03-25 13:30:46 UTC
Permalink
Post by Neil Richards
Hi,
I've noticed that the fix introduced to address bug 6735255 [1] [2] had
an unfortunate side-effect.
That fix stores references to the InputStream objects returned from
ZipFile.getInputStream(ZipEntry) in a HashSet (within ZipFile), so that
the close() method may be called upon those objects when ZipFile.close()
is called.
It does this to conform to the existing API description, and to avoid a
native memory leak which would occur if the InputStream is GC'd without
its close() being called.
However, by holding these InputStreams in a set within their ZipFile
object, their lifecycle is now prolonged until the ZipFile object either
has its close() method called, or is finalized (prior to GC).
I've created 7031076 to track this and generated a webrev from the
change-set that you attached:
http://cr.openjdk.java.net/~alanb/7031076/webrev/

Sherman mentioned off-list that he plans to look at this.

-Alan.
Xueming Shen
2011-03-29 19:05:12 UTC
Permalink
Hi Neil,

It appears to be a "regression" in scenario you described (user
application never close the
input stream after use and the ZipFile instance being retained during
the lifetime of the
process). The proposed approach seems to solve this particular problem.

Here are my comments regarding the patch.

(1) ZipFileInflaterInputStream.close() now synchronizes on Zipfile.this,
is it really necessary?
given the rest of the implementation doesn't guarantee the
returned InputStream object
is thread-safe (the implementation does make sure the access to
the underlying
ZipFile is thread-safe, though). The new finalize() now also
invokes close(), which means
the ZipFile object now gets locked twice (assume the good manner
application does
invoke the close() after use) for each InputStream after use.

(2) Is there any particular reason that we don't want to "reuse" the
Inflater(), if the close
comes from the finalize() in your testing evn? One thing we
noticed before is that the
moment the InputStream gets finalized, the Inflater embedded
might have been
finalized already, this is the reason why we have a "inf.ended()"
check in releaseInflater().

The disadvantage/limitation of existing Inflater caching mechanism
is that it does not
have a "cap" to limit the maximum number of inflater it should
cache. Are you worried
too many inflaters get cached, if they only get collected during
GC/finalize()? This actually
indicates that the cache mechanism does not work at all in your
scenario, even with the
proposed fix. An alternative solution (better?) is to make ZFIIS
to check the bytes read
during its read() method, and pro-actively close the stream when
it reaches the end of
the stream, same as what ZipFileInputStream does, which I believe
should solve the
problem in most use scenario (except, the application does not
bother reading the
InputSteeam to its end, which is unlikely).

(3) The attached test case does "f.close()" in its finally block, I
guess it should be f.delete()?

-Sherman
Post by Neil Richards
Hi,
I've noticed that the fix introduced to address bug 6735255 [1] [2] had
an unfortunate side-effect.
That fix stores references to the InputStream objects returned from
ZipFile.getInputStream(ZipEntry) in a HashSet (within ZipFile), so that
the close() method may be called upon those objects when ZipFile.close()
is called.
It does this to conform to the existing API description, and to avoid a
native memory leak which would occur if the InputStream is GC'd without
its close() being called.
However, by holding these InputStreams in a set within their ZipFile
object, their lifecycle is now prolonged until the ZipFile object either
has its close() method called, or is finalized (prior to GC).
I've found scenarios (in user code) were ZipFile objects are held onto
for a long time (eg. the entire lifetime of the process), but where
InputStream objects from that ZipFile (for individual entries in the zip
file) are obtained, used, then abandoned on a large number of occasions.
(An example here might be a user-defined, long-lasting class loader,
which might retain a ZipFile instance for each jar file in its search
path, but which will create transitory input streams to read out
particular files from those (large) jar files).
I see that the InputStream objects returned from
ZipFile.getInputStream(ZipEntry) tend to hold references to a couple of
sizeable byte arrays, one 512 bytes in size, the other 1002 bytes.
So by prolonging the life span of these objects, the fix for 6735255 can
inadvertently cause a significant increase in the demand/usage on the
heap (in practice, running into many Mb's).
(This is not so if the user code is good-mannered enough to explicitly
always call close() on the InputStreams in question.
However, experience shows that user code cannot be relied upon to behave
so benignly).
1. Holding references to the InputStreams in the 'streams' Set
within ZipFile via WeakReferences, so they may be GC'd as soon
as they are no longer externally (strongly) referenced.
2. Adding finalization logic to the InputStream implementation
classes, which ensures their close() method is called prior to
GC.
3. Prevent Inflater objects from being returned to the pool (in the
ZipFile object) if close() has been called from finalize().
To that end, please find below an 'hg export' of a proposed change which
implements these aspects, together with a testcase to demonstrate the
problem/fix.
Any comments / suggestions on this gratefully received,
Thanks,
Neil
[1] http://bugs.sun.com/view_bug.do?bug_id=6735255
[2] http://hg.openjdk.java.net/jdk7/jdk7/jdk/rev/49478a651a28
Neil Richards
2011-03-30 19:53:30 UTC
Permalink
Hi Sherman,
Thanks for your review and comments on this.
Post by Xueming Shen
Hi Neil,
It appears to be a "regression" in scenario you described (user
application never close the input stream after use and the ZipFile
instance being retained during the lifetime of the process). The
proposed approach seems to solve this particular problem.
Here are my comments regarding the patch.
(1) ZipFileInflaterInputStream.close() now synchronizes on
Zipfile.this, is it really necessary?
Synchronization is used so that an update to 'isClosed' on one thread is
seen by another.

Adding finalization (ie. a call from finalize() to close()) effectively
introduces a new thread into the mix, namely the finalization thread.
Whilst you're correct in saying that, in general, InputStream gives no
thread-safe guarantees, in this particular case I believe it is valid to
make sure updates to this field are seen across threads, to prevent the
logic of close() being run twice.

I chose to synchronize on ZipFile.this merely for consistency - ie.
because that's what ZipFileInputStream (the other InputStream impl in
ZipFile) does.

I suppose I felt the risks of creating some kind of obscure deadlock
scenario was minimised by copying the existing pattern.

I can be easily swayed if you think there's a better object to
synchronize on, that doesn't run an increased risk of deadlock.
Post by Xueming Shen
(2) Is there any particular reason that we don't want to "reuse" the
Inflater(), if the close comes from the finalize() in your testing
evn? One thing we noticed before is that the moment the
InputStream gets finalized, the Inflater embedded might have been
finalized already, this is the reason why we have a "inf.ended()"
check in releaseInflater().
The ZipFileInflaterInputStream object's finalize() method is called by
the finalization thread, after it has been added to the finalization
queue by GC.

If *it* has been added to the finalization queue (because it's eligible
for GC), then so will the Inflater object it is referencing.

As finalization gives no guarantees as to the order in which objects are
finalized, regardless of whether finalize() has been called on the
Inflater object, it is nevertheless the case that its method *will* be
called at some point, so the Inflater object *will* be ended.

If it has been put back into the inflater cache (because ZFIIS's
finalize happened to be called first), then this ended inflater will lie
in wait for another ZFIIS to use it, which would result in a
particularly unexpected exception being thrown.

In general, I believe it to be quite frowned on to "resurrect" objects
which are going through finalization (including those queued up for
finalization) and place them back into the live set of object, as weird
behaviour often ensues if one does.

(Especially as, iirc, finalize() is only called once per object,
regardless of whether it is "dug back up" in this fashion).
Post by Xueming Shen
(3) The attached test case does "f.close()" in its finally block, I
guess it should be f.delete()?
When I wrote the testcase, I relied upon File.deleteOnExit() to take
care of cleaning up the temporary file(s) created.

However, I recall now that I don't think it guarantees deletion on
abnormal termination.

I agree that calling f.delete() instead of f.close() would be a good
improvement to the testcase.

Please find below an updated changeset, with this change made and the
bug number filled in.

Hope this helps to clarify things,
Thanks again,
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 8ab5aa0669a176d95502d9cf68027ae9679ccab2
# Parent 554adcfb615e63e62af530b1c10fcf7813a75b26
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 --git a/src/share/classes/java/util/zip/ZipFile.java b/src/share/classes/java/util/zip/ZipFile.java
--- a/src/share/classes/java/util/zip/ZipFile.java
+++ b/src/share/classes/java/util/zip/ZipFile.java
@@ -30,11 +30,14 @@
import java.io.IOException;
import java.io.EOFException;
import java.io.File;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
import java.nio.charset.Charset;
import java.util.Vector;
import java.util.Enumeration;
import java.util.Set;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.NoSuchElementException;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
@@ -315,7 +318,16 @@
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 Set<WeakReference<InputStream>> streams = new HashSet<>();
+ private final ReferenceQueue<? super InputStream> staleStreamQueue =
+ new ReferenceQueue<>();
+
+ private void clearStaleStreams() {
+ Object staleStream;
+ while (null != (staleStream = staleStreamQueue.poll())) {
+ streams.remove(staleStream);
+ }
+ }

/**
* Returns an input stream for reading the contents of the specified
@@ -339,6 +351,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 +364,19 @@

switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ streams.add(
+ new WeakReference<InputStream>(in, staleStreamQueue));
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.add(
+ new WeakReference<InputStream>(is, staleStreamQueue));
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,6 +384,58 @@
}
}

+ private class ZipFileInflaterInputStream extends InflaterInputStream {
+ private boolean isClosed = false;
+ private boolean eof = false;
+ private final ZipFileInputStream zfin;
+ private boolean inFinalizer = false;
+
+ ZipFileInflaterInputStream(ZipFileInputStream zfin, Inflater inf,
+ int size) {
+ super(zfin, inf, size);
+ this.zfin = zfin;
+ }
+
+ public void close() throws IOException {
+ synchronized (ZipFile.this) {
+ if (!isClosed) {
+ super.close();
+ if (false == inFinalizer)
+ 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);
+ }
+
+ protected void finalize() throws IOException {
+ inFinalizer = true;
+ close();
+ }
+ }
+
/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
@@ -543,11 +576,14 @@
synchronized (this) {
closeRequested = true;

- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
+ Iterator<WeakReference<InputStream>> streamsIterator =
+ streams.iterator();
+ while (streamsIterator.hasNext()) {
+ InputStream is = streamsIterator.next().get();
+ if (null != is) {
is.close();
+ }
+ streamsIterator.remove();
}

if (jzfile != 0) {
@@ -684,9 +720,12 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}


diff --git a/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java b/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java
new file mode 100644
--- /dev/null
+++ b/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java
@@ -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;
+ }
+}
Xueming Shen
2011-03-30 20:31:06 UTC
Permalink
Post by Neil Richards
Hi Sherman,
Thanks for your review and comments on this.
Post by Xueming Shen
Hi Neil,
It appears to be a "regression" in scenario you described (user
application never close the input stream after use and the ZipFile
instance being retained during the lifetime of the process). The
proposed approach seems to solve this particular problem.
Here are my comments regarding the patch.
(1) ZipFileInflaterInputStream.close() now synchronizes on
Zipfile.this, is it really necessary?
Synchronization is used so that an update to 'isClosed' on one thread is
seen by another.
Adding finalization (ie. a call from finalize() to close()) effectively
introduces a new thread into the mix, namely the finalization thread.
Whilst you're correct in saying that, in general, InputStream gives no
thread-safe guarantees, in this particular case I believe it is valid to
make sure updates to this field are seen across threads, to prevent the
logic of close() being run twice.
Isn't it true that when the finalize()->close() gets invoked, there
should be no strong
reference anywhere else that you can use to invoke close() in other thread?
ZipFileInputStream class has to sync on ZipFile.this, the read/close()
methods are
accessing the underlying/shared bits of the ZipFile. For
ZipFileInflaterInputStream.close()
case, even we want to make sure the isClose is synced, I would prefer
to see a "local"
lock, maybe simply make close() synchronized is better?

-Sherman
Post by Neil Richards
I chose to synchronize on ZipFile.this merely for consistency - ie.
because that's what ZipFileInputStream (the other InputStream impl in
ZipFile) does.
I suppose I felt the risks of creating some kind of obscure deadlock
scenario was minimised by copying the existing pattern.
I can be easily swayed if you think there's a better object to
synchronize on, that doesn't run an increased risk of deadlock.
Post by Xueming Shen
(2) Is there any particular reason that we don't want to "reuse" the
Inflater(), if the close comes from the finalize() in your testing
evn? One thing we noticed before is that the moment the
InputStream gets finalized, the Inflater embedded might have been
finalized already, this is the reason why we have a "inf.ended()"
check in releaseInflater().
The ZipFileInflaterInputStream object's finalize() method is called by
the finalization thread, after it has been added to the finalization
queue by GC.
If *it* has been added to the finalization queue (because it's eligible
for GC), then so will the Inflater object it is referencing.
As finalization gives no guarantees as to the order in which objects are
finalized, regardless of whether finalize() has been called on the
Inflater object, it is nevertheless the case that its method *will* be
called at some point, so the Inflater object *will* be ended.
If it has been put back into the inflater cache (because ZFIIS's
finalize happened to be called first), then this ended inflater will lie
in wait for another ZFIIS to use it, which would result in a
particularly unexpected exception being thrown.
In general, I believe it to be quite frowned on to "resurrect" objects
which are going through finalization (including those queued up for
finalization) and place them back into the live set of object, as weird
behaviour often ensues if one does.
(Especially as, iirc, finalize() is only called once per object,
regardless of whether it is "dug back up" in this fashion).
Post by Xueming Shen
(3) The attached test case does "f.close()" in its finally block, I
guess it should be f.delete()?
When I wrote the testcase, I relied upon File.deleteOnExit() to take
care of cleaning up the temporary file(s) created.
However, I recall now that I don't think it guarantees deletion on
abnormal termination.
I agree that calling f.delete() instead of f.close() would be a good
improvement to the testcase.
Please find below an updated changeset, with this change made and the
bug number filled in.
Hope this helps to clarify things,
Thanks again,
Neil
Neil Richards
2011-04-01 16:42:57 UTC
Permalink
Post by Xueming Shen
Isn't it true that when the finalize()->close() gets invoked, there
should be no strong reference anywhere else that you can use to invoke
close() in other thread?
It's true that once finalize() has been called, there can't be another
explicit call to close().
However, if close() is explicitly called first, it will be called again
when finalize() calls it, so one still wants the update to 'isClosed' to
be seen by the finalizer thread (in this case).
Post by Xueming Shen
ZipFileInputStream class has to sync on ZipFile.this, the read/close()
methods are accessing the underlying/shared bits of the ZipFile. For
ZipFileInflaterInputStream.close() case, even we want to make sure the
isClose is synced, I would prefer to see a "local" lock, maybe simply
make close() synchronized is better?
After assessing it again, I tend to agree - synchronizing on the ZFIIS
(instead of the ZipFile) now looks to me to be safe because it does not
do anything which synchronizes on the ZipFile object.

(Note that if it _did_ cause such synchronization, it would risk
deadlock with the code in ZipFile's close() method, which is
synchronized on itself when it calls close() on each remaining ZFIIS.
It's the difference in the order of acquiring the monitors that would
cause the potential for deadlock).

As it is, both close() methods synchronize on the 'inflaters' object
within their respective synchronized blocks, but that does not risk
deadlock.

Please find below an updated changeset below, containing the suggested
change to synchronization.

Thanks once again for your suggestions,
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 4aa4844cd3b9ae747ad736469134a8828ebeb652
# Parent 554adcfb615e63e62af530b1c10fcf7813a75b26
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 --git a/src/share/classes/java/util/zip/ZipFile.java b/src/share/classes/java/util/zip/ZipFile.java
--- a/src/share/classes/java/util/zip/ZipFile.java
+++ b/src/share/classes/java/util/zip/ZipFile.java
@@ -30,11 +30,14 @@
import java.io.IOException;
import java.io.EOFException;
import java.io.File;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
import java.nio.charset.Charset;
import java.util.Vector;
import java.util.Enumeration;
import java.util.Set;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.NoSuchElementException;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
@@ -315,7 +318,16 @@
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 Set<WeakReference<InputStream>> streams = new HashSet<>();
+ private final ReferenceQueue<? super InputStream> staleStreamQueue =
+ new ReferenceQueue<>();
+
+ private void clearStaleStreams() {
+ Object staleStream;
+ while (null != (staleStream = staleStreamQueue.poll())) {
+ streams.remove(staleStream);
+ }
+ }

/**
* Returns an input stream for reading the contents of the specified
@@ -339,6 +351,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 +364,19 @@

switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ streams.add(
+ new WeakReference<InputStream>(in, staleStreamQueue));
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.add(
+ new WeakReference<InputStream>(is, staleStreamQueue));
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,6 +384,56 @@
}
}

+ private class ZipFileInflaterInputStream extends InflaterInputStream {
+ private boolean isClosed = false;
+ private boolean eof = false;
+ private final ZipFileInputStream zfin;
+ private boolean inFinalizer = false;
+
+ ZipFileInflaterInputStream(ZipFileInputStream zfin, Inflater inf,
+ int size) {
+ super(zfin, inf, size);
+ this.zfin = zfin;
+ }
+
+ public synchronized void close() throws IOException {
+ if (!isClosed) {
+ super.close();
+ if (false == inFinalizer)
+ 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);
+ }
+
+ protected void finalize() throws IOException {
+ inFinalizer = true;
+ close();
+ }
+ }
+
/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
@@ -543,11 +574,14 @@
synchronized (this) {
closeRequested = true;

- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
+ Iterator<WeakReference<InputStream>> streamsIterator =
+ streams.iterator();
+ while (streamsIterator.hasNext()) {
+ InputStream is = streamsIterator.next().get();
+ if (null != is) {
is.close();
+ }
+ streamsIterator.remove();
}

if (jzfile != 0) {
@@ -684,9 +718,12 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}


diff --git a/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java b/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java
new file mode 100644
--- /dev/null
+++ b/test/java/util/zip/ZipFile/ClearStaleZipFileInputStreams.java
@@ -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;
+ }
+}
Xueming Shen
2011-04-01 19:07:33 UTC
Permalink
Post by Neil Richards
Post by Xueming Shen
Isn't it true that when the finalize()->close() gets invoked, there
should be no strong reference anywhere else that you can use to invoke
close() in other thread?
It's true that once finalize() has been called, there can't be another
explicit call to close().
However, if close() is explicitly called first, it will be called again
when finalize() calls it, so one still wants the update to 'isClosed' to
be seen by the finalizer thread (in this case).
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly
invoked by some thread, means someone has a strong reference to it, I
don't think the
finalize() can kick in until that close() returns and the strong
reference used to make that
explicit invocation is cleared. The InputStream is eligible for
finalization only after it is
"weakly" reachable, means no more "stronger" reachable exists, right?

-sherman
Post by Neil Richards
Post by Xueming Shen
ZipFileInputStream class has to sync on ZipFile.this, the read/close()
methods are accessing the underlying/shared bits of the ZipFile. For
ZipFileInflaterInputStream.close() case, even we want to make sure the
isClose is synced, I would prefer to see a "local" lock, maybe simply
make close() synchronized is better?
After assessing it again, I tend to agree - synchronizing on the ZFIIS
(instead of the ZipFile) now looks to me to be safe because it does not
do anything which synchronizes on the ZipFile object.
(Note that if it _did_ cause such synchronization, it would risk
deadlock with the code in ZipFile's close() method, which is
synchronized on itself when it calls close() on each remaining ZFIIS.
It's the difference in the order of acquiring the monitors that would
cause the potential for deadlock).
As it is, both close() methods synchronize on the 'inflaters' object
within their respective synchronized blocks, but that does not risk
deadlock.
Please find below an updated changeset below, containing the suggested
change to synchronization.
Thanks once again for your suggestions,
Neil
David Holmes
2011-04-01 23:17:40 UTC
Permalink
Post by Xueming Shen
Post by Neil Richards
Post by Xueming Shen
Isn't it true that when the finalize()->close() gets invoked, there
should be no strong reference anywhere else that you can use to invoke
close() in other thread?
It's true that once finalize() has been called, there can't be another
explicit call to close().
However, if close() is explicitly called first, it will be called again
when finalize() calls it, so one still wants the update to 'isClosed' to
be seen by the finalizer thread (in this case).
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly
invoked by some thread, means someone has a strong reference to it, I
don't think the
finalize() can kick in until that close() returns and the strong
reference used to make that
explicit invocation is cleared. The InputStream is eligible for
finalization only after it is
"weakly" reachable, means no more "stronger" reachable exists, right?
Actually no. One of the more obscure corner cases with finalization is
that you can actually finalize an object that is still being used. The
JLS actually spells this out - see section 12.6.1 and in particular the
Discussion within that section.

David
Post by Xueming Shen
-sherman
Post by Neil Richards
Post by Xueming Shen
ZipFileInputStream class has to sync on ZipFile.this, the read/close()
methods are accessing the underlying/shared bits of the ZipFile. For
ZipFileInflaterInputStream.close() case, even we want to make sure the
isClose is synced, I would prefer to see a "local" lock, maybe simply
make close() synchronized is better?
After assessing it again, I tend to agree - synchronizing on the ZFIIS
(instead of the ZipFile) now looks to me to be safe because it does not
do anything which synchronizes on the ZipFile object.
(Note that if it _did_ cause such synchronization, it would risk
deadlock with the code in ZipFile's close() method, which is
synchronized on itself when it calls close() on each remaining ZFIIS.
It's the difference in the order of acquiring the monitors that would
cause the potential for deadlock).
As it is, both close() methods synchronize on the 'inflaters' object
within their respective synchronized blocks, but that does not risk
deadlock.
Please find below an updated changeset below, containing the suggested
change to synchronization.
Thanks once again for your suggestions,
Neil
Neil Richards
2011-04-02 01:28:38 UTC
Permalink
Post by David Holmes
Post by Xueming Shen
Post by Neil Richards
Post by Xueming Shen
Isn't it true that when the finalize()->close() gets invoked, there
should be no strong reference anywhere else that you can use to invoke
close() in other thread?
It's true that once finalize() has been called, there can't be another
explicit call to close().
However, if close() is explicitly called first, it will be called again
when finalize() calls it, so one still wants the update to 'isClosed' to
be seen by the finalizer thread (in this case).
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly invoked by some thread, means someone has a
strong reference to it, I don't think the finalize() can kick in
until that close() returns and the strong reference used to make
that explicit invocation is cleared. The InputStream is eligible
for finalization only after it is "weakly" reachable, means no more
"stronger" reachable exists, right?
Actually no. One of the more obscure corner cases with finalization is
that you can actually finalize an object that is still being used. The
JLS actually spells this out - see section 12.6.1 and in particular the
Discussion within that section.
Also, I don't think my argument rests just on the behaviour in this
corner case.

Consider a normal thread with a normal (ie. "strong") reference
explicitly calls close(), and so updates the 'isClosed' field to 'true'.
To guarantee that that update is flushed from that normal thread's local
memory cache down to main memory, and to guarantee another thread (eg.
the finalizer thread) will see the update on a subsequent call (by
fetching the value from main memory rather than relying on its local
memory cache), one must either:
1. Using some form of synchronization for both the write and the
read of the field's value.
2. Mark the field as being 'volatile'.

As the logic inside close() involving 'isClosed' is intended to only let
the first caller through to perform the close operations, it seems more
appropriate in this case to use synchronization here.


David, I can't claim to have reached true enlightenment wrt the section
of the JLS that you point to - I suspect I'll need to lie down in a
darkened room with a dampened flannel across the brow and ponder the
nature of things a while to get there :-)

In the meantime, can you (or other knowledgeable GC folk) advise whether
it has any implications which would cause the current suggested fix to
be unsuitable?
(To recap, the current suggestion has the ZipFileInflaterInputStream's
close() method synchronized on itself, and called from its finalize()
method. The close() method has both the read and write of the 'isClosed'
field).

Any reassurance (or otherwise) gratefully accepted on this matter,
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
David Holmes
2011-04-03 23:04:48 UTC
Permalink
Hi Neil,
Post by Neil Richards
David, I can't claim to have reached true enlightenment wrt the section
of the JLS that you point to - I suspect I'll need to lie down in a
darkened room with a dampened flannel across the brow and ponder the
nature of things a while to get there :-)
In the meantime, can you (or other knowledgeable GC folk) advise whether
it has any implications which would cause the current suggested fix to
be unsuitable?
(To recap, the current suggestion has the ZipFileInflaterInputStream's
close() method synchronized on itself, and called from its finalize()
method. The close() method has both the read and write of the 'isClosed'
field).
Any reassurance (or otherwise) gratefully accepted on this matter,
I don't fully understand the object relationships here so I'll just make
a couple of observations on the synchronization in this code:

public synchronized void close() throws IOException {
if (!isClosed) {
super.close();
if (false == inFinalizer)
releaseInflater(inf);
isClosed = true;
}
}

public int available() throws IOException {
if (isClosed)
return 0;
long avail = zfin.size() - inf.getBytesWritten();
...
}

protected void finalize() throws IOException {
inFinalizer = true;
close();
}

1. If a call to close() occurs around the same time as finalization
occurs then the finalizer thread will set inFinalizer to true, at which
point the thread executing close() can see it is true and so may skip
the releaseInflater(inf) call.

2. As isClosed is not volatile, and available() is not synchronized, a
thread calling available() on a closed stream may not see that it has
been closed and so will likely encounter an IOException rather than
getting a zero return.


Even if #1 is not a practical problem I'd be inclined to make the
finalizer synchronized as well. By doing that you're effectively
ensuring that premature finalization of the stream won't happen.

For #2 it is a tricky call. If you don't actually expect the stream
object to be used by multiple threads then using a synchronized block to
read isClosed will be cheap in VMs that go to great effort to reduce the
cost of unnecessary synchronization (eg biased-locking in hotspot).
Otherwise making isClosed volatile is likely the better option.

HTH

David
Neil Richards
2011-04-07 13:30:23 UTC
Permalink
Post by David Holmes
1. If a call to close() occurs around the same time as finalization
occurs then the finalizer thread will set inFinalizer to true, at which
point the thread executing close() can see it is true and so may skip
the releaseInflater(inf) call.
2. As isClosed is not volatile, and available() is not synchronized, a
thread calling available() on a closed stream may not see that it has
been closed and so will likely encounter an IOException rather than
getting a zero return.
Even if #1 is not a practical problem I'd be inclined to make the
finalizer synchronized as well. By doing that you're effectively
ensuring that premature finalization of the stream won't happen.
I tend to agree, especially as it also makes the intention of the code
clearer.
Post by David Holmes
For #2 it is a tricky call. If you don't actually expect the stream
object to be used by multiple threads then using a synchronized block to
read isClosed will be cheap in VMs that go to great effort to reduce the
cost of unnecessary synchronization (eg biased-locking in hotspot).
Otherwise making isClosed volatile is likely the better option.
The check at the start of available() guards the logic beyond (which
uses a value from the inflater object, which would not be valid if the
stream has been closed()).

Because of this, I think it would be clearer to synchronize the
available() method too.

As you say, it is extremely likely that, in practice, this
synchronization will never be contended, and so won't impact performance
significantly (in modern VMs).

Please find below an updated changeset with these modifications,

Thanks for your advice in this,
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 db52003c128c8706f45e0b2bf9f98d5e905d2090
# Parent 554adcfb615e63e62af530b1c10fcf7813a75b26
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 554adcfb615e -r db52003c128c src/share/classes/java/util/zip/ZipFile.java
--- a/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:01:07 2011 -0700
+++ b/src/share/classes/java/util/zip/ZipFile.java Wed Mar 16 15:26:48 2011 +0000
@@ -30,11 +30,14 @@
import java.io.IOException;
import java.io.EOFException;
import java.io.File;
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
import java.nio.charset.Charset;
import java.util.Vector;
import java.util.Enumeration;
import java.util.Set;
import java.util.HashSet;
+import java.util.Iterator;
import java.util.NoSuchElementException;
import java.security.AccessController;
import sun.security.action.GetPropertyAction;
@@ -315,7 +318,16 @@
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 Set<WeakReference<InputStream>> streams = new HashSet<>();
+ private final ReferenceQueue<? super InputStream> staleStreamQueue =
+ new ReferenceQueue<>();
+
+ private void clearStaleStreams() {
+ Object staleStream;
+ while (null != (staleStream = staleStreamQueue.poll())) {
+ streams.remove(staleStream);
+ }
+ }

/**
* Returns an input stream for reading the contents of the specified
@@ -339,6 +351,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 +364,19 @@

switch (getEntryMethod(jzentry)) {
case STORED:
- streams.add(in);
+ streams.add(
+ new WeakReference<InputStream>(in, staleStreamQueue));
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.add(
+ new WeakReference<InputStream>(is, staleStreamQueue));
return is;
default:
throw new ZipException("invalid compression method");
@@ -403,6 +384,56 @@
}
}

+ private class ZipFileInflaterInputStream extends InflaterInputStream {
+ private boolean isClosed = false;
+ private boolean eof = false;
+ private final ZipFileInputStream zfin;
+ private boolean inFinalizer = false;
+
+ ZipFileInflaterInputStream(ZipFileInputStream zfin, Inflater inf,
+ int size) {
+ super(zfin, inf, size);
+ this.zfin = zfin;
+ }
+
+ public synchronized void close() throws IOException {
+ if (!isClosed) {
+ super.close();
+ if (false == inFinalizer)
+ 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 synchronized 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);
+ }
+
+ protected synchronized void finalize() throws IOException {
+ inFinalizer = true;
+ close();
+ }
+ }
+
/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
@@ -543,11 +574,14 @@
synchronized (this) {
closeRequested = true;

- if (streams.size() !=0) {
- Set<InputStream> copy = streams;
- streams = new HashSet<>();
- for (InputStream is: copy)
+ Iterator<WeakReference<InputStream>> streamsIterator =
+ streams.iterator();
+ while (streamsIterator.hasNext()) {
+ InputStream is = streamsIterator.next().get();
+ if (null != is) {
is.close();
+ }
+ streamsIterator.remove();
}

if (jzfile != 0) {
@@ -684,9 +718,12 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ }
}


diff -r 554adcfb615e -r db52003c128c 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;
+ }
+}
David Holmes
2011-04-07 22:13:23 UTC
Permalink
Hi Neil,
Post by Neil Richards
Post by David Holmes
1. If a call to close() occurs around the same time as finalization
occurs then the finalizer thread will set inFinalizer to true, at which
point the thread executing close() can see it is true and so may skip
the releaseInflater(inf) call.
2. As isClosed is not volatile, and available() is not synchronized, a
thread calling available() on a closed stream may not see that it has
been closed and so will likely encounter an IOException rather than
getting a zero return.
Even if #1 is not a practical problem I'd be inclined to make the
finalizer synchronized as well. By doing that you're effectively
ensuring that premature finalization of the stream won't happen.
I tend to agree, especially as it also makes the intention of the code
clearer.
Post by David Holmes
For #2 it is a tricky call. If you don't actually expect the stream
object to be used by multiple threads then using a synchronized block to
read isClosed will be cheap in VMs that go to great effort to reduce the
cost of unnecessary synchronization (eg biased-locking in hotspot).
Otherwise making isClosed volatile is likely the better option.
The check at the start of available() guards the logic beyond (which
uses a value from the inflater object, which would not be valid if the
stream has been closed()).
Because of this, I think it would be clearer to synchronize the
available() method too.
As you say, it is extremely likely that, in practice, this
synchronization will never be contended, and so won't impact performance
significantly (in modern VMs).
Please find below an updated changeset with these modifications,
Thanks for your advice in this,
No problem. From a synchronization perspective this all seems fine now.

Cheers,
David
Xueming Shen
2011-04-07 23:02:55 UTC
Permalink
Neil,

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.

Sherman
Post by Neil Richards
Post by David Holmes
1. If a call to close() occurs around the same time as finalization
occurs then the finalizer thread will set inFinalizer to true, at which
point the thread executing close() can see it is true and so may skip
the releaseInflater(inf) call.
2. As isClosed is not volatile, and available() is not synchronized, a
thread calling available() on a closed stream may not see that it has
been closed and so will likely encounter an IOException rather than
getting a zero return.
Even if #1 is not a practical problem I'd be inclined to make the
finalizer synchronized as well. By doing that you're effectively
ensuring that premature finalization of the stream won't happen.
I tend to agree, especially as it also makes the intention of the code
clearer.
Post by David Holmes
For #2 it is a tricky call. If you don't actually expect the stream
object to be used by multiple threads then using a synchronized block to
read isClosed will be cheap in VMs that go to great effort to reduce the
cost of unnecessary synchronization (eg biased-locking in hotspot).
Otherwise making isClosed volatile is likely the better option.
The check at the start of available() guards the logic beyond (which
uses a value from the inflater object, which would not be valid if the
stream has been closed()).
Because of this, I think it would be clearer to synchronize the
available() method too.
As you say, it is extremely likely that, in practice, this
synchronization will never be contended, and so won't impact performance
significantly (in modern VMs).
Please find below an updated changeset with these modifications,
Thanks for your advice in this,
Neil
Neil Richards
2011-04-08 12:36:33 UTC
Permalink
Post by Xueming Shen
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.richards at ngmr.net>, <neil_richards at 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.richards at 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.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;
+ }
+}
David Holmes
2011-04-08 23:01:20 UTC
Permalink
Neil,

You now have concurrency issues again. isClosed would need to be
volatile if the methods that set/check it aren't synchronized. Multiple
threads could call close() at the same time, and threads can call
available etc after close has finished its main work.

David
Post by Neil Richards
Post by Xueming Shen
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
Neil Richards
2011-04-09 00:04:29 UTC
Permalink
Hi David,

You are correct that the ZipFileInflaterInputStream code is not inherently
thread-safe (without user synchronization), but then, the contract of
InputStream does not require it to be (as Sherman previously observed on
this thread).

The previous suggested fix caused ZFIIS to interact with the finalize
thread, thus it had to care about being thread-safe at least to the extent
of safe-guarding that interaction.

As Sherman observed, and I agreed, finalization on that object is not really
needed, so the inherent cross-thread interaction that would be introduced by
the finalize method can be avoided in that case, and so the need to cater
for inherent thread-safety can likewise be avoided.

With the latest suggested change, if the ZFIIS is driven by multiple
threads, it will be done at the user's (developer's) behest, so the user can
(should) know to implement suitable synchronization to ensure correct
behaviour.

Therefore, I believe the same lack of synchronization in ZFIIS as there was
in the previous anonymous inner class to be entirely justified in this case,
given that it no longer has a finalize() method.

Please let me know if you still disagree with this assessment,
Thanks,
Neil

(Sent from my phone, so probably missing my usual signature)
David Holmes
2011-04-09 01:14:24 UTC
Permalink
Post by Neil Richards
You are correct that the ZipFileInflaterInputStream code is not
inherently thread-safe (without user synchronization), but then, the
contract of InputStream does not require it to be (as Sherman previously
observed on this thread).
The previous suggested fix caused ZFIIS to interact with the finalize
thread, thus it had to care about being thread-safe at least to the
extent of safe-guarding that interaction.
As Sherman observed, and I agreed, finalization on that object is not
really needed, so the inherent cross-thread interaction that would be
introduced by the finalize method can be avoided in that case, and so
the need to cater for inherent thread-safety can likewise be avoided.
Ok.

David
------
Post by Neil Richards
With the latest suggested change, if the ZFIIS is driven by multiple
threads, it will be done at the user's (developer's) behest, so the user
can (should) know to implement suitable synchronization to ensure
correct behaviour.
Therefore, I believe the same lack of synchronization in ZFIIS as there
was in the previous anonymous inner class to be entirely justified in
this case, given that it no longer has a finalize() method.
Please let me know if you still disagree with this assessment,
Thanks,
Neil
(Sent from my phone, so probably missing my usual signature)
Xueming Shen
2011-04-09 03:53:26 UTC
Permalink
Neil,

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?

Sherman
Post by Neil Richards
Post by Xueming Shen
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
Neil Richards
2011-04-10 17:28:02 UTC
Permalink
Post by Xueming Shen
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;
+ }
+}
Neil Richards
2011-04-11 12:15:16 UTC
Permalink
Post by Neil Richards
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.
Actually, I have a slight change of heart on this aspect.

Because close() may be driven from a finalize() method in user code (as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input stream)
and having started to be used by that other stream at the point at which
the code in close() is run.

(This is because weak references will be cleared, and *may* be queued on
the ReferenceQueue, prior to finalization).

Because of this, it's not actually entirely safe now to call inf.reset()
from ZFIIS.close().

Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.

As always, 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 b66c974bcfa957281e69a63cd4019a12c13cacae
# 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 b66c974bcfa9 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,78 @@
}
}

+ 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();
+ 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);
+ }
+
+ protected void finalize() {
+ clearStaleStreams();
+ }
+ }
+
/*
* 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);
+ private synchronized Inflater getInflater() {
+ Inflater inf;
+ while (null != (inf = inflaterCache.poll())) {
+ if (false == inf.ended()) {
return inf;
- } else {
- return new Inflater(true);
}
}
+ 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 +570,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 +624,7 @@
* @see java.util.zip.ZipFile#close()
*/
protected void finalize() throws IOException {
- close();
+ closeFile();
}

private static native void close(long jzfile);
@@ -684,9 +723,13 @@
freeEntry(ZipFile.this.jzfile, jzentry);
jzentry = 0;
}
- streams.remove(this);
}
}
+
+ protected void finalize() {
+ close();
+ clearStaleStreams();
+ }
}


diff -r aa13e7702cd9 -r b66c974bcfa9 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;
+ }
+}
Xueming Shen
2011-04-12 19:33:03 UTC
Permalink
Hi Neil,

(1) I believe it would be better to keep the synchronization lock for
get/releaseInfalter()
"local" instead of using the "global" ZipFile.this, which I agree
is "simple". But it also
means each/every time when you release the used inflater back to
cache, ZipFile.this
has to be held and any possible/potential read operation on
ZipFile from other thead/
inputstream has to wait ("get" seems fine, the current impl holds
the ZipFile.this
anyway before reach the getInflater()).

(2) The "resource" Infalter mainly holds is the native memory block it
alloc-ed at native
for zlib, which is not in the Java heap, so I doubt making it
"softly" really helps for GC.
Sure, cleanup of those "objects" themself is a plus, but I'm not
sure if it is really worth
using SoftReference in this case (it appears you are now invoking
clearStaleStreams()
from the finalize()).

(3) The releaseInfalter() is now totally driven from
clearStaleStreams()/staleStreamQueue,
which is under full control of GC. If GC does not kick in for
hours/days, infalters can never
be put back into its cache after use, even the applications
correctly/promptly close those
streams after use. And when the GC kicks in, we might see "bunch"
(hundreds, thousands)
of inflaters get pushed into cache. The approach does solve the
"timing" issue that got
us here, but it also now has this "native memory cache" mechanism
totally under the
control of/driven by the GC, the java heap management mechanism,
which might not be
a preferred scenario. Just wonder if we can have a better choice,
say with this GC-driven
as the backup cleanup and meanwhile still have the ZFIIS.close()
to do something to safely
put the used inflater back to cache promptly. To put the infalter
as the value of the "streams"
map appears to be a good idea/start, now the "infalter" will not
be targeted for finalized
until the entry gets cleaned from the map, in which might in fact
provide us a sort of
"orderly" (between the "stream" and its "inflater") clearup that
the GC/finalizer can't
guarantee. We still have couple days before we hit the "deadline"
(to get this one in), so it
might worth taking some time on this direction?

-Sherman
Post by Neil Richards
Post by Neil Richards
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.
Actually, I have a slight change of heart on this aspect.
Because close() may be driven from a finalize() method in user code (as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input stream)
and having started to be used by that other stream at the point at which
the code in close() is run.
(This is because weak references will be cleared, and *may* be queued on
the ReferenceQueue, prior to finalization).
Because of this, it's not actually entirely safe now to call inf.reset()
from ZFIIS.close().
Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.
As always, please get back to me with any comments, questions or
suggestions on this,
Thanks,
Neil
Steve Poole
2011-04-13 15:19:22 UTC
Permalink
Post by Xueming Shen
Hi Neil,
Hi Sherman , Neil is out on vacation so I will do my best to stand in
for him.
Post by Xueming Shen
(1) I believe it would be better to keep the synchronization lock for
get/releaseInfalter()
"local" instead of using the "global" ZipFile.this, which I agree
is "simple". But it also
means each/every time when you release the used inflater back to
cache, ZipFile.this
has to be held and any possible/potential read operation on
ZipFile from other thead/
inputstream has to wait ("get" seems fine, the current impl holds
the ZipFile.this
anyway before reach the getInflater()).
Ok - I agree - seems sensible. (Though I reserve the right to
contradict myself later)
Post by Xueming Shen
(2) The "resource" Infalter mainly holds is the native memory block it
alloc-ed at native
for zlib, which is not in the Java heap, so I doubt making it
"softly" really helps for GC.
Sure, cleanup of those "objects" themself is a plus, but I'm not
sure if it is really worth
using SoftReference in this case (it appears you are now invoking
clearStaleStreams()
from the finalize()).
I'd like to keep this in - not all implementations of zlib are equal in
where they allocate memory.
Post by Xueming Shen
(3) The releaseInfalter() is now totally driven from
clearStaleStreams()/staleStreamQueue,
which is under full control of GC. If GC does not kick in for
hours/days, infalters can never
be put back into its cache after use, even the applications
correctly/promptly close those
streams after use. And when the GC kicks in, we might see "bunch"
(hundreds, thousands)
of inflaters get pushed into cache. The approach does solve the
"timing" issue that got
us here, but it also now has this "native memory cache" mechanism
totally under the
control of/driven by the GC, the java heap management mechanism,
which might not be
a preferred scenario. Just wonder if we can have a better choice,
say with this GC-driven
as the backup cleanup and meanwhile still have the ZFIIS.close()
to do something to safely
put the used inflater back to cache promptly. To put the infalter
as the value of the "streams"
map appears to be a good idea/start, now the "infalter" will not
be targeted for finalized
until the entry gets cleaned from the map, in which might in fact
provide us a sort of
"orderly" (between the "stream" and its "inflater") clearup that
the GC/finalizer can't
guarantee. We still have couple days before we hit the "deadline"
(to get this one in), so it
might worth taking some time on this direction?
What is this "deadline" you are talking about?
Post by Xueming Shen
-Sherman
Post by Neil Richards
Post by Neil Richards
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.
Actually, I have a slight change of heart on this aspect.
Because close() may be driven from a finalize() method in user code (as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input stream)
and having started to be used by that other stream at the point at which
the code in close() is run.
(This is because weak references will be cleared, and *may* be queued on
the ReferenceQueue, prior to finalization).
Because of this, it's not actually entirely safe now to call inf.reset()
from ZFIIS.close().
Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.
As always, please get back to me with any comments, questions or
suggestions on this,
Thanks,
Neil
Steve Poole
2011-04-13 15:25:06 UTC
Permalink
Post by Steve Poole
Post by Xueming Shen
Hi Neil,
Hi Sherman , Neil is out on vacation so I will do my best to stand
in for him.
Post by Xueming Shen
(1) I believe it would be better to keep the synchronization lock for
get/releaseInfalter()
"local" instead of using the "global" ZipFile.this, which I
agree is "simple". But it also
means each/every time when you release the used inflater back to
cache, ZipFile.this
has to be held and any possible/potential read operation on
ZipFile from other thead/
inputstream has to wait ("get" seems fine, the current impl
holds the ZipFile.this
anyway before reach the getInflater()).
Ok - I agree - seems sensible. (Though I reserve the right to
contradict myself later)
Post by Xueming Shen
(2) The "resource" Infalter mainly holds is the native memory block
it alloc-ed at native
for zlib, which is not in the Java heap, so I doubt making it
"softly" really helps for GC.
Sure, cleanup of those "objects" themself is a plus, but I'm not
sure if it is really worth
using SoftReference in this case (it appears you are now
invoking clearStaleStreams()
from the finalize()).
I'd like to keep this in - not all implementations of zlib are equal
in where they allocate memory.
Post by Xueming Shen
(3) The releaseInfalter() is now totally driven from
clearStaleStreams()/staleStreamQueue,
which is under full control of GC. If GC does not kick in for
hours/days, infalters can never
be put back into its cache after use, even the applications
correctly/promptly close those
streams after use. And when the GC kicks in, we might see
"bunch" (hundreds, thousands)
of inflaters get pushed into cache. The approach does solve the
"timing" issue that got
us here, but it also now has this "native memory cache"
mechanism totally under the
control of/driven by the GC, the java heap management mechanism,
which might not be
a preferred scenario. Just wonder if we can have a better
choice, say with this GC-driven
as the backup cleanup and meanwhile still have the ZFIIS.close()
to do something to safely
put the used inflater back to cache promptly. To put the
infalter as the value of the "streams"
map appears to be a good idea/start, now the "infalter" will not
be targeted for finalized
until the entry gets cleaned from the map, in which might in
fact provide us a sort of
"orderly" (between the "stream" and its "inflater") clearup that
the GC/finalizer can't
guarantee. We still have couple days before we hit the
"deadline" (to get this one in), so it
might worth taking some time on this direction?
Dang! - pressed send when I meant save. I understand your comments -
on the face of it I agree with what you're suggesting - let me think it
through some more though.
Post by Steve Poole
What is this "deadline" you are talking about?
Post by Xueming Shen
-Sherman
Post by Neil Richards
Post by Neil Richards
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.
Actually, I have a slight change of heart on this aspect.
Because close() may be driven from a finalize() method in user code (as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input stream)
and having started to be used by that other stream at the point at which
the code in close() is run.
(This is because weak references will be cleared, and *may* be queued on
the ReferenceQueue, prior to finalization).
Because of this, it's not actually entirely safe now to call
inf.reset()
from ZFIIS.close().
Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.
As always, please get back to me with any comments, questions or
suggestions on this,
Thanks,
Neil
Xueming Shen
2011-04-13 19:22:24 UTC
Permalink
Post by Steve Poole
Post by Steve Poole
Post by Xueming Shen
Hi Neil,
Hi Sherman , Neil is out on vacation so I will do my best to stand
in for him.
Post by Xueming Shen
(1) I believe it would be better to keep the synchronization lock
for get/releaseInfalter()
"local" instead of using the "global" ZipFile.this, which I
agree is "simple". But it also
means each/every time when you release the used inflater back
to cache, ZipFile.this
has to be held and any possible/potential read operation on
ZipFile from other thead/
inputstream has to wait ("get" seems fine, the current impl
holds the ZipFile.this
anyway before reach the getInflater()).
Ok - I agree - seems sensible. (Though I reserve the right to
contradict myself later)
Post by Xueming Shen
(2) The "resource" Infalter mainly holds is the native memory block
it alloc-ed at native
for zlib, which is not in the Java heap, so I doubt making it
"softly" really helps for GC.
Sure, cleanup of those "objects" themself is a plus, but I'm
not sure if it is really worth
using SoftReference in this case (it appears you are now
invoking clearStaleStreams()
from the finalize()).
I'd like to keep this in - not all implementations of zlib are equal
in where they allocate memory.
Hi Steve,

It might be better/appropriate to use the SoftReference in the
"inflaterCache" instead of the
"streams", if we want to use the SoftReference for inflater caching purpose.

/*
* Gets an inflater from the list of available inflaters or allocates
* a new one.
*/
private synchronized Inflater getInflater() {
SoftReference<Inflater> sref;
Inflater inf;
while (null != (sref = inflaterCache.poll())) {
if ((inf = sref.get()) != null && !inf.ended()) {
return inf;
}
}
return new Inflater(true);
}

/*
* Releases the specified inflater to the list of available inflaters.
*/
private synchronized void releaseInflater(Inflater inf) {
if ((null != inf) && (false == inf.ended())) {
inf.reset();
inflaterCache.add(new SoftReference(inf));
}
}

// List of available Inflater objects for decompression
private Deque<SoftReference<Inflater>> inflaterCache = new
ArrayDeque<>();

-Sherman
Post by Steve Poole
Post by Steve Poole
Post by Xueming Shen
(3) The releaseInfalter() is now totally driven from
clearStaleStreams()/staleStreamQueue,
which is under full control of GC. If GC does not kick in for
hours/days, infalters can never
be put back into its cache after use, even the applications
correctly/promptly close those
streams after use. And when the GC kicks in, we might see
"bunch" (hundreds, thousands)
of inflaters get pushed into cache. The approach does solve the
"timing" issue that got
us here, but it also now has this "native memory cache"
mechanism totally under the
control of/driven by the GC, the java heap management
mechanism, which might not be
a preferred scenario. Just wonder if we can have a better
choice, say with this GC-driven
as the backup cleanup and meanwhile still have the
ZFIIS.close() to do something to safely
put the used inflater back to cache promptly. To put the
infalter as the value of the "streams"
map appears to be a good idea/start, now the "infalter" will
not be targeted for finalized
until the entry gets cleaned from the map, in which might in
fact provide us a sort of
"orderly" (between the "stream" and its "inflater") clearup
that the GC/finalizer can't
guarantee. We still have couple days before we hit the
"deadline" (to get this one in), so it
might worth taking some time on this direction?
Dang! - pressed send when I meant save. I understand your comments -
on the face of it I agree with what you're suggesting - let me think
it through some more though.
Post by Steve Poole
What is this "deadline" you are talking about?
Post by Xueming Shen
-Sherman
Post by Neil Richards
Post by Neil Richards
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.
Actually, I have a slight change of heart on this aspect.
Because close() may be driven from a finalize() method in user code (as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input stream)
and having started to be used by that other stream at the point at which
the code in close() is run.
(This is because weak references will be cleared, and *may* be queued on
the ReferenceQueue, prior to finalization).
Because of this, it's not actually entirely safe now to call inf.reset()
from ZFIIS.close().
Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.
As always, please get back to me with any comments, questions or
suggestions on this,
Thanks,
Neil
Xueming Shen
2011-04-14 21:48:24 UTC
Permalink
Steve, Neil,

As we discussed in previous emails that Neil's last patch (to put the
inflater into the "streams" as
the value of the map) does solve the FinalizeInflater failure issue
(provides a "orderly" final cleanup
between the "stream" and the "inflater". However to leave
releaseInfalter() to be solely driven
by GC might delay the inflater to be cached/re-used in a timely manner,
even in scenario that
those streams get closed explicitly by the invoker. Ideally it is
desirable that the inflater can be
re-used/put back into the cache when ZFIIS.close() gets invoked explicitly.

It appears it might be too hard, if not impossible, to arrange the
releaseInflater() to be invoked from
both clearStaleStreams()/staleStreamQueue and ZFIIS.close(), given the
non-guarantee, by the VM/GC,
of the timing/order among the clear up of weak reference objects,
enqueue them and the invocation of
finalize(). So I would suggest a relatively simple solution as showed in
webrev at

http://cr.openjdk.java.net/~sherman/7031076/webrev/src/share/classes/java/util/zip/ZipFile.java.sdiff.html

(1) simply use the WeakHashMap to keep the <InputStream, Inflater> pair,
we don't try
to release/put the used inflater back to cache, if the ZFIIS
stream does not get closed explicitly
(its underlying ZFIS stream and the inflater will then get
finalized by themself).

(2) release the inflater back to cache in ZFIIS.close(), only if the
inflater is sill "alive" in the
"streams", which guarantees that the inflater is not being
finalized and safe to put back
to cache (so long as we have the weak reference -> inflater pair
in "streams", it does not
matter if the reference to ZFIIS object is cleared or not). In
most cases, I would expect
when the ZFIIS.close() is invoked, both ZFIIS object and its
inflater are still reachable, so
the inflater gets re-used as expected. In "weird" scenario like
what we have in
FinalizeInflater, the ZFIIS object and its outer wrapper object
are being finalized when
ZFIIS.close() gets invoked (from wrapper.finalize()), there is
possibility that the inflater might
not be put back to cache, if the weak reverence->inflater pair has
been expunged out of the
"streams" already (because the weak reference to ZFIIS might have
been cleared, so its
entry in "streams" can be expunged any time, by any thread. At
this moment, the inflater
object might be declared as "un-reachable" and no longer safe to
put back into cache), but
I think we can live with this "missing".

(3) I realized that we actually are not removing the closed ZFIIS
objects (only the ZFIS objects
are removed when closed) from "streams" even though those input
streams are closed
gracefully by the invoker (as preferred, suggested, desired),
which means all those ZFIIS
streams will stay in "streams" (strong referenced in current
implementation), no matter
whether or not they get closed explicitly, until the ZipFile
object itself gets closed/finalized.
This might contribute to the problem Neil observed at first place
(the ZipFile InputStreams
accumulat in heap), actually I do have another incident report
#7033523 which I have
closed as the dup of #7031076.

Opinion? anything I overlooked/missed?

-Sherman
Post by Steve Poole
Post by Steve Poole
Post by Xueming Shen
Hi Neil,
Hi Sherman , Neil is out on vacation so I will do my best to stand
in for him.
Post by Xueming Shen
(1) I believe it would be better to keep the synchronization lock
for get/releaseInfalter()
"local" instead of using the "global" ZipFile.this, which I
agree is "simple". But it also
means each/every time when you release the used inflater back
to cache, ZipFile.this
has to be held and any possible/potential read operation on
ZipFile from other thead/
inputstream has to wait ("get" seems fine, the current impl
holds the ZipFile.this
anyway before reach the getInflater()).
Ok - I agree - seems sensible. (Though I reserve the right to
contradict myself later)
Post by Xueming Shen
(2) The "resource" Infalter mainly holds is the native memory block
it alloc-ed at native
for zlib, which is not in the Java heap, so I doubt making it
"softly" really helps for GC.
Sure, cleanup of those "objects" themself is a plus, but I'm
not sure if it is really worth
using SoftReference in this case (it appears you are now
invoking clearStaleStreams()
from the finalize()).
I'd like to keep this in - not all implementations of zlib are equal
in where they allocate memory.
Post by Xueming Shen
(3) The releaseInfalter() is now totally driven from
clearStaleStreams()/staleStreamQueue,
which is under full control of GC. If GC does not kick in for
hours/days, infalters can never
be put back into its cache after use, even the applications
correctly/promptly close those
streams after use. And when the GC kicks in, we might see
"bunch" (hundreds, thousands)
of inflaters get pushed into cache. The approach does solve the
"timing" issue that got
us here, but it also now has this "native memory cache"
mechanism totally under the
control of/driven by the GC, the java heap management
mechanism, which might not be
a preferred scenario. Just wonder if we can have a better
choice, say with this GC-driven
as the backup cleanup and meanwhile still have the
ZFIIS.close() to do something to safely
put the used inflater back to cache promptly. To put the
infalter as the value of the "streams"
map appears to be a good idea/start, now the "infalter" will
not be targeted for finalized
until the entry gets cleaned from the map, in which might in
fact provide us a sort of
"orderly" (between the "stream" and its "inflater") clearup
that the GC/finalizer can't
guarantee. We still have couple days before we hit the
"deadline" (to get this one in), so it
might worth taking some time on this direction?
Dang! - pressed send when I meant save. I understand your comments -
on the face of it I agree with what you're suggesting - let me think
it through some more though.
Post by Steve Poole
What is this "deadline" you are talking about?
Post by Xueming Shen
-Sherman
Post by Neil Richards
Post by Neil Richards
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.
Actually, I have a slight change of heart on this aspect.
Because close() may be driven from a finalize() method in user code (as
is done in the InflaterFinalizer test case), I believe it is possible
(albeit unlikely) for an inflater object to have been reclaimed from
'streams' (by a call to clearStaleStreams()), put into the inflater
cache, retrieved from there (by another thread creating an input stream)
and having started to be used by that other stream at the point at which
the code in close() is run.
(This is because weak references will be cleared, and *may* be queued on
the ReferenceQueue, prior to finalization).
Because of this, it's not actually entirely safe now to call inf.reset()
from ZFIIS.close().
Instead, I have added additional calls to clearStaleStreams() from the
finalize() methods of both InputStream implementations in the latest
(hopefully in the meaning of "last") changeset included below.
As always, please get back to me with any comments, questions or
suggestions on this,
Thanks,
Neil
Neil Richards
2011-04-18 12:33:21 UTC
Permalink
Post by Xueming Shen
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.richards at ngmr.net>, <neil_richards at 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.richards at 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.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;
+ }
+}
Neil Richards
2011-04-18 14:04:15 UTC
Permalink
Post by Neil Richards
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.
In fact, it's probably clearer (more closely following the pattern
elsewhere in the file) for ZFIIS.finalize() to call ZFIIS.close().

See changeset below with this additional tweak,
Please let me know what you think 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 9e0a3d49e4978264054cbacc9d3023715f1776d3
# 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 9e0a3d49e497 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,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 {
+ 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 +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();
+ }
}


diff -r aa13e7702cd9 -r 9e0a3d49e497 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;
+ }
+}
Xueming Shen
2011-04-18 16:49:00 UTC
Permalink
Post by Neil Richards
Post by Xueming Shen
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.
(resent, got some mail issue)

Hi Neil,

If you are now explicitly synchronize on "streams" everywhere, I don't
think we even need a copy
at close().

To add "close() in ZFIIS.finalize() appears to be safe now (?, maybe I
should think it again), but
given we are now using WeakHashMap, those weak reference will get
expunged every time that
map gets accessed, I doubt that really help lots (have to wait for the
GC kicks in anyway, if not
closed explicitly)

I'm running tests now, with the "copy" mentioned above.

-Sherman
Post by Neil Richards
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
Xueming Shen
2011-04-18 17:32:56 UTC
Permalink
Hi Neil,

All tests passed.

I'm starting to push your last patch. I generated the webrev at

http://cr.openjdk.java.net/~sherman/7031076/webrev/

It should be exactly the same as your last patch.

Thanks,
Sherman
Post by Xueming Shen
Post by Neil Richards
Post by Xueming Shen
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.
(resent, got some mail issue)
Hi Neil,
If you are now explicitly synchronize on "streams" everywhere, I don't
think we even need a copy
at close().
To add "close() in ZFIIS.finalize() appears to be safe now (?, maybe I
should think it again), but
given we are now using WeakHashMap, those weak reference will get
expunged every time that
map gets accessed, I doubt that really help lots (have to wait for the
GC kicks in anyway, if not
closed explicitly)
I'm running tests now, with the "copy" mentioned above.
-Sherman
Post by Neil Richards
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
Neil Richards
2011-04-18 18:11:56 UTC
Permalink
?Hi Neil,
All tests passed.
I'm starting to push your last patch. I generated the webrev at
http://cr.openjdk.java.net/~sherman/7031076/webrev/
It should be exactly the same as your last patch.
Thanks,
Sherman
Hi Sherman,
Thanks for all your advice and help in reaching (and committing) a
resolution for this issue - it really is much appreciated,
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
Neil Richards
2011-04-18 17:59:51 UTC
Permalink
If you are now explicitly synchronize on "streams" everywhere, I don't think
we even need a copy
at close().
I thought that the copy is needed so that the Map being iterated over
(in close()) is not able to be modified by the same thread via the
calls to InputStream.close() that it makes (which will try to call
streams.remove()), and hence avoid the risk of getting
ConcurrentModificationExceptions being thrown.

Are you sure that it is safe to avoid doing this copy ?
To add "close() in ZFIIS.finalize() appears to be safe now (?, maybe I
should think it again), but
given we are now using WeakHashMap, those weak reference will get expunged
every time that
map gets accessed, I doubt that really help lots (have to wait for the GC
kicks in anyway, if not
closed explicitly)
As I recall, WeakHashMap only looks to expunge stale entries when one
of its methods is called.
Therefore, causing it to be accessed (indirectly) from the finalize()
methods (by calling close()) helps to ensure that stale entries (with
the retained inflater's associated system resources) are not retained
for longer than needed.

This is true even if the particular access is unlikely to retrieve a
"live" entry.

Hope this provides some clarification to the suggested changeset,
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
Xueming Shen
2011-04-02 07:00:03 UTC
Permalink
Post by David Holmes
Post by Xueming Shen
Post by Neil Richards
Post by Xueming Shen
Isn't it true that when the finalize()->close() gets invoked, there
should be no strong reference anywhere else that you can use to invoke
close() in other thread?
It's true that once finalize() has been called, there can't be another
explicit call to close().
However, if close() is explicitly called first, it will be called again
when finalize() calls it, so one still wants the update to
'isClosed' to
be seen by the finalizer thread (in this case).
I'm not a GC guy, so I might be missing something here, but if
close() is being explicitly
invoked by some thread, means someone has a strong reference to it, I
don't think the
finalize() can kick in until that close() returns and the strong
reference used to make that
explicit invocation is cleared. The InputStream is eligible for
finalization only after it is
"weakly" reachable, means no more "stronger" reachable exists, right?
Actually no. One of the more obscure corner cases with finalization is
that you can actually finalize an object that is still being used. The
JLS actually spells this out - see section 12.6.1 and in particular
the Discussion within that section.
David
David,

The scenario that Neil and I were discussing is something like this,

There is class A

class A {
void close() {
...
}

protect void finalize() {
...
close();
}

}

when we are in the middle of A's close() (invoked by someone, not the
finalizer), do we need to worry about that
A's finalize() being invoked (and then the close()) by the finalizer
concurrently.

Does you "an object that still being used" include the scenario like
above, which means an object became
finalizer-reachable, when still in the middle of the execution (by some
alive, non-finalizer-thread) of one of its
instance method body?

The JLS 12.6.1, if I read it correctly, is for scenario that a reachable
object which is strongly referenced by a
stack reference can/may become finalizer-reachable sooner than it might
be expected, for example, the
compiler optimization can null out such reference in the middle of the
method body, so that object becomes
finalizer-reachable before the execution reach the return point of the
method, or ... The "execution" discussed
is not the execution inside the target object's method body. Am I
reading it correctly? Otherwise, it becomes a
little weird, image, you are in the middle of the execution of an
instance method, suddenly, the instance itself
is being finalized, all the native resource get released...

-Sherman
David Holmes
2011-04-03 01:15:46 UTC
Permalink
...
Post by Xueming Shen
Post by David Holmes
Post by Xueming Shen
explicit invocation is cleared. The InputStream is eligible for
finalization only after it is
"weakly" reachable, means no more "stronger" reachable exists, right?
Actually no. One of the more obscure corner cases with finalization is
that you can actually finalize an object that is still being used. The
JLS actually spells this out - see section 12.6.1 and in particular
the Discussion within that section.
The scenario that Neil and I were discussing is something like this,
There is class A
class A {
void close() {
...
}
protect void finalize() {
...
close();
}
}
when we are in the middle of A's close() (invoked by someone, not the
finalizer), do we need to worry about that
A's finalize() being invoked (and then the close()) by the finalizer
concurrently.
Does you "an object that still being used" include the scenario like
above, which means an object became
finalizer-reachable, when still in the middle of the execution (by some
alive, non-finalizer-thread) of one of its
instance method body?
The JLS 12.6.1, if I read it correctly, is for scenario that a reachable
object which is strongly referenced by a
stack reference can/may become finalizer-reachable sooner than it might
be expected, for example, the
compiler optimization can null out such reference in the middle of the
method body, so that object becomes
finalizer-reachable before the execution reach the return point of the
method, or ... The "execution" discussed
is not the execution inside the target object's method body. Am I
reading it correctly? Otherwise, it becomes a
little weird, image, you are in the middle of the execution of an
instance method, suddenly, the instance itself
is being finalized, all the native resource get released...
Yes 12.6.1 refers to the case where the object is only
strongly-reachable from a local stack reference. This includes the
"little weird" scenario you describe. A thread can be executing
a.close() where 'a' is a local stack reference, and 'a' can be finalized
while that is heppening.

I'm not saying hotspot will actually do this, but it is permissible
within the spec. This situation has been discussed quite a bit in the
past on the Java Memory Model mailing list.

David
Post by Xueming Shen
-Sherman
Xueming Shen
2011-04-03 05:39:17 UTC
Permalink
Thanks David!

And Neil, it seems my assumption is wrong and we do need the
synchronization for the close().
Your latest patch looks fine for me.

Thanks,
-Sherman
Post by David Holmes
...
Post by Xueming Shen
Post by David Holmes
Post by Xueming Shen
explicit invocation is cleared. The InputStream is eligible for
finalization only after it is
"weakly" reachable, means no more "stronger" reachable exists, right?
Actually no. One of the more obscure corner cases with finalization
is that you can actually finalize an object that is still being
used. The JLS actually spells this out - see section 12.6.1 and in
particular the Discussion within that section.
The scenario that Neil and I were discussing is something like this,
There is class A
class A {
void close() {
...
}
protect void finalize() {
...
close();
}
}
when we are in the middle of A's close() (invoked by someone, not the
finalizer), do we need to worry about that
A's finalize() being invoked (and then the close()) by the finalizer
concurrently.
Does you "an object that still being used" include the scenario like
above, which means an object became
finalizer-reachable, when still in the middle of the execution (by
some alive, non-finalizer-thread) of one of its
instance method body?
The JLS 12.6.1, if I read it correctly, is for scenario that a
reachable object which is strongly referenced by a
stack reference can/may become finalizer-reachable sooner than it
might be expected, for example, the
compiler optimization can null out such reference in the middle of
the method body, so that object becomes
finalizer-reachable before the execution reach the return point of
the method, or ... The "execution" discussed
is not the execution inside the target object's method body. Am I
reading it correctly? Otherwise, it becomes a
little weird, image, you are in the middle of the execution of an
instance method, suddenly, the instance itself
is being finalized, all the native resource get released...
Yes 12.6.1 refers to the case where the object is only
strongly-reachable from a local stack reference. This includes the
"little weird" scenario you describe. A thread can be executing
a.close() where 'a' is a local stack reference, and 'a' can be
finalized while that is heppening.
I'm not saying hotspot will actually do this, but it is permissible
within the spec. This situation has been discussed quite a bit in the
past on the Java Memory Model mailing list.
David
Post by Xueming Shen
-Sherman
Jeroen Frijters
2011-04-02 06:56:48 UTC
Permalink
Post by Xueming Shen
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly invoked by some thread, means someone has a strong
reference to it, I don't think the finalize() can kick in until that
close() returns
This is not correct. You can re-publish the reference from another finalizer method and thereby allow another thread to access the object concurrently with the finalizer.

Here's a possible sequence of events:
1) GC runs and determines that A and B are finalizable
2) Finalizer thread run A.finalize()
3) A.finalize publishes reference to B in static variable
4a) Another thread reads the static variable and calls B.close()
4b) Finalizer thread runs B.finalize()

Regards,
Jeroen
Xueming Shen
2011-04-02 07:47:01 UTC
Permalink
Post by Jeroen Frijters
Post by Xueming Shen
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly invoked by some thread, means someone has a strong
reference to it, I don't think the finalize() can kick in until that
close() returns
This is not correct. You can re-publish the reference from another finalizer method and thereby allow another thread to access the object concurrently with the finalizer.
1) GC runs and determines that A and B are finalizable
2) Finalizer thread run A.finalize()
3) A.finalize publishes reference to B in static variable
Jeroen, are you talking about the object resurrection from finalize()?

How do you re-publish/get the reference to B inside A.finalize()? I
think you can do that inside
B's finalize() to assign "this" to a global static variable.

Regard,
-Sherman
Post by Jeroen Frijters
4a) Another thread reads the static variable and calls B.close()
4b) Finalizer thread runs B.finalize()
Regards,
Jeroen
Xueming Shen
2011-04-02 08:15:14 UTC
Permalink
Post by Xueming Shen
Post by Jeroen Frijters
Post by Xueming Shen
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly invoked by some thread, means someone has a strong
reference to it, I don't think the finalize() can kick in until that
close() returns
This is not correct. You can re-publish the reference from another
finalizer method and thereby allow another thread to access the
object concurrently with the finalizer.
1) GC runs and determines that A and B are finalizable
2) Finalizer thread run A.finalize()
3) A.finalize publishes reference to B in static variable
Jeroen, are you talking about the object resurrection from finalize()?
How do you re-publish/get the reference to B inside A.finalize()? I
think you can do that inside
B's finalize() to assign "this" to a global static variable.
Jeroen,

Guess I replied too quick. I think I got what you meant. A has a
reference to B, GC finalizes A
and B the same time, then A republish reference to B and let someone
else to invoke b.close()..
I would have to admit it's a possible use scenario:-)

-Sherman
Post by Xueming Shen
Regard,
-Sherman
Post by Jeroen Frijters
4a) Another thread reads the static variable and calls B.close()
4b) Finalizer thread runs B.finalize()
Regards,
Jeroen
Robert Fischer
2011-04-02 22:40:32 UTC
Permalink
If A has a reference to B, how did B become finalizable?

Is it something like this?

root ---> A ---> B
root ----> null (leaving A ---> B both stranded)

Will the finalizer then consider both A and B eligilble for
finalization? I always assumed A was then eligible for finalization,
but B isn't.

~~ Robert.
Post by Xueming Shen
Post by Xueming Shen
Post by Jeroen Frijters
Post by Xueming Shen
I'm not a GC guy, so I might be missing something here, but if close()
is being explicitly invoked by some thread, means someone has a strong
reference to it, I don't think the finalize() can kick in until that
close() returns
This is not correct. You can re-publish the reference from another
finalizer method and thereby allow another thread to access the object
concurrently with the finalizer.
1) GC runs and determines that A and B are finalizable
2) Finalizer thread run A.finalize()
3) A.finalize publishes reference to B in static variable
Jeroen, are you talking about the object resurrection from finalize()?
How do you re-publish/get the reference to B inside A.finalize()? I think
you can do that inside
B's finalize() to assign "this" to a global static variable.
Jeroen,
Guess I replied too quick. I think I got what you meant. A has a reference
to B, ?GC finalizes A
and B the same time, then A republish reference to B and let someone else to
invoke b.close()..
I would have to admit it's a possible use scenario:-)
-Sherman
Post by Xueming Shen
Regard,
-Sherman
Post by Jeroen Frijters
4a) Another thread reads the static variable and calls B.close()
4b) Finalizer thread runs B.finalize()
Regards,
Jeroen
Loading...