RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
Hi, Please help review the change for JDK-8185582. issue: https://bugs.openjdk.java.net/browse/JDK-8185582 webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/ csr: https://bugs.openjdk.java.net/browse/JDK-8187485 The proposed change here is to replace the finalize() cleanup mechanism with the newly introduced java.lang.ref.Cleaner for java.util.zip package (Deflater, Inflater, ZipFile and ZipFile.ZipFileInputStrean/ZipFileInflaterInputStream). Since it's an "incompatible" change, both behavioral and source incompatibility, to remove the public finalize() from these classes, a corresponding CSR is also created and need review as well. Thanks, Sherman
Hi Sherman, At first I checked the Deflater/Inflater changes. I'll come back with ZipFile later. I think the code is mostly correct, but I have a concern. If clean() is invoked via Deflater.end(), then the Deflater instance is still alive and synchronization is necessary as other threads might be concurrently invoking other methods. If clean() is invoked via Cleaner thread, then Deflater instance is already phantom reachable and no thread may be accessing it or be in the middle of executing any of its instance methods. How is this guaranteed? Critical Deflater methods synchronize on the zsRef instance, so at least at the beginning of the synchronized block, the Deflater instance is strongly reachable. Take for example the following Deflater instance method: 254 public void setDictionary(byte[] b, int off, int len) { 255 if (b == null) { 256 throw new NullPointerException(); 257 } 258 if (off < 0 || len < 0 || off > b.length - len) { 259 throw new ArrayIndexOutOfBoundsException(); 260 } 261 synchronized (zsRef) { 262 ensureOpen(); 263 setDictionary(zsRef.address(), b, off, len); 264 } 265 } Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run(): 49 public void run() { 50 long addr = address; 51 address = 0; 52 if (addr != 0) { 53 end.accept(addr); 54 } 55 } Possible program execution is therefore: Thread1: zsRef.address() - returning non-zero address Thread2: ZStreamRef.run() - invoking Deflater.end(address) with non-zero address via the passed-in lambda. Thread1: setDictionary(<non-zero address from before>, b, off, len) To fix this, you could sprinkle Reference.reachabilityFence(this) at appropriate places in Deflater, but it is simpler to just add synchronized modifier to ZStreamRef.run() method. There's no danger of deadlock(s) since Cleaner ensures the run() method is invoked at most once. The same reasoning applies to Inflater to as code is similar. A nit (Deflater and similar to Inflater): 187 ZStreamRef ref = new ZStreamRef(init(level, DEFAULT_STRATEGY, nowrap), 188 addr -> end(addr)); You could use a method reference there. Regards, Peter On 09/27/2017 08:35 AM, Xueming Shen wrote:
Hi,
Please help review the change for JDK-8185582.
issue: https://bugs.openjdk.java.net/browse/JDK-8185582 webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/ csr: https://bugs.openjdk.java.net/browse/JDK-8187485
The proposed change here is to replace the finalize() cleanup mechanism with the newly introduced java.lang.ref.Cleaner for java.util.zip package (Deflater, Inflater, ZipFile and ZipFile.ZipFileInputStrean/ZipFileInflaterInputStream).
Since it's an "incompatible" change, both behavioral and source incompatibility, to remove the public finalize() from these classes, a corresponding CSR is also created and need review as well.
Thanks, Sherman
Hi Peter, I thought Cleaner was supposed to avoid all these unpredictable reachability races? Otherwise why switch from using a finalizer ?? David On 27/09/2017 7:31 PM, Peter Levart wrote:
Hi Sherman,
At first I checked the Deflater/Inflater changes. I'll come back with ZipFile later.
I think the code is mostly correct, but I have a concern. If clean() is invoked via Deflater.end(), then the Deflater instance is still alive and synchronization is necessary as other threads might be concurrently invoking other methods. If clean() is invoked via Cleaner thread, then Deflater instance is already phantom reachable and no thread may be accessing it or be in the middle of executing any of its instance methods. How is this guaranteed? Critical Deflater methods synchronize on the zsRef instance, so at least at the beginning of the synchronized block, the Deflater instance is strongly reachable. Take for example the following Deflater instance method:
254 public void setDictionary(byte[] b, int off, int len) { 255 if (b == null) { 256 throw new NullPointerException(); 257 } 258 if (off < 0 || len < 0 || off > b.length - len) { 259 throw new ArrayIndexOutOfBoundsException(); 260 } 261 synchronized (zsRef) { 262 ensureOpen(); 263 setDictionary(zsRef.address(), b, off, len); 264 } 265 }
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
49 public void run() { 50 long addr = address; 51 address = 0; 52 if (addr != 0) { 53 end.accept(addr); 54 } 55 }
Possible program execution is therefore:
Thread1:
zsRef.address() - returning non-zero address
Thread2:
ZStreamRef.run() - invoking Deflater.end(address) with non-zero address via the passed-in lambda.
Thread1:
setDictionary(<non-zero address from before>, b, off, len)
To fix this, you could sprinkle Reference.reachabilityFence(this) at appropriate places in Deflater, but it is simpler to just add synchronized modifier to ZStreamRef.run() method. There's no danger of deadlock(s) since Cleaner ensures the run() method is invoked at most once.
The same reasoning applies to Inflater to as code is similar.
A nit (Deflater and similar to Inflater):
187 ZStreamRef ref = new ZStreamRef(init(level, DEFAULT_STRATEGY, nowrap), 188 addr -> end(addr));
You could use a method reference there.
Regards, Peter
On 09/27/2017 08:35 AM, Xueming Shen wrote:
Hi,
Please help review the change for JDK-8185582.
issue: https://bugs.openjdk.java.net/browse/JDK-8185582 webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/ csr: https://bugs.openjdk.java.net/browse/JDK-8187485
The proposed change here is to replace the finalize() cleanup mechanism with the newly introduced java.lang.ref.Cleaner for java.util.zip package (Deflater, Inflater, ZipFile and ZipFile.ZipFileInputStrean/ZipFileInflaterInputStream).
Since it's an "incompatible" change, both behavioral and source incompatibility, to remove the public finalize() from these classes, a corresponding CSR is also created and need review as well.
Thanks, Sherman
Hi David, On 09/27/2017 11:52 AM, David Holmes wrote:
Hi Peter,
I thought Cleaner was supposed to avoid all these unpredictable reachability races? Otherwise why switch from using a finalizer ??
Unfortunately, there is no hidden magic in Cleaner. It is better than finalize() mostly because the clean-up function may be invoked by the user, which also de-registers it, allowing GC to collect it rather than leaving it to reference-processing pipeline to process it for no reason. And of course, it guarantees that the tracked referent can not be resurrected as a result of cleanup code execution. What remains unchanged with Cleaner is the point in program execution when the tracked referent is determined to be phantom-reachable (finalizable) and therefore its associated PhantomReference(s) eligible for processing (finalize() invoked). Regards, Peter
David
On 27/09/2017 7:31 PM, Peter Levart wrote:
Hi Sherman,
At first I checked the Deflater/Inflater changes. I'll come back with ZipFile later.
I think the code is mostly correct, but I have a concern. If clean() is invoked via Deflater.end(), then the Deflater instance is still alive and synchronization is necessary as other threads might be concurrently invoking other methods. If clean() is invoked via Cleaner thread, then Deflater instance is already phantom reachable and no thread may be accessing it or be in the middle of executing any of its instance methods. How is this guaranteed? Critical Deflater methods synchronize on the zsRef instance, so at least at the beginning of the synchronized block, the Deflater instance is strongly reachable. Take for example the following Deflater instance method:
254 public void setDictionary(byte[] b, int off, int len) { 255 if (b == null) { 256 throw new NullPointerException(); 257 } 258 if (off < 0 || len < 0 || off > b.length - len) { 259 throw new ArrayIndexOutOfBoundsException(); 260 } 261 synchronized (zsRef) { 262 ensureOpen(); 263 setDictionary(zsRef.address(), b, off, len); 264 } 265 }
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
49 public void run() { 50 long addr = address; 51 address = 0; 52 if (addr != 0) { 53 end.accept(addr); 54 } 55 }
Possible program execution is therefore:
Thread1:
zsRef.address() - returning non-zero address
Thread2:
ZStreamRef.run() - invoking Deflater.end(address) with non-zero address via the passed-in lambda.
Thread1:
setDictionary(<non-zero address from before>, b, off, len)
To fix this, you could sprinkle Reference.reachabilityFence(this) at appropriate places in Deflater, but it is simpler to just add synchronized modifier to ZStreamRef.run() method. There's no danger of deadlock(s) since Cleaner ensures the run() method is invoked at most once.
The same reasoning applies to Inflater to as code is similar.
A nit (Deflater and similar to Inflater):
187 ZStreamRef ref = new ZStreamRef(init(level, DEFAULT_STRATEGY, nowrap), 188 addr -> end(addr));
You could use a method reference there.
Regards, Peter
On 09/27/2017 08:35 AM, Xueming Shen wrote:
Hi,
Please help review the change for JDK-8185582.
issue: https://bugs.openjdk.java.net/browse/JDK-8185582 webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/ csr: https://bugs.openjdk.java.net/browse/JDK-8187485
The proposed change here is to replace the finalize() cleanup mechanism with the newly introduced java.lang.ref.Cleaner for java.util.zip package (Deflater, Inflater, ZipFile and ZipFile.ZipFileInputStrean/ZipFileInflaterInputStream).
Since it's an "incompatible" change, both behavioral and source incompatibility, to remove the public finalize() from these classes, a corresponding CSR is also created and need review as well.
Thanks, Sherman
Hi Peter, Sorry, I might not understand your use scenario correctly. Let me try :-) If clean() is invoked via Deflater.end() first, my reading of the Cleaner code suggests that the clean()->ZStreamRef.run() is run by the thread calling deflater.end() directly, not the Cleaner thread. In this scenario, since we are still inside the synchronized(zsRef) block, all other "zsRef" sensitive deflater methods should be blocked by the same synchronzied(zeRef) ? I would assume we don't have problem in this use scenario? 567 public void end() { 568 synchronized (zsRef) { 569 cleanable.clean(); 570 buf = null; 571 } 572 } If some other method is invoked first, for example the setDictionary(...) as in your sample, any direct invocation of delfater.end() from other thread should be blocked because of "synchronized(zsRef)". So I would assume this is not a concern. It seems the concern is that the "Deflater.this" object itself is no longer needed and becomes unreachable after line#261, therefore this deflater becomes phantom-reachable and the ZStreamRef.run() is called by the Cleaner thread, then we have a race condition between the thread calls the setDictionary() and the Cleaner thread? I guess I might have a wrong assumption here, is it true that if someone/thread is calling deflater.setDictionary(), that someone/thread should still hold a reference to the deflater and therefore prevent it from becoming phantom-reachable? Or you are saying it is possible under the hotspot optimization the deflater.setDictionary() (and its ensureOpen() included) is being inline-ed and therefore there is no more reference to that deflater even we are still inside that deflater.setDictionary(), so that "after ine#261" scenario becomes possible? Thanks, Sherman On 9/27/17, 2:31 AM, Peter Levart wrote:
Hi Sherman,
At first I checked the Deflater/Inflater changes. I'll come back with ZipFile later.
I think the code is mostly correct, but I have a concern. If clean() is invoked via Deflater.end(), then the Deflater instance is still alive and synchronization is necessary as other threads might be concurrently invoking other methods. If clean() is invoked via Cleaner thread, then Deflater instance is already phantom reachable and no thread may be accessing it or be in the middle of executing any of its instance methods. How is this guaranteed? Critical Deflater methods synchronize on the zsRef instance, so at least at the beginning of the synchronized block, the Deflater instance is strongly reachable. Take for example the following Deflater instance method:
254 public void setDictionary(byte[] b, int off, int len) { 255 if (b == null) { 256 throw new NullPointerException(); 257 } 258 if (off < 0 || len < 0 || off > b.length - len) { 259 throw new ArrayIndexOutOfBoundsException(); 260 } 261 synchronized (zsRef) { 262 ensureOpen(); 263 setDictionary(zsRef.address(), b, off, len); 264 } 265 }
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
49 public void run() { 50 long addr = address; 51 address = 0; 52 if (addr != 0) { 53 end.accept(addr); 54 } 55 }
Possible program execution is therefore:
Thread1:
zsRef.address() - returning non-zero address
Thread2:
ZStreamRef.run() - invoking Deflater.end(address) with non-zero address via the passed-in lambda.
Thread1:
setDictionary(<non-zero address from before>, b, off, len)
To fix this, you could sprinkle Reference.reachabilityFence(this) at appropriate places in Deflater, but it is simpler to just add synchronized modifier to ZStreamRef.run() method. There's no danger of deadlock(s) since Cleaner ensures the run() method is invoked at most once.
The same reasoning applies to Inflater to as code is similar.
A nit (Deflater and similar to Inflater):
187 ZStreamRef ref = new ZStreamRef(init(level, DEFAULT_STRATEGY, nowrap), 188 addr -> end(addr));
You could use a method reference there.
Regards, Peter
On 09/27/2017 08:35 AM, Xueming Shen wrote:
Hi,
Please help review the change for JDK-8185582.
issue: https://bugs.openjdk.java.net/browse/JDK-8185582 webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev/ csr: https://bugs.openjdk.java.net/browse/JDK-8187485
The proposed change here is to replace the finalize() cleanup mechanism with the newly introduced java.lang.ref.Cleaner for java.util.zip package (Deflater, Inflater, ZipFile and ZipFile.ZipFileInputStrean/ZipFileInflaterInputStream).
Since it's an "incompatible" change, both behavioral and source incompatibility, to remove the public finalize() from these classes, a corresponding CSR is also created and need review as well.
Thanks, Sherman
On 9/27/17 2:31 AM, Peter Levart wrote:
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
What about making the native setDictionary method as an instance method (currently it's a static method) so that this object remains strongly reachable until the method returns? Mandy
----- Mail original -----
De: "mandy chung" <mandy.chung@oracle.com> À: "Peter Levart" <peter.levart@gmail.com>, "Xueming Shen" <xueming.shen@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 22:34:52 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
On 9/27/17 2:31 AM, Peter Levart wrote:
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
What about making the native setDictionary method as an instance method (currently it's a static method) so that this object remains strongly reachable until the method returns?
Mandy, unlike in C or C++, in Java a reference is garbage collected as soon as you do not need it anymore, so using an instance method will not change the issue here. one way to be sure that a referenced object is not garbage collected is to use http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reacha...
Mandy
Rémi
On 9/29/17 1:49 PM, Remi Forax wrote:
On 9/27/17 2:31 AM, Peter Levart wrote:
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run(): What about making the native setDictionary method as an instance method (currently it's a static method) so that this object remains strongly reachable until the method returns? Mandy, unlike in C or C++, in Java a reference is garbage collected as soon as you do not need it anymore, so using an instance method will not change the issue here.
The case that Peter observed is when "this" is being optimized out and becomes unreachable before setDictionary is called. Since setDictionary is a JNI function, the caller has to pass this (as jobject) to the native function. Would that cover this special case? Mandy
Hi Remi, On 09/29/17 22:49, Remi Forax wrote:
----- Mail original -----
De: "mandy chung" <mandy.chung@oracle.com> À: "Peter Levart" <peter.levart@gmail.com>, "Xueming Shen" <xueming.shen@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 22:34:52 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers On 9/27/17 2:31 AM, Peter Levart wrote:
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run(): What about making the native setDictionary method as an instance method (currently it's a static method) so that this object remains strongly reachable until the method returns? Mandy, unlike in C or C++, in Java a reference is garbage collected as soon as you do not need it anymore, so using an instance method will not change the issue here.
I might be wrong, but native instance method is an exception. It can't be inlined. The preparation for native method invocation makes sure 'this' is kept reachable because it can be dereferenced from the native code then and native code is out-of-bounds for JIT optimization. Regards, Peter
one way to be sure that a referenced object is not garbage collected is to use http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reacha...
Mandy Rémi
De: "Peter Levart" <peter.levart@gmail.com> À: "Remi Forax" <forax@univ-mlv.fr>, "mandy chung" <mandy.chung@oracle.com> Cc: "Xueming Shen" <xueming.shen@oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 22:56:26 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
Hi Remi, Hi Peter,
On 09/29/17 22:49, Remi Forax wrote:
----- Mail original -----
De: "mandy chung" [ mailto:mandy.chung@oracle.com | <mandy.chung@oracle.com> ] À: "Peter Levart" [ mailto:peter.levart@gmail.com | <peter.levart@gmail.com> ] , "Xueming Shen" [ mailto:xueming.shen@oracle.com | <xueming.shen@oracle.com> ] , "core-libs-dev" [ mailto:core-libs-dev@openjdk.java.net | <core-libs-dev@openjdk.java.net> ] Envoyé: Vendredi 29 Septembre 2017 22:34:52 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
On 9/27/17 2:31 AM, Peter Levart wrote:
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
What about making the native setDictionary method as an instance method (currently it's a static method) so that this object remains strongly reachable until the method returns?
Mandy, unlike in C or C++, in Java a reference is garbage collected as soon as you do not need it anymore, so using an instance method will not change the issue here.
I might be wrong, but native instance method is an exception. It can't be inlined. The preparation for native method invocation makes sure 'this' is kept reachable because it can be dereferenced from the native code then and native code is out-of-bounds for JIT optimization. I do not think that you are wrong now but this is a property (a side effect) of the current implementation, Panama and Metropolis may change that. I think it's better to keep the reference available with a reachability fence.
Regards, Peter Rémi
one way to be sure that a referenced object is not garbage collected is to use [ http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reacha... | http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reacha... ] -
Mandy
Rémi
On 09/29/17 22:34, mandy chung wrote:
On 9/27/17 2:31 AM, Peter Levart wrote:
Up to a point where 'this' is dereferenced to obtain the 'zsRef' value (line 261), the Deflater instance is reachable. But after that, even ensureOpen() may be inlined and 'this' is not needed any more. After that point, obtaining zsRef.address() and calling setDictionaly on the obtained address may be racing with Cleaner thread invoking ZStreamRef.run():
What about making the native setDictionary method as an instance method (currently it's a static method) so that this object remains strongly reachable until the method returns?
Mandy
This is a possibility too, yes. In general there might be other places where the same could be performed. It is equivalent to puting Reference.reachabilityFence(this) after the invocation of setDictionary() static native method. But this would only fix public setDictionary() instance method. There might be other public methods with similar problems. Synchronizing the ZStreamRef.run() method fixes all of them in one place. Regards, Peter
Comment on the CSR: On 9/26/17 11:35 PM, Xueming Shen wrote:
I think the @apiNote can be simpler. Deflater (similar comment for Inflater) |* @apiNote * In earlier versions, the {@link Object#finalize} method was overridden * to call the {@link #end} method when a {@code Deflater} object * becomes unreachable. * The {@link #finalize} method is no longer defined. If a subclass * overrides ||the {@code end} method, the overridden {@code end} method * is no longer invoked. * <p> * It is strongly recommended to explicitly call {@code end} to ||* discard any unprocessed input promptly to free up resources |* when |||the compressor |is no longer in use.| |ZipFile * @apiNote |* In earlier versions, the {@link Object#finalize} method was overridden * to call the {@link #close} method when a {@code ZipFile} object * becomes unreachable.| |* The {@link #finalize} method is no longer defined. If a subclass * overrides ||the {@code close} method, the overridden {@code close} method * is no longer invoked.| * <p> |* The recommended cleanup for |||{@code ZipFile}| is to explicitly call {@code close} * or use try-with-resources.| 657 * <p> 658 * Since the time when the resources held by this object will be released 659 * is undetermined, if this method is not invoked explicitly, it is strongly 660 * recommended that applications invoke this method as soon they have 661 * finished accessing this {@code ZipFile}. This will prevent holding up 662 * system resources for an undetermined length of time. 663 * <p> I would suggest to drop this paragraph. @apiNote and @implNote in class spec cover that. Mandy ||
Thanks Mandy! I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me. https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev -Sherman On 9/27/17, 3:08 PM, mandy chung wrote:
Comment on the CSR:
On 9/26/17 11:35 PM, Xueming Shen wrote:
I think the @apiNote can be simpler.
Deflater (similar comment for Inflater) | * @apiNote * In earlier versions, the {@link Object#finalize} method was overridden * to call the {@link #end} method when a {@code Deflater} object * becomes unreachable. * The {@link #finalize} method is no longer defined. If a subclass * overrides||the {@code end} method, the overridden {@code end} method * is no longer invoked. *<p> * It is strongly recommended to explicitly call {@code end} to || * discard any unprocessed input promptly to free up resources | * when|||the compressor|is no longer in use.|
|ZipFile
* @apiNote | * In earlier versions, the {@link Object#finalize} method was overridden * to call the {@link #close} method when a {@code ZipFile} object * becomes unreachable.| | * The {@link #finalize} method is no longer defined. If a subclass * overrides||the {@code close} method, the overridden {@code close} method * is no longer invoked.| *<p> | * The recommended cleanup for|||{@code ZipFile}| is to explicitly call {@code close} * or use try-with-resources.|
657 *<p> 658 * Since the time when the resources held by this object will be released 659 * is undetermined, if this method is not invoked explicitly, it is strongly 660 * recommended that applications invoke this method as soon they have 661 * finished accessing this {@code ZipFile}. This will prevent holding up 662 * system resources for an undetermined length of time. 663 *<p>
I would suggest to drop this paragraph. @apiNote and @implNote in class spec cover that.
Mandy ||
Hi Sherman, I looked into ZipFile as promised. One thing I noticed is that FinalizeZipFile.java test fails compilation: test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone). The other thing I noticed is that Releaser 1st closes the streams (that are still reachable via streams WeakHashMap) and also ends the associated inflaters. But closing the stream will already release the inflater (in case it is a ZipFileInflaterInputStream) into the inflaters cache and the cache is scanned and inflaters ended later. So we don't need a stream -> inflater association outside the stream in the form of WeekHashMap. But we still need to keep the set of input streams weakly reachable from ZipFile in case we want to close the ZipFile explicitly (and there is no harm although useless if this also happens as a result of automatic ZipFile cleaner processing). This could be implemented in a form of: final Set<InputStream> streams = Collections.newSetFromMap(new WeakHashMap<>()); I also noticed that it is useless to test whether the inflater is ended() when obtaining it from or releasing it into cache if the code keeps the invariant that it never ends inflaters while they are still in cache or associated with the open stream (the only place where inflaters should be ended explicitly is in the Releaser). To make this even more obvious, it might be good to move the obtaining/releasing logic directly into the ZipFileInflaterInputStream constructor which would be passed a reference to the inflatersCache instead of the Inflater instance. Here's what I have in mind (I cahnged just the ZipFile and FinalizeZipFile): http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/ What do you think? Because Inflaters used in ZipFile will never be automatically ended by their own cleaners (they are kept strongly reachable in the cache of inflaters, which is strongly reachable from the registered ZipFile cleanup function), it might be useful to add a special package-private constructor to Inflater which would not register its own cleaner. But this could be left as an exercise for some later time. Regards, Peter On 09/28/17 01:41, Xueming Shen wrote:
Thanks Mandy!
I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev
-Sherman
On 9/27/17, 3:08 PM, mandy chung wrote:
Comment on the CSR:
On 9/26/17 11:35 PM, Xueming Shen wrote:
I think the @apiNote can be simpler.
Deflater (similar comment for Inflater) | * @apiNote * In earlier versions, the {@link Object#finalize} method was overridden * to call the {@link #end} method when a {@code Deflater} object * becomes unreachable. * The {@link #finalize} method is no longer defined. If a subclass * overrides||the {@code end} method, the overridden {@code end} method * is no longer invoked. *<p> * It is strongly recommended to explicitly call {@code end} to || * discard any unprocessed input promptly to free up resources | * when|||the compressor|is no longer in use.|
|ZipFile * @apiNote | * In earlier versions, the {@link Object#finalize} method was overridden * to call the {@link #close} method when a {@code ZipFile} object * becomes unreachable.| | * The {@link #finalize} method is no longer defined. If a subclass * overrides||the {@code close} method, the overridden {@code close} method * is no longer invoked.| *<p> | * The recommended cleanup for|||{@code ZipFile}| is to explicitly call {@code close} * or use try-with-resources.|
657 *<p> 658 * Since the time when the resources held by this object will be released 659 * is undetermined, if this method is not invoked explicitly, it is strongly 660 * recommended that applications invoke this method as soon they have 661 * finished accessing this {@code ZipFile}. This will prevent holding up 662 * system resources for an undetermined length of time. 663 *<p>
I would suggest to drop this paragraph. @apiNote and @implNote in class spec cover that.
Mandy ||
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable. Will return to the other comments later. Thanks! -sherman
On 9/29/17 1:49 PM, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable.
I would suggest to update InstrumentedZipFile to migrate away from the finalizer. I can override the close method instead of the finalize method. It can test explicitly calling close (that's what the test is currently doing) and try-with-resource. Mandy
fyi, The proposed[1] changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses. We may need retain the finalize methods (with empty bodies) to mitigate source compatibility. Roger [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.h... On 9/29/2017 4:49 PM, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable.
Will return to the other comments later.
Thanks! -sherman
Hi Roger, On 09/29/17 22:55, Roger Riggs wrote:
fyi,
The proposed[1] changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses. We may need retain the finalize methods (with empty bodies) to mitigate source compatibility.
The problem is that empty finalize() method that throws anything other than Throwable will not compile. Regards, Peter
Roger
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.h...
On 9/29/2017 4:49 PM, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable.
Will return to the other comments later.
Thanks! -sherman
On 09/29/17 23:11, Peter Levart wrote:
Hi Roger,
On 09/29/17 22:55, Roger Riggs wrote:
fyi,
The proposed[1] changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses. We may need retain the finalize methods (with empty bodies) to mitigate source compatibility.
The problem is that empty finalize() method that throws anything other than Throwable will not compile.
Correction - it will compile (I was thinking about a method calling just super.finalize() which is not empty, of course). Yes, this is the solution to source compatibility.
Regards, Peter
Roger
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.h...
On 9/29/2017 4:49 PM, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable.
Will return to the other comments later.
Thanks! -sherman
----- Mail original -----
De: "Peter Levart" <peter.levart@gmail.com> À: "Roger Riggs" <Roger.Riggs@Oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 23:14:33 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
On 09/29/17 23:11, Peter Levart wrote:
Hi Roger,
On 09/29/17 22:55, Roger Riggs wrote:
fyi,
The proposed[1] changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses. We may need retain the finalize methods (with empty bodies) to mitigate source compatibility.
The problem is that empty finalize() method that throws anything other than Throwable will not compile.
Correction - it will compile (I was thinking about a method calling just super.finalize() which is not empty, of course). Yes, this is the solution to source compatibility.
Overriding finalize() will make the object not to be GCed as soon as it should.
Regards, Peter
Rémi
Roger
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.h...
On 9/29/2017 4:49 PM, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable.
Will return to the other comments later.
Thanks! -sherman
On 09/29/17 23:23, Remi Forax wrote:
----- Mail original -----
De: "Peter Levart" <peter.levart@gmail.com> À: "Roger Riggs" <Roger.Riggs@Oracle.com>, "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 23:14:33 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers On 09/29/17 23:11, Peter Levart wrote:
Hi Roger,
On 09/29/17 22:55, Roger Riggs wrote:
fyi,
The proposed[1] changes to FileInputStream and FileOutputStream remove the finalize method exposing Object.finalize (throws Throwable) to subclasses. We may need retain the finalize methods (with empty bodies) to mitigate source compatibility. The problem is that empty finalize() method that throws anything other than Throwable will not compile. Correction - it will compile (I was thinking about a method calling just super.finalize() which is not empty, of course). Yes, this is the solution to source compatibility.
Overriding finalize() will make the object not to be GCed as soon as it should.
I think the hotspot has an optimization and detects that finalize() has no bytecodes and treats the object as not needing finalization. Regards, Peter
Regards, Peter
Rémi
Roger
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-September/049351.h...
On 9/29/2017 4:49 PM, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
Yes, it's the expected source incompatibility issue specified in the CSR request. I think I had it changed somewhere but obviously it's not in the webrev. Thanks for catching it. Yes, the test needs to update to be catch the throwable.
Will return to the other comments later.
Thanks! -sherman
On 9/29/17 2:38 PM, Peter Levart wrote:
I think the hotspot has an optimization and detects that finalize() has no bytecodes and treats the object as not needing finalization. http://hg.openjdk.java.net/jdk10/master/file/7d67bb6b0599/src/hotspot/share/...
// Check if this klass has an empty finalize method (i.e. one with return bytecode only), // in which case we don't have to register objects as finalizable if (!_has_empty_finalizer) { if (_has_finalizer || (super != NULL && super->has_finalizer())) { ik->set_has_finalizer(); } } Mandy
Very cool, i learn something today :) Rémi ----- Mail original -----
De: "mandy chung" <mandy.chung@oracle.com> À: "core-libs-dev" <core-libs-dev@openjdk.java.net> Envoyé: Vendredi 29 Septembre 2017 23:45:18 Objet: Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers
On 9/29/17 2:38 PM, Peter Levart wrote:
I think the hotspot has an optimization and detects that finalize() has no bytecodes and treats the object as not needing finalization. http://hg.openjdk.java.net/jdk10/master/file/7d67bb6b0599/src/hotspot/share/...
// Check if this klass has an empty finalize method (i.e. one with return bytecode only), // in which case we don't have to register objects as finalizable if (!_has_empty_finalizer) { if (_has_finalizer || (super != NULL && super->has_finalizer())) { ik->set_has_finalizer(); } }
Mandy
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman, putStream> streams = Collections.newSetFromMap(new WeakHashMap<>());
I also noticed that it is useless to test whether the inflater is ended() when obtaining it from or releasing it into cache if the code keeps the invariant that it never ends inflaters while they are still in cache or associated with the open stream (the only place where inflaters should be ended explicitly is in the Releaser). To make this even more obvious, it might be good to move the obtaining/releasing logic directly into the ZipFileInflaterInputStream constructor which would be passed a reference to the inflatersCache instead of the Inflater instance.
If my memory serves me correctly, this "ended()" check was for the situation that the ZipfileInflaterStream is getting finalized (close() is not called explicitly/directly) and its inflater is also getting finalized at the same time/pass, and the inflater.end() gets called (by its finalize()) first, so when the zfis tries to release the inflater back to the cache, it has been "ended" already. We had some nasty finalize() and object resurrection timing issue in earlier releases, that probably is part of the fix (since it's a timing issue, we also do the same "ended()" check in getInflater() when lease it out) So my question is that do we have the similar situation under the Cleaner? mean the zfis and its inflater both are phantom reachable and the inflater's cleaner gets called first? I'm still reading your proposed change to remove the "inflater" from the stream->inflater mapping to see if there is any loophole, will reply that separately later. -sherman
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
The other thing I noticed is that Releaser 1st closes the streams (that are still reachable via streams WeakHashMap) and also ends the associated inflaters. But closing the stream will already release the inflater (in case it is a ZipFileInflaterInputStream) into the inflaters cache and the cache is scanned and inflaters ended later.
So we don't need a stream -> inflater association outside the stream in the form of WeekHashMap. But we still need to keep the set of input streams weakly reachable from ZipFile in case we want to close the ZipFile explicitly (and there is no harm although useless if this also happens as a result of automatic ZipFile cleaner processing).
This could be implemented in a form of:
final Set<InputStream> streams = Collections.newSetFromMap(new WeakHashMap<>());
I also noticed that it is useless to test whether the inflater is ended() when obtaining it from or releasing it into cache if the code keeps the invariant that it never ends inflaters while they are still in cache or associated with the open stream (the only place where inflaters should be ended explicitly is in the Releaser). To make this even more obvious, it might be good to move the obtaining/releasing logic directly into the ZipFileInflaterInputStream constructor which would be passed a reference to the inflatersCache instead of the Inflater instance.
Here's what I have in mind (I cahnged just the ZipFile and FinalizeZipFile):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/
What do you think?
Peter, I read those old emails back to 2011 on why we put the "Inflater" into the streams as the value of the map entry. It appears the main (only) purpose is to keep the finalization of <stream, inflater> in order. As I explained in previous email, stream and its inflater can be phantom reachable at same round, if inflater gets finalized/ended first, then it can no longer be reused/cached and has to be thrown away, which means the caching mechanism is actually broken/not functioning. We do have use scenario depends on it to avoid too many "new Inflater()' that might pressure the memory usage. Putting the pair in a weakhashmap appears to solve this problem back then (the ended() checks are still there just in case there is rare case, the inflater still gets cleaned up first). The situation might be changed now in Cleaner case, as the cleaner object itself now holds a strong reference, which in theory will/should prevent the inflater from being phantom reachable before stream is cleaned, so we might no longer need this <stream, inflater> pair in a map to have a "ordered" cleanup. Just wanted to make sure my assumption is correct here. sherman
Hi Sherman, On 09/30/17 02:02, Xueming Shen wrote:
On 9/29/17, 1:18 PM, Peter Levart wrote:
Hi Sherman,
I looked into ZipFile as promised.
One thing I noticed is that FinalizeZipFile.java test fails compilation:
test/jdk/java/util/zip/ZipFile/FinalizeZipFile.java:49: error: unreported exception Throwable; must be caught or declared to be thrown super.finalize(); ^ (the overridden finalize() in InstrumentedZipFile should now declare throws Throwable, since it overrides Object.finalize() and not ZipFile.finalize() which is gone).
The other thing I noticed is that Releaser 1st closes the streams (that are still reachable via streams WeakHashMap) and also ends the associated inflaters. But closing the stream will already release the inflater (in case it is a ZipFileInflaterInputStream) into the inflaters cache and the cache is scanned and inflaters ended later.
So we don't need a stream -> inflater association outside the stream in the form of WeekHashMap. But we still need to keep the set of input streams weakly reachable from ZipFile in case we want to close the ZipFile explicitly (and there is no harm although useless if this also happens as a result of automatic ZipFile cleaner processing).
This could be implemented in a form of:
final Set<InputStream> streams = Collections.newSetFromMap(new WeakHashMap<>());
I also noticed that it is useless to test whether the inflater is ended() when obtaining it from or releasing it into cache if the code keeps the invariant that it never ends inflaters while they are still in cache or associated with the open stream (the only place where inflaters should be ended explicitly is in the Releaser). To make this even more obvious, it might be good to move the obtaining/releasing logic directly into the ZipFileInflaterInputStream constructor which would be passed a reference to the inflatersCache instead of the Inflater instance.
Here's what I have in mind (I cahnged just the ZipFile and FinalizeZipFile):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.01/
What do you think?
Peter,
I read those old emails back to 2011 on why we put the "Inflater" into the streams as the value of the map entry. It appears the main (only) purpose is to keep the finalization of <stream, inflater> in order. As I explained in previous email, stream and its inflater can be phantom reachable at same round, if inflater gets finalized/ended first, then it can no longer be reused/cached and has to be thrown away, which means the caching mechanism is actually broken/not functioning. We do have use scenario depends on it to avoid too many "new Inflater()' that might pressure the memory usage. Putting the pair in a weakhashmap appears to solve this problem back then (the ended() checks are still there just in case there is rare case, the inflater still gets cleaned up first).
The situation might be changed now in Cleaner case, as the cleaner object itself now holds a strong reference, which in theory will/should prevent the inflater from being phantom reachable before stream is cleaned, so we might no longer need this <stream, inflater> pair in a map to have a "ordered" cleanup. Just wanted to make sure my assumption is correct here.
sherman
Right, the Inflater is captured by the cleaning function for the ZipFileInflaterInputStream and is kept strongly reachable until the function fires, which may happen automatically or manually. At that time, it is returned to the inflaters cache, which is rooted at the ZipFile instance and also captured by the ZipFile cleaning function - the Releaser, which is strongly reachable until fired. So I claim that there is no possibility for Inflaters used in ZipFile to ever get cleaned-up (ended) automatically by their cleaner functions as a result of getting phantom reachable. The situation has changed. It could even be beneficial to create a package-private Inflater constructor for this case, which creates Inflaters without cleaner registration. Regards, Peter
Peter, Webrev has been updated accordingly to remove the "inflater" from the "streams" as the "value" of the map. As suggested, the "streams" now itself is a "set". And, a package private "Infater()" has been added to avoid registering the inflater to Cleaner for those inflaters from ZipFile. Many, The @apiNote has been updated accordingly to remove the words regarding "finalize() overriding". http://cr.openjdk.java.net/~sherman/8185582/webrev/ Thanks, Sherman On 9/30/17, 12:41 AM, Peter Levart wrote:
Right, the Inflater is captured by the cleaning function for the ZipFileInflaterInputStream and is kept strongly reachable until the function fires, which may happen automatically or manually. At that time, it is returned to the inflaters cache, which is rooted at the ZipFile instance and also captured by the ZipFile cleaning function - the Releaser, which is strongly reachable until fired. So I claim that there is no possibility for Inflaters used in ZipFile to ever get cleaned-up (ended) automatically by their cleaner functions as a result of getting phantom reachable. The situation has changed. It could even be beneficial to create a package-private Inflater constructor for this case, which creates Inflaters without cleaner registration.
Regards, Peter
Hi Sherman, On 10/11/2017 12:22 AM, Xueming Shen wrote:
Peter,
Webrev has been updated accordingly to remove the "inflater" from the "streams" as the "value" of the map. As suggested, the "streams" now itself is a "set".
And, a package private "Infater()" has been added to avoid registering the inflater to Cleaner for those inflaters from ZipFile.
Many,
The @apiNote has been updated accordingly to remove the words regarding "finalize() overriding".
http://cr.openjdk.java.net/~sherman/8185582/webrev/
Thanks, Sherman
This looks good to me. Regards, Peter
On 9/30/17, 12:41 AM, Peter Levart wrote:
Right, the Inflater is captured by the cleaning function for the ZipFileInflaterInputStream and is kept strongly reachable until the function fires, which may happen automatically or manually. At that time, it is returned to the inflaters cache, which is rooted at the ZipFile instance and also captured by the ZipFile cleaning function - the Releaser, which is strongly reachable until fired. So I claim that there is no possibility for Inflaters used in ZipFile to ever get cleaned-up (ended) automatically by their cleaner functions as a result of getting phantom reachable. The situation has changed. It could even be beneficial to create a package-private Inflater constructor for this case, which creates Inflaters without cleaner registration.
Regards, Peter
On 9/27/17 4:41 PM, Xueming Shen wrote:
Thanks Mandy!
I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev ||
76 * specified to call the {@code end} method to close the {@code deflater} and s/deflater/Deflater 80 * The recommended cleanup for compressor is to explicitly call {@code end} 81 * method when it is no longer in use. Existing subclasses of {@code Deflater} 82 * that override {@code end} and require {@code end} to be invoked when the 83 * instance is unreachable should explicitly override {@link Object#finalize} 84 * and call {@code end}. I suggest not to recommend "explicitly override Object.finalize" (in fact, we should discourage it) and the overridden end method should be called explicitly. This was what I suggested in the previous mail:|| |* It is strongly recommended to explicitly call {@code end} to ||* discard any unprocessed input promptly to free up resources |* when |||the compressor |is no longer in use.| ||Same comment applies to Inflater. 75 * specified to call the {@code end} method to close the {@code inflater} and | s/inflater/Inflater FinalizeZipFile.zip (I have mentioned this in the other mail): I suggest to update InstrumentedZipFile to migrate away from the finalizer. I can override the close method instead of the finalize method. It can test explicitly calling close (that's what the test is currently doing) and try-with-resource. TestCleaner.java 130 throw new RuntimeException("'ZipFile.Source.zfile' is not accesible"); s/accesible/accessible Mandy
On 9/29/17, 1:58 PM, mandy chung wrote:
On 9/27/17 4:41 PM, Xueming Shen wrote:
Thanks Mandy!
I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev ||
76 * specified to call the {@code end} method to close the {@code deflater} and
s/deflater/Deflater 80 * The recommended cleanup for compressor is to explicitly call {@code end} 81 * method when it is no longer in use. Existing subclasses of {@code Deflater} 82 * that override {@code end} and require {@code end} to be invoked when the 83 * instance is unreachable should explicitly override {@link Object#finalize} 84 * and call {@code end}. I suggest not to recommend "explicitly override Object.finalize" (in fact, we should discourage it) and the overridden end method should be called explicitly. This was what I suggested in the previous mail:
"calling end() directly/explicitly to release the resource" is being recommended at the first sentence. The second sentence here is meant to provide a possible alternative if any existing subclass is implemented the way that it has the dependency on the old mechanism that its own "end()" being called, for situation that the Cleaner is not possible, or difficult to implement. No value at all? -Sherman
|| | * It is strongly recommended to explicitly call {@code end} to || * discard any unprocessed input promptly to free up resources | * when|||the compressor|is no longer in use.|
||Same comment applies to Inflater.
75 * specified to call the {@code end} method to close the {@code inflater} and |
* Xueming Shen:
I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev
In ZipFile: 387 Inflater inf = getInflater(); 388 InputStream is = new ZipFileInflaterInputStream(in, inf, (int)size, 389 () -> releaseInflater(inf)); 390 synchronized (streams) { 391 streams.add(is); 392 } Doesn't this leak the inflater if Cleaner.register() and thus the ZipFileInflaterInputStream constructor throw?
On 10/28/17, 7:16 AM, Florian Weimer wrote:
* Xueming Shen:
I removed the ln#657-#663, and updated the @apiNote in deflter, inflater and zipfile accordingly, mainly combined your comment and the approach for the fis/fo. they are "simpler" and straightforward now, at least for me.
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile:
387 Inflater inf = getInflater(); 388 InputStream is = new ZipFileInflaterInputStream(in, inf, (int)size, 389 () -> releaseInflater(inf)); 390 synchronized (streams) { 391 streams.add(is); 392 }
Doesn't this leak the inflater if Cleaner.register() and thus the ZipFileInflaterInputStream constructor throw?
Hi Florian, thanks for reviewing. The answer is "yes", if Cleaner.register() fails and throws a runtime exception ( internal error probably?) somewhere inside its implementation. I'm now an expert on Cleaner's implementation, but the "register" operation looks straightforward, creating the phantom ref object and "inserting" the newly created phantom ref into the queue, which appears unlikely to fail easily. The "assumption" of taking the approach to move away from finalize() to cleaner is that "Cleaner.register()" should work as reliable as finalizer. But in situation it really fails, I would assume there is really nothing can be done to save the world here. Similar to the situation like "free()" fails (for the "calloc()/free()" pair) and the memory to be deallocated might be left "un-freed"? In the latest version (this webrev) the inflater's default "cleaner" is turned off via a package private constructor, with the assumption that it is guaranteed that the inflater will always be putback into the "inflaterCache" by the corresponding ZFIIS cleaner and all inflaters inside "inflaterCache" is guaranteed to be cleaned up via the ZipFile cleaner. So yes arguably we are less safe by removing that (if this is the concern) But if Cleaner.register() here can fail, there is no guarantee inflater's default Cleaner.register() will not fail. It might be possible to try-catch to expect Cleaner.register might fail, but I doubt it's really necessary here and it is the recommended usage of Cleaner? Thanks! -Sherman
* Xueming Shen:
It might be possible to try-catch to expect Cleaner.register might fail, but I doubt it's really necessary here and it is the recommended usage of Cleaner?
That is actually why I posted this. 8-) For similar functionality in other languages, it is often necessary to perform all the required allocations for cleanup registration upfront, then allocate the resource, and finally install it in the cleanup handler. A straightforward translation to the Cleaner facility and the CleaningExample in the documentation would not allocate the state-to-be-cleaned in the State constructor, but in a separate method which is called after Cleaner.register(). But I don't know if this could run into any concurrency issues.
Hi Florian, On 10/28/17 16:16, Florian Weimer wrote:
* Xueming Shen:
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile:
387 Inflater inf = getInflater(); 388 InputStream is = new ZipFileInflaterInputStream(in, inf, (int)size, 389 () -> releaseInflater(inf)); 390 synchronized (streams) { 391 streams.add(is); 392 }
Doesn't this leak the inflater if Cleaner.register() and thus the ZipFileInflaterInputStream constructor throw?
Thanks for being alert. I think that all that can be thrown between getInfalter() call and streams.add() successfully returning is OutOfMemoryError or StackOverflowError. OOME can be thrown anywhere, not only as a consequence of Cleaner.register(). I see following potential places where allocation may be taking place: - constructing the lambda instance (line 389) - allocating ZipFileInflaterInputStream object - multiple places in ZipFileInflaterInputStream.<init> and InflaterInputStream.<init> (including but not limited to Cleaner.register()) - streams.add() (line 391) Can we do anything about it? Let's see. For example: Inflater inf = getInflater(); InputStream is; try { is = new ZipFileInflaterInputStream(in, inf, (int)size, () -> releaseInflater(inf)); } catch (OutOfMemoryError e) { releaseInflater(inf); throw e; } try { synchronized (streams) { streams.add(is); } } catch (OutOfMemoryError e) { try { is.close(); } catch (IOException ignore) {} } ...but, even releaseInflater(inf) may throw OOME (in line 470): 467 private void releaseInflater(Inflater inf) { 468 inf.reset(); 469 synchronized (inflaterCache) { 470 inflaterCache.add(inf); 471 } 472 } So we may change it to the following, which would make it more robust for other uses too: private void releaseInflater(Inflater inf) { inf.reset(); try { synchronized (inflaterCache) { inflaterCache.add(inf); } } catch (OutOfMemoryError e) { inf.end(); throw e; } } ...and that's it for OOME, hopefully. But what about StackOverflowError? If we want to go that far, we may need to create a special minimal critical method which encapsulates the logic of obtaining Inflater and creating ZipFileInflaterInputStream with it and then put a @ReservedStackAccess annotation on it. Like the following: @ReservedStackAccess private ZipFileInflaterInputStream createZipFileInflaterInputStream( ZipFileInputStream zfin, int size ) { Inflater inf = getInflater(); ZipFileInflaterInputStream is; try { is = new ZipFileInflaterInputStream(zfin, inf, size, () -> releaseInflater(inf)); } catch (OutOfMemoryError e) { releaseInflater(inf); throw e; } try { synchronized (streams) { streams.add(is); } } catch (OutOfMemoryError e) { try { is.close(); } catch (IOException ignore) { // not really possible - just making javac happy } throw e; } return is; } What do you think, Sherman? Regards, Peter
On 10/28/17, 10:47 AM, Peter Levart wrote:
Hi Florian,
On 10/28/17 16:16, Florian Weimer wrote:
* Xueming Shen:
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile:
387 Inflater inf = getInflater(); 388 InputStream is = new ZipFileInflaterInputStream(in, inf, (int)size, 389 () -> releaseInflater(inf)); 390 synchronized (streams) { 391 streams.add(is); 392 }
Doesn't this leak the inflater if Cleaner.register() and thus the ZipFileInflaterInputStream constructor throw?
Thanks for being alert. I think that all that can be thrown between getInfalter() call and streams.add() successfully returning is OutOfMemoryError or StackOverflowError. OOME can be thrown anywhere, not only as a consequence of Cleaner.register(). I see following potential places where allocation may be taking place:
- constructing the lambda instance (line 389) - allocating ZipFileInflaterInputStream object - multiple places in ZipFileInflaterInputStream.<init> and InflaterInputStream.<init> (including but not limited to Cleaner.register()) - streams.add() (line 391)
Can we do anything about it? Let's see. For example:
Inflater inf = getInflater(); InputStream is;
try { is = new ZipFileInflaterInputStream(in, inf, (int)size, () -> releaseInflater(inf)); } catch (OutOfMemoryError e) { releaseInflater(inf); throw e; }
try { synchronized (streams) { streams.add(is); } } catch (OutOfMemoryError e) { try { is.close(); } catch (IOException ignore) {} }
...but, even releaseInflater(inf) may throw OOME (in line 470):
467 private void releaseInflater(Inflater inf) { 468 inf.reset(); 469 synchronized (inflaterCache) { 470 inflaterCache.add(inf); 471 } 472 }
So we may change it to the following, which would make it more robust for other uses too:
private void releaseInflater(Inflater inf) { inf.reset(); try { synchronized (inflaterCache) { inflaterCache.add(inf); } } catch (OutOfMemoryError e) { inf.end(); throw e; } }
...and that's it for OOME, hopefully.
But what about StackOverflowError? If we want to go that far, we may need to create a special minimal critical method which encapsulates the logic of obtaining Inflater and creating ZipFileInflaterInputStream with it and then put a @ReservedStackAccess annotation on it. Like the following:
@ReservedStackAccess private ZipFileInflaterInputStream createZipFileInflaterInputStream( ZipFileInputStream zfin, int size ) { Inflater inf = getInflater(); ZipFileInflaterInputStream is; try { is = new ZipFileInflaterInputStream(zfin, inf, size, () -> releaseInflater(inf)); } catch (OutOfMemoryError e) { releaseInflater(inf); throw e; } try { synchronized (streams) { streams.add(is); } } catch (OutOfMemoryError e) { try { is.close(); } catch (IOException ignore) { // not really possible - just making javac happy } throw e; } return is; }
What do you think, Sherman?
There is reason why those are VirutalMachineError erros. It appears in this situation the vm is in a more critical status than a inflater is getting leaked out. I would assume you only catch these errors if/when the consequence of not catching is more severe and the "thing" you are going to do in catch is not going to trigger more errors. Sure we do that here and there in the libraries code for various reasons, but wonder if here is the place that is worth doing that. That said, if this is a real concern, instead of catching all the possible vm erros here and there to make Cleaner work correctly, a better alternative might be to go back to the previous version to leave the zipfile/deflater cleaner configured/registerd (give up the special constructor idea), so the deflater can get cleaned by itself in case all upstream clean up mechanism failed and the Cleaner mechanism somehow still functions. Opinion? -Sherman
Hi Sherman, On 10/28/2017 09:01 PM, Xueming Shen wrote:
On 10/28/17, 10:47 AM, Peter Levart wrote:
Hi Florian,
On 10/28/17 16:16, Florian Weimer wrote:
* Xueming Shen:
https://bugs.openjdk.java.net/browse/JDK-8187485 http://cr.openjdk.java.net/~sherman/8185582/webrev In ZipFile:
387 Inflater inf = getInflater(); 388 InputStream is = new ZipFileInflaterInputStream(in, inf, (int)size, 389 () -> releaseInflater(inf)); 390 synchronized (streams) { 391 streams.add(is); 392 }
Doesn't this leak the inflater if Cleaner.register() and thus the ZipFileInflaterInputStream constructor throw?
There is reason why those are VirutalMachineError erros. It appears in this situation the vm is in a more critical status than a inflater is getting leaked out. I would assume you only catch these errors if/when the consequence of not catching is more severe and the "thing" you are going to do in catch is not going to trigger more errors. Sure we do that here and there in the libraries code for various reasons, but wonder if here is the place that is worth doing that. That said, if this is a real concern, instead of catching all the possible vm erros here and there to make Cleaner work correctly, a better alternative might be to go back to the previous version to leave the zipfile/deflater cleaner configured/registerd (give up the special constructor idea), so the deflater can get cleaned by itself in case all upstream clean up mechanism failed and the Cleaner mechanism somehow still functions. Opinion?
-Sherman
I wouldn't go so far as to try to deal with potential StackOverflowError(s), but for OOMEs, I have the following opinion: - going back to previous version (give up the special constructor idea) might not, as you say, be any better. If you just look at the public Inflater constructor: 121 public Inflater(boolean nowrap) { 122 ZStreamRef ref = new ZStreamRef(init(nowrap), Inflater::end); 123 this.zsRef = ref; 124 this.cleanable = CleanerFactory.cleaner().register(this, ref); 125 } ...OOME can be thrown after init() and before successful Cleaner registration in multiple places: - method reference construction - ZStreamRef object allocation - Cleaner.register() and the probability of that occurring when there is heap exhaustion is not any smaller. So I'm more inclined to try to make such code more robust. There will always be places in such code where some native resource is 1st allocated, then some logic is executed which allocates objects and finally Cleaner.register() is attempted. By carefully programming those steps, a potential native resource leak in case of heap exhaustion can be prevented. There will usually be a way to back-out (i.e. free the native resource just allocated) without provoking further trouble. In the above example, it would be possible to do this: public Inflater(boolean nowrap) { long address = init(nowrap); try { ZStreamRef ref = new ZStreamRef(address, Inflater::end); this.zsRef = ref; this.cleanable = CleanerFactory.cleaner().register(this, ref); } catch (OutOfMemoryError oome) { end(address); throw oome; } } An alternative would be to perform all steps that allocate objects upfront and then, as the final action, allocate the native resource and "inject" it into the final place: public Inflater(boolean nowrap) { ZStreamRef ref = new ZStreamRef(Inflater::end); this.zsRef = ref; this.cleanable = CleanerFactory.cleaner().register(this, ref); ref.initAddress(init(nowrap)); } Concerning ZipFile, I would make releaseInflater() more robust, because it is easy: private void releaseInflater(Inflater inf) { inf.reset(); try { synchronized (inflaterCache) { inflaterCache.add(inf); } } catch (OutOfMemoryError e) { // Inflater.end() does no allocation inf.end(); throw e; } } For lines 387 ... 392, I would simply do this: Inflater inf = getInflater(); InputStream is; try { is = new ZipFileInflaterInputStream(in, inf, (int)size, () -> releaseInflater(inf)); } catch (OutOfMemoryError e) { // calling releaseInflater() is not sensible here, because it allocates and will // probably result in Inflater.end() anyway... inf.end(); throw e; } try { synchronized (streams) { streams.add(is); } } catch (OutOfMemoryError e) { // ZipFileInflaterInputStream.close() does not allocate, IOException is never thrown try { is.close(); } catch (IOException ignore) {} throw e; } Simply saying that "vm is in a more critical status than a inflater is getting leaked out" is, in my opinion, covering the problem under the rug. The VM is not in critical state - the program is. VM is robust enough to recover from OOMEs. The program might be in critical status (i.e. in inconsistent state) partly because of not accounting for such OOME situations. By taking care of them, the program has a better chance of recovering from such situation(s). Handling native resources is one place where I think it is beneficial to complicate things in order to make native resource leaks (im/less )possible. The other such place is synchronization primitives. We must admit that finalization does have this benefit that it makes it hard to construct code that would allocate the native resource before cleanup registration (which is performed as part of Object.<init>, while logic to allocate native resource is usually invoked from subclass constructor). To achieve the same robustness with Cleaner API, one has to be careful to either perform registration upfront and then allocate native resource or arrange for back-out in case of trouble. Regards, Peter
* Peter Levart:
Simply saying that "vm is in a more critical status than a inflater is getting leaked out" is, in my opinion, covering the problem under the rug. The VM is not in critical state - the program is. VM is robust enough to recover from OOMEs. The program might be in critical status (i.e. in inconsistent state) partly because of not accounting for such OOME situations. By taking care of them, the program has a better chance of recovering from such situation(s).
On the other hand, memory leaks on OOM are extremely common, and for the Inflater/ZStreamRef case, it might not be that critical to get this absolutely airtight (particularly if the fix has non-trivial concurrency implications).
Handling native resources is one place where I think it is beneficial to complicate things in order to make native resource leaks (im/less )possible. The other such place is synchronization primitives. We must admit that finalization does have this benefit that it makes it hard to construct code that would allocate the native resource before cleanup registration (which is performed as part of Object.<init>, while logic to allocate native resource is usually invoked from subclass constructor). To achieve the same robustness with Cleaner API, one has to be careful to either perform registration upfront and then allocate native resource or arrange for back-out in case of trouble.
If you allocate multiple resources, you probably need to apply the same level of care with finalizers. And the difficulty there lies in implementing a close() operating which releases resources and inhibits the effect of finalization.
Peter, Given we have to put in those "make it more robust" code in ZipFile to make cleaner work correctly with the zipfile/inflater in those vm error circumstances I would assume it is a wrong design decision to have the short-cut Inflater constructor to try to save some runtime circle for ZipFile/Inflater/cleaner. If the only purpose of those code is to deal with the rare vm error situation in which we can't call inflater.end() normally, then arguably this is the main reason we have the cleaner mechanism at first place, and we probably should just let the cleaner to do the job (clean the resource when the normal cleanup/release path does not work), instead of having yet another mechanism to replace it, and with more code to workaround the possible rare circumstances. Yes, if the vm error is a concern, the usage/implementation in Deflater/Inflater/ZStreamRef has the similar problem. Potentially the try/catch approach might have issues. Arguably the OOME might come from "register", and in theory there is no way to know whether or not the OOME is triggered before the "cleaner" has been successfully put in the Queue already or after If later, the cleaner might still be invoked later by the Cleaner to try to release the memory one more time. public Inflater(boolean nowrap) { long address = init(nowrap); try { ZStreamRef ref = new ZStreamRef(address, Inflater::end); this.zsRef = ref; this.cleanable = CleanerFactory.cleaner().register(this, ref); } catch (OutOfMemoryError oome) { end(address); throw oome; } } A normal return from register tells us everything is fine, the cleaner is registered and it will perform as expected, but an unexpected RuntimeException/Error from register really tells us nothing for now. The only "safe" approach seems to be the "alternative". As you suggested "..To achieve the same robustness with Cleaner API, one has to be careful to either perform registration upfront and then allocate native resource or arrange for back-out in case of trouble." It appears this might to be a very general usage issue of the "cleaner" mechanism. Now other than the "cleaning code should not have object reference the object being registered" restriction, it might be dirsired to have another suggestion/warning somewhere on the "order" of register the cleaner and the creation of the resource to be cleaned, and probably some guarantee that the "state" of the registered cleaner, still functional or thrown away, when the unexpected happens, such as a VM Error gets thrown during registration. Which reminds me the question asked early regarding other Cleaner use scenario. It appears, based on my experience of using Cleaner in this case, even the finalize() mechanism has "lots" of issues in its implementation, it provides a "nice/clean/simple" API to the "clean up the resource when not used" problem, while the Cleaner API appears to have lots of restriction to use it correctly. Webrev has been updated to (1) remove the special Inflater() (2) "allocate the resource and inject it later" for in/deflater. http://cr.openjdk.java.net/~sherman/8185582/webrev (the preview webrev has been rename to webrev.04) thanks, Sherman
... long t0 = System.nanoTime(); this.zsrc = Source.get(file, (mode & OPEN_DELETE) != 0); this.streams = Collections.newSetFromMap(new WeakHashMap<> this.inflaterCache = new ArrayDeque<>(); this.cleanable = CleanerFactory.cleaner().register(this, new Releaser(this.streams, this.inflaterCache, this.zsrc)); ... Now look at the "cleaner" for ZipFile. Obviously the new Releaser(...) might trigger OOME ... So we have to do something similar and move this piece to the top of the ZipFile constructor before any resource gets allocated, before we have the "zsrc"... yes, we have to inject the "zsrc" later. As, the zsrc will not get released if we allocate it before register. It appears we have a general usage issue here with the Cleaner? if the unexpected throwable needs to be taken care of. The resource needed to be released must be obtained after you have the cleaner created and successfully registered. Otherwise it might be leaked out if the "cleaner creation and registration" throws an unexpected throwable. As I suggested in my early email, try/catch might not simply work here as well, as an unexpected throwable from CleanerFactory.cleaner().register(...) really tells nothing about the status of the cleaner being registered, so the "cleaner" implementation will have to be crafted specially to take care of this situation. Sherman On 10/30/2017 11:45 AM, Xueming Shen wrote:
Peter,
Given we have to put in those "make it more robust" code in ZipFile to make cleaner work correctly with the zipfile/inflater in those vm error circumstances I would assume it is a wrong design decision to have the short-cut Inflater constructor to try to save some runtime circle for ZipFile/Inflater/cleaner. If the only purpose of those code is to deal with the rare vm error situation in which we can't call inflater.end() normally, then arguably this is the main reason we have the cleaner mechanism at first place, and we probably should just let the cleaner to do the job (clean the resource when the normal cleanup/release path does not work), instead of having yet another mechanism to replace it, and with more code to workaround the possible rare circumstances.
Yes, if the vm error is a concern, the usage/implementation in Deflater/Inflater/ZStreamRef has the similar problem. Potentially the try/catch approach might have issues. Arguably the OOME might come from "register", and in theory there is no way to know whether or not the OOME is triggered before the "cleaner" has been successfully put in the Queue already or after If later, the cleaner might still be invoked later by the Cleaner to try to release the memory one more time.
public Inflater(boolean nowrap) { long address = init(nowrap); try { ZStreamRef ref = new ZStreamRef(address, Inflater::end); this.zsRef = ref; this.cleanable = CleanerFactory.cleaner().register(this, ref); } catch (OutOfMemoryError oome) { end(address); throw oome; } }
A normal return from register tells us everything is fine, the cleaner is registered and it will perform as expected, but an unexpected RuntimeException/Error from register really tells us nothing for now. The only "safe" approach seems to be the "alternative".
As you suggested "..To achieve the same robustness with Cleaner API, one has to be careful to either perform registration upfront and then allocate native resource or arrange for back-out in case of trouble." It appears this might to be a very general usage issue of the "cleaner" mechanism. Now other than the "cleaning code should not have object reference the object being registered" restriction, it might be dirsired to have another suggestion/warning somewhere on the "order" of register the cleaner and the creation of the resource to be cleaned, and probably some guarantee that the "state" of the registered cleaner, still functional or thrown away, when the unexpected happens, such as a VM Error gets thrown during registration. Which reminds me the question asked early regarding other Cleaner use scenario. It appears, based on my experience of using Cleaner in this case, even the finalize() mechanism has "lots" of issues in its implementation, it provides a "nice/clean/simple" API to the "clean up the resource when not used" problem, while the Cleaner API appears to have lots of restriction to use it correctly.
Webrev has been updated to (1) remove the special Inflater() (2) "allocate the resource and inject it later" for in/deflater.
http://cr.openjdk.java.net/~sherman/8185582/webrev (the preview webrev has been rename to webrev.04)
thanks, Sherman
Hi Sherman, On 10/30/17 19:45, Xueming Shen wrote:
Peter,
Given we have to put in those "make it more robust" code in ZipFile to make cleaner work correctly with the zipfile/inflater in those vm error circumstances I would assume it is a wrong design decision to have the short-cut Inflater constructor to try to save some runtime circle for ZipFile/Inflater/cleaner. If the only purpose of those code is to deal with the rare vm error situation in which we can't call inflater.end() normally, then arguably this is the main reason we have the cleaner mechanism at first place, and we probably should just let the cleaner to do the job (clean the resource when the normal cleanup/release path does not work), instead of having yet another mechanism to replace it, and with more code to workaround the possible rare circumstances.
Ok, but in that case the Cleaner registration in Inflater should be reliable and not fail in the same circumstances.
Yes, if the vm error is a concern, the usage/implementation in Deflater/Inflater/ZStreamRef has the similar problem. Potentially the try/catch approach might have issues. Arguably the OOME might come from "register", and in theory there is no way to know whether or not the OOME is triggered before the "cleaner" has been successfully put in the Queue already or after If later, the cleaner might still be invoked later by the Cleaner to try to release the memory one more time.
The Cleaner.register() can only fail with OOME and only because the jdk.internal.ref.CleanerImpl.PhantomCleanableRef object allocation fails. This *is the only* allocation performed by Cleaner.register(). If PhantomCleanableRef object allocation fails (a PhantomReference subclass), no PhantomReference instance is created and therefore can not be discovered by GC and consequently no cleanup happens.
public Inflater(boolean nowrap) { long address = init(nowrap); try { ZStreamRef ref = new ZStreamRef(address, Inflater::end); this.zsRef = ref; this.cleanable = CleanerFactory.cleaner().register(this, ref); } catch (OutOfMemoryError oome) { end(address); throw oome; } }
A normal return from register tells us everything is fine, the cleaner is registered and it will perform as expected, but an unexpected RuntimeException/Error from register really tells us nothing for now.
The only RuntimeException possible from Cleaner.register() is a NullPointerException because of null operands and the only possible Error is OOME which in both cases tells us that registration did not happen. As said, Cleaner.register() allocates a single instance - an instance of jdk.internal.ref.CleanerImpl.PhantomCleanableRef. Everything else is just "dancing with links".
The only "safe" approach seems to be the "alternative".
I agree that it is a more direct and simple approach to just reorder the operations, rather than arrange for back-out, but I think it is equally safe. Might be good to put some comments just before the init() to explain why it is done last in Inflater constructor (same for Deflater)?
As you suggested "..To achieve the same robustness with Cleaner API, one has to be careful to either perform registration upfront and then allocate native resource or arrange for back-out in case of trouble." It appears this might to be a very general usage issue of the "cleaner" mechanism.
Yes. It appears so. But OTOH it is something that is easily understood and might be beneficial to document in the Cleaner javadoc.
Now other than the "cleaning code should not have object reference the object being registered" restriction, it might be dirsired to have another suggestion/warning somewhere on the "order" of register the cleaner and the creation of the resource to be cleaned,
Right. To mimic the finalization registration, it might be a good to encourage the following coding pattern (using Inflater/ZStreamRef just as an example, not suggesting to do that here unless you like it): class ZStreamRef implements Runnable { private final LongConsumer end; private volatile long address; final Cleaner.Cleanable cleanable; // move cleanable from Inflater/Deflater to here ZStreamRef (Object reference, LongSupplier init, LongConsumer end) { // perform registration as 1st thing in constructor cleanable = CleanerFactory.cleaner().register(reference, this); this.end = end; this.address = init.getAsLong(); } long address() { return address; } public synchronized void run() { long addr = address; address = 0; if (addr != 0) { end.accept(addr); } } } // ... and then in Inflater / Deflater: public Inflater(boolean nowrap) { this.zsRef = new ZStreamRef(this, () -> init(nowrap), Inflater::end); } public Deflater(int level, boolean nowrap) { this.level = level; this.strategy = DEFAULT_STRATEGY; this.zsRef = new ZStreamRef( this, () -> init(level, DEFAULT_STRATEGY, nowrap), Deflater::end); } public void end() { synchronized (zsRef) { zsRef.cleanable.clean(); buf = null; } }
and probably some guarantee that the "state" of the registered cleaner, still functional or thrown away, when the unexpected happens, such as a VM Error gets thrown during registration.
I'm pretty sure it is guaranteed that Cleaner.register() throwing OOME means that no registration happened.
Which reminds me the question asked early regarding other Cleaner use scenario. It appears, based on my experience of using Cleaner in this case, even the finalize() mechanism has "lots" of issues in its implementation, it provides a "nice/clean/simple" API to the "clean up the resource when not used" problem, while the Cleaner API appears to have lots of restriction to use it correctly.
It is not trivial to use, but it has benefits that finalization lacks. Like on-demand cleanup with de-registration.
Webrev has been updated to (1) remove the special Inflater() (2) "allocate the resource and inject it later" for in/deflater.
http://cr.openjdk.java.net/~sherman/8185582/webrev (the preview webrev has been rename to webrev.04)
The Inflater and Deflater now look fine (except you don't have to check for cleanable != null any more in Inflater.end()). But what shall we do with ZipFile?
thanks, Sherman
Regards, Peter
Hi again, On 10/30/17 21:34, Peter Levart wrote:
To mimic the finalization registration, it might be a good to encourage the following coding pattern (using Inflater/ZStreamRef just as an example, not suggesting to do that here unless you like it):
class ZStreamRef implements Runnable {
private final LongConsumer end; private volatile long address; final Cleaner.Cleanable cleanable; // move cleanable from Inflater/Deflater to here
ZStreamRef (Object reference, LongSupplier init, LongConsumer end) { // perform registration as 1st thing in constructor cleanable = CleanerFactory.cleaner().register(reference, this); this.end = end; this.address = init.getAsLong(); }
long address() { return address; }
public synchronized void run() { long addr = address; address = 0; if (addr != 0) { end.accept(addr); } } }
...above example lends itself as a use case for the following equivalent alternative using internal low-level API where ZStreamRef becomes the Cleanable itself: class ZStreamRef extends PhantomCleanable<Object> { private final LongConsumer end; private volatile long address; ZStreamRef (Object referent, LongSupplier init, LongConsumer end) { // here the registration MUST happen as 1st thing - enforced by javac super(referent, CleanerFactory.cleaner()); this.end = end; this.address = init.getAsLong(); } long address() { return address; } @Override protected void performCleanup() { long addr = address; address = 0; if (addr != 0) { end.accept(addr); } } } Inflater/Deflater constructor is unchanged while end() becomes: public void end() { synchronized (zsRef) { zsRef.clean(); // zsRef is-a Cleanable buf = null; } } It's interesting that something that is considered a "low-level" API in above example forces you to do it right, while the high level API doesn't. Regards, Peter
On 10/30/17 1:49 PM, Peter Levart wrote:
...above example lends itself as a use case for the following equivalent alternative using internal low-level API where ZStreamRef becomes the Cleanable itself:
class ZStreamRef extends PhantomCleanable<Object> {
private final LongConsumer end; private volatile long address;
ZStreamRef (Object referent, LongSupplier init, LongConsumer end) { // here the registration MUST happen as 1st thing - enforced by javac super(referent, CleanerFactory.cleaner()); this.end = end; this.address = init.getAsLong(); }
long address() { return address; }
@Override protected void performCleanup() { long addr = address; address = 0; if (addr != 0) { end.accept(addr); } } }
I was thinking something along this line what could ensure the "cleanable" object is allocated before allocating the resources that the cleanable is responsible for clean up. I think it'd be a good RFE to improve the Cleaner API to address the OOM case. Mandy
Hi Mandy, Sherman, Roger, On 10/31/17 00:25, mandy chung wrote:
On 10/30/17 1:49 PM, Peter Levart wrote:
...above example lends itself as a use case for the following equivalent alternative using internal low-level API where ZStreamRef becomes the Cleanable itself:
class ZStreamRef extends PhantomCleanable<Object> {
private final LongConsumer end; private volatile long address;
ZStreamRef (Object referent, LongSupplier init, LongConsumer end) { // here the registration MUST happen as 1st thing - enforced by javac super(referent, CleanerFactory.cleaner()); this.end = end; this.address = init.getAsLong(); }
long address() { return address; }
@Override protected void performCleanup() { long addr = address; address = 0; if (addr != 0) { end.accept(addr); } } }
I was thinking something along this line what could ensure the "cleanable" object is allocated before allocating the resources that the cleanable is responsible for clean up. I think it'd be a good RFE to improve the Cleaner API to address the OOM case.
Mandy
I played a little with an idea of how such additional Cleaner API might look like. Here's what I came up with, together with how this would apply to ZipFile/Inflater/Deflater: http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.02/ @Sherman: While looking at how to apply this to ZipFile Cleaner(s), I noticed that Releaser, as proposed, does not have to include opened streams or cached inflaters. Why? - opened streams keep a reference to the ZipFile, so ZipFile's Cleaner will not fire automatically until all opened streams and ZipFile become phantom-reachable. Even more than that, ZipFileInflaterInputStream's Cleaner keeps a reference to ZipFile in order to call ZipFile.releaseInflater. So all ZipFileInflaterInputStream(s) have to be closed 1st (either cleaned automatically or explicitly) so that their inflaters are released back to cache and ZipFileInflaterInputStream's cleanup functions are released. Only then may ZipFile become phantom-reachable. When Releaser is finally triggered automatically, there are no more open ZipFileInflaterInputStream(s) only ZipFileInputStream(s) may be still open, but already phantom-reachable. PhantomReference specification says: * <p> Suppose the garbage collector determines at a certain point in time * that an object is <a href="package-summary.html#reachability"> * phantom reachable</a>. At that time it will atomically clear * all phantom references to that object and all phantom references to * any other phantom-reachable objects from which that object is reachable. * At the same time or at some later time it will enqueue those newly-cleared * phantom references that are registered with reference queues. ...which means that when ZipFile's Cleaner finally fires automatically, all opened stream's Cleaners have already fired or are going to fire together in same "batch" with the ZipFile's Cleaner. No need for ZipFile to include opened streams in its automatic cleanup. It only has to do that in explicit close(). The same reasoning goes for cached inflaters. They are only reachable from ZipFile. So when ZipFile becomes phantom-reachable, so do cached Inflaters at the same time and are therefore cleaned in the same "batch". The only resource that ZipFile needs to clean in its automatic cleanup function is the shared, globally cached, reference counted Source (zsrc) object. And this is what I did with this new proposed Cleaner API in above webrev.02. So, what do you think of this new Cleaner API extension? Regards, Peter
On 10/31/17 4:07 PM, Peter Levart wrote:
Hi Mandy, Sherman, Roger,
On 10/31/17 00:25, mandy chung wrote:
I played a little with an idea of how such additional Cleaner API might look like. Here's what I came up with, together with how this would apply to ZipFile/Inflater/Deflater:
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.02/
So, what do you think of this new Cleaner API extension?
This serves as a good starting point. I converted the ClassLoader.NativeLibrary to use it: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev-cleanable/ Looks reasonably well and probably some other improvement I could do. I use createResource rather than createLongResource which is specialized for an address. We should check out panama how this plays out. I think it would be useful to iterate on several more use cases and it would be better to separate this RFE from JDK-8185582 so that we can explore with its own pace. Mandy
Hi Peter, The Supplier/Consumer allocator/deallocator version is useful and interesting encapsulation. We should be able to do better than exposing raw native pointers for other resources. They should have better more complete encapsulation including their cleanup. For example, the recent work on FileDescriptor. The current state with direct memory pointers floating freely is quite sensitive. I'm hopeful that Panama will support that direction. In CleanerImp:322, is there a JMM issue with the escape of the reference to PhantomCleanableResource to the cleaner thread before the constructor has finished and published the instance? True, the constructor holds this until the very end of the method but what makes sure the new valuewritten to the resource field will be read by the cleanable thread? Thanks, Roger On 10/31/2017 11:17 PM, mandy chung wrote:
On 10/31/17 4:07 PM, Peter Levart wrote:
Hi Mandy, Sherman, Roger,
On 10/31/17 00:25, mandy chung wrote:
I played a little with an idea of how such additional Cleaner API might look like. Here's what I came up with, together with how this would apply to ZipFile/Inflater/Deflater:
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.02/
So, what do you think of this new Cleaner API extension?
This serves as a good starting point. I converted the ClassLoader.NativeLibrary to use it: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev-cleanable/
Looks reasonably well and probably some other improvement I could do. I use createResource rather than createLongResource which is specialized for an address. We should check out panama how this plays out.
I think it would be useful to iterate on several more use cases and it would be better to separate this RFE from JDK-8185582 so that we can explore with its own pace.
Mandy
On 10/30/2017 01:34 PM, Peter Levart wrote:
The Inflater and Deflater now look fine (except you don't have to check for cleanable != null any more in Inflater.end()).
But what shall we do with ZipFile?
Peter, The fundamental issue here is the gap between the resource allocation and the cleaner registration, as far as these two are not an atomic operation, it's hard to handle it in the normal use scenario. You might be able to workaround it by registering the cleaner first and then send the resource reference into the cleaner, as we are going to do for the deflater/ inflater. But it appears to be a more general issue and in some circumstance even the "register-first-allocate-assign-in-later" approach might fail. For example, you have N resource to allocate, something goes wrong between the first and the Nth resource allocation completed/or simply say before you can reach the "register()". The question then I would like to ask again is if this is really something we want to support/cover with the Cleaner mechanism. It doesn't appear to be supported under the finalize() mechanism. For example, if someone throws out an exception inside the constructor, in normal use scenario, regardless you catch or not catch that exception (outside the constructor), the finalize() method is probably not going to be called by the finalizer for this partially/incompletely constructed object, if you don't purposely do something special to publish the object. Sure arguably It appears to be out of the scope of Cleaner API and the only thing needs to be addressed here is whether or not register(..) operation fails and throws an Error. But it might be helpful to see if there is any different opinion before going further? Sherman
Hi Sherman, On 10/31/17 00:32, Xueming Shen wrote:
On 10/30/2017 01:34 PM, Peter Levart wrote:
The Inflater and Deflater now look fine (except you don't have to check for cleanable != null any more in Inflater.end()).
But what shall we do with ZipFile?
Peter,
The fundamental issue here is the gap between the resource allocation and the cleaner registration, as far as these two are not an atomic operation, it's hard to handle it in the normal use scenario. You might be able to workaround it by registering the cleaner first and then send the resource reference into the cleaner, as we are going to do for the deflater/ inflater.
RIght. Finalization, as designed, usually guides the programmer to not mess this order. The finalizable instance is registered as part of Object.<init> constructor call. When resource allocation happens in the subclass constructor(s), all is well, because code of subclass constructors is executed after Object.<init> successfully returns. Subclassing PhantomCleanable<T> has this same good property when resource allocation is performed in the PhantomCleanable's subclass constructor(s).
But it appears to be a more general issue and in some circumstance even the "register-first-allocate-assign-in-later" approach might fail. For example, you have N resource to allocate, something goes wrong between the first and the Nth resource allocation completed/or simply say before you can reach the "register()".
If you really do "register-first-allocate-assign-in-later" then there is no problem as those resources that are assigned are the resources that were allocated. The cleanup function should selectively deallocate the resources that were assigned (using null or some special default value to identify resources that weren't).
The question then I would like to ask again is if this is really something we want to support/cover with the Cleaner mechanism. It doesn't appear to be supported under the finalize() mechanism. For example, if someone throws out an exception inside the constructor, in normal use scenario, regardless you catch or not catch that exception (outside the constructor), the finalize() method is probably not going to be called by the finalizer for this partially/incompletely constructed object, if you don't purposely do something special to publish the object.
Wrong. Try this example: public class TestFinalization { final int resource; public TestFinalization() { if (true) { // by the time this is thrown, the object is already registered // for finalization.... throw new RuntimeException(); } resource = 42; } @SuppressWarnings("deprecation") @Override protected void finalize() { System.out.println("finalize() called: resource=" + resource); } public static void main(String[] args) throws Exception { try { new TestFinalization(); } catch (RuntimeException e) { System.out.println("Exception caught"); } System.gc(); Thread.sleep(1000L); } } It prints the following: Exception caught finalize() called: resource=0
Sure arguably It appears to be out of the scope of Cleaner API and the only thing needs to be addressed here is whether or not register(..) operation fails and throws an Error. But it might be helpful to see if there is any different opinion before going further?
I don't see much difference between finalization and Cleaner mechanism. The main difference is that with finalization, the object that is being tracked is also the object that contains the state and logic needed for cleanup. Cleaner API decouples those two aspects. The low-level (extending PhantomCleanable<T>) API also encourages the correct order of things: 1st register then allocate. If we either expose the low-level API to the public or make some additions to the high-level API to encourage the same, all will be well. Regards, Peter
Sherman
Hi Peter, After tried couple implementations it seems the most reasonable approach is to use the coding pattern you suggested to move all pieces into ZSStream Ref. Given we are already using the internal API CleanerFactory it is attractive to go a little further to subclass PhantomCleanable directly (in which I would assume we can save one extra object), but I think I'd better to follow the "suggested" clean usage (to register a runnable into the cleaner) for now. 39 class ZStreamRef implements Runnable { 40 41 private LongConsumer end; 42 private volatile long address; 43 private final Cleanable cleanable; 44 45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer end) { 46 this.cleanable = CleanerFactory.cleaner().register(owner, this); 47 this.end = end; 48 this.address = addr.getAsLong(); 49 } 50 Similar change has been made for the ZipFile cleaner to follow the same coding pattern. The "cleaner" is now renamed from Releaser to CleanableResource. http://cr.openjdk.java.net/~sherman/8185582/webrev/ Thanks, Sherman
Hi Sherman, On 11/01/17 00:25, Xueming Shen wrote:
Hi Peter,
After tried couple implementations it seems the most reasonable approach is to use the coding pattern you suggested to move all pieces into ZSStream Ref. Given we are already using the internal API CleanerFactory it is attractive to go a little further to subclass PhantomCleanable directly (in which I would assume we can save one extra object), but I think I'd better to follow the "suggested" clean usage (to register a runnable into the cleaner) for now.
39 class ZStreamRef implements Runnable { 40 41 private LongConsumer end; 42 private volatile long address; 43 private final Cleanable cleanable; 44 45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer end) { 46 this.cleanable = CleanerFactory.cleaner().register(owner, this); 47 this.end = end; 48 this.address = addr.getAsLong(); 49 } 50
Similar change has been made for the ZipFile cleaner to follow the same coding pattern. The "cleaner" is now renamed from Releaser to CleanableResource.
http://cr.openjdk.java.net/~sherman/8185582/webrev/
Thanks, Sherman
This looks mostly fine. One minor nit is that ZStreamRef.address field does not have to be volatile. I checked all usages of ZStreamRef.address() and all of them invoke it while holding a lock on the ZStreamRef instance. The ZStreamRef.run() which modifies the address is also synchronized. The other minor nit is that the following ZipFile imports are not needed: import java.nio.file.Path; import java.util.Map; import jdk.internal.misc.JavaIORandomAccessFileAccess; import static java.util.zip.ZipConstants.*; (at least my IDEA colors them unused) Cleaner is modified in this webrev (just one empty line deleted) - better not touch this file in this changeset Additional nits in ZipFile: - in lines 355, 356 you pull out two res fields into locals, but then not use them consistently (lines 372, 389) - line 403 has a TAB character (is this OK?) and shows incorrectly indented in my editor (should I set tab stops differently?) - line 457 - while should actually be if ? - in ensureOpen() the check for res.zsrc == null can never succeed (CleanableResource.zsrc is a final field. If CleanableResource constructor fails, there's no res object and there's no ZipFile object either as ZipFile constructor does not do anything for this to escape prematurely) - why don't you let IOExceptions thrown from CleanableResource.run() propagate out of ZipFile.close() ? I would also rename static method Source.close(Source) to Source.release(Source) so it would not clash with instance method Source.close() which makes it ambiguous when one wants to use Source::close method reference (both methods apply). I would also make static methods Source.get() and Source.release(Source) package-private (currently one is public and the other is private, which needs compiler bridges to be invoked) and both are in a private nested class. Inflater/Deflater/ZipFile now follow the coding pattern as suggested. But ZipFileInflaterInputStream still does not. It's not critical since failing to register cleanup which releases the inflater back into cache would simply mean that Inflater employs its own cleanup and ends itself. And now another thing I would like to discuss. Why an initiative for using Cleaner instead of finalization()? Among drawbacks finalization has one of the more troubling is that the tracked referent survives the GC cycle that initiates its finalization reference processing pipeline, so the GC may reclaim the object (and referenced objects) only after the finalize() has finished in yet another GC round. Cleaner API separates the tracked object from the cleanup function and state needed to perform it into distinct instances. The tracked object can be reclaimed and the cleanup reference processing pipeline initiated in the same GC cycle. More heap may be reclaimed earlier. Unless we are careful and create a cleaning function for one tracked object which captures (directly or indirectly) another object which registers its own cleaning function but we don't deal with explicit cleaning of the 2nd object in the 1st cleaning function. Take for example the ZipFileInflaterInputStream's cleaning function. It captures the ZipFile in order to invoke ZipFile.releaseInflater instance method. What this means is that ZipFile will be kept reachable until all ZipFileInflaterInputStream's cleaning functions have fired. So we are back to the finalization drawback which needs at least 2 GC cycles to collect and clean-up what might be done in one go. I suggest moving the getInflater() and releaseInflater() from ZipFile into the CleanableResource so that ZipFileInflaterInputStream's cleaning function captures just the CleanableResource and not the ZipFile. ZipFile therefore may become eligible for cleanup as soon as all opened input streams become eligible (but their cleaning functions need not have fired yet). CleanableResource.run() (the ZipFile cleaning function) and CleanableResource.releaseInflater() (the ZipFileInflaterInputStream's cleaning function) may therefore be invoked in arbitrary order (maybe even concurrently if one of them is explicit cleanup and the other is automatic), so code must be prepared for that. I have tried to capture all above in a modified webrev (taking your last webrev as a base): http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.03/ Regards, Peter
Hi Peter, I like the idea of moving get/releaseInflter() into CleanableResource, though I doubt how much it can really help the GC it should be a good thing to do to remove the strong reference of ZipFile from stream's cleaner, and the code appears a little cleaner as well. I was debating with myself whether or not the ZipFile.close() should throw an UncheckedIOException (like those java.nio.file.Files methods do). But you're right it's not good to simply ignoring them silently. Now I catch/unwarp the potential UncheckedIOException from res.clean() and re-throw the embedded ioe. I need to dig a little to recall the real reason of zsrc==null check in ensureOpen() the comment does not sound updated. res.zsrc is not final and it is set to null when clean up/close. So I keep it for now. Most (if not all?) "minor nit" has been updated accordingly. It seems like we might have have put the "finalize()" method back as an empty-body method for compatibility reason. So not done yet :-) http://cr.openjdk.java.net/~sherman/8185582/webrev/ thanks, sherman On 11/1/17, 5:04 AM, Peter Levart wrote:
Hi Sherman,
On 11/01/17 00:25, Xueming Shen wrote:
Hi Peter,
After tried couple implementations it seems the most reasonable approach is to use the coding pattern you suggested to move all pieces into ZSStream Ref. Given we are already using the internal API CleanerFactory it is attractive to go a little further to subclass PhantomCleanable directly (in which I would assume we can save one extra object), but I think I'd better to follow the "suggested" clean usage (to register a runnable into the cleaner) for now.
39 class ZStreamRef implements Runnable { 40 41 private LongConsumer end; 42 private volatile long address; 43 private final Cleanable cleanable; 44 45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer end) { 46 this.cleanable = CleanerFactory.cleaner().register(owner, this); 47 this.end = end; 48 this.address = addr.getAsLong(); 49 } 50
Similar change has been made for the ZipFile cleaner to follow the same coding pattern. The "cleaner" is now renamed from Releaser to CleanableResource.
http://cr.openjdk.java.net/~sherman/8185582/webrev/
Thanks, Sherman
This looks mostly fine. One minor nit is that ZStreamRef.address field does not have to be volatile. I checked all usages of ZStreamRef.address() and all of them invoke it while holding a lock on the ZStreamRef instance. The ZStreamRef.run() which modifies the address is also synchronized. The other minor nit is that the following ZipFile imports are not needed:
import java.nio.file.Path; import java.util.Map; import jdk.internal.misc.JavaIORandomAccessFileAccess; import static java.util.zip.ZipConstants.*;
(at least my IDEA colors them unused)
Cleaner is modified in this webrev (just one empty line deleted) - better not touch this file in this changeset
Additional nits in ZipFile:
- in lines 355, 356 you pull out two res fields into locals, but then not use them consistently (lines 372, 389) - line 403 has a TAB character (is this OK?) and shows incorrectly indented in my editor (should I set tab stops differently?) - line 457 - while should actually be if ? - in ensureOpen() the check for res.zsrc == null can never succeed (CleanableResource.zsrc is a final field. If CleanableResource constructor fails, there's no res object and there's no ZipFile object either as ZipFile constructor does not do anything for this to escape prematurely) - why don't you let IOExceptions thrown from CleanableResource.run() propagate out of ZipFile.close() ?
I would also rename static method Source.close(Source) to Source.release(Source) so it would not clash with instance method Source.close() which makes it ambiguous when one wants to use Source::close method reference (both methods apply). I would also make static methods Source.get() and Source.release(Source) package-private (currently one is public and the other is private, which needs compiler bridges to be invoked) and both are in a private nested class.
Inflater/Deflater/ZipFile now follow the coding pattern as suggested. But ZipFileInflaterInputStream still does not. It's not critical since failing to register cleanup which releases the inflater back into cache would simply mean that Inflater employs its own cleanup and ends itself.
And now another thing I would like to discuss. Why an initiative for using Cleaner instead of finalization()? Among drawbacks finalization has one of the more troubling is that the tracked referent survives the GC cycle that initiates its finalization reference processing pipeline, so the GC may reclaim the object (and referenced objects) only after the finalize() has finished in yet another GC round. Cleaner API separates the tracked object from the cleanup function and state needed to perform it into distinct instances. The tracked object can be reclaimed and the cleanup reference processing pipeline initiated in the same GC cycle. More heap may be reclaimed earlier.
Unless we are careful and create a cleaning function for one tracked object which captures (directly or indirectly) another object which registers its own cleaning function but we don't deal with explicit cleaning of the 2nd object in the 1st cleaning function.
Take for example the ZipFileInflaterInputStream's cleaning function. It captures the ZipFile in order to invoke ZipFile.releaseInflater instance method. What this means is that ZipFile will be kept reachable until all ZipFileInflaterInputStream's cleaning functions have fired. So we are back to the finalization drawback which needs at least 2 GC cycles to collect and clean-up what might be done in one go.
I suggest moving the getInflater() and releaseInflater() from ZipFile into the CleanableResource so that ZipFileInflaterInputStream's cleaning function captures just the CleanableResource and not the ZipFile. ZipFile therefore may become eligible for cleanup as soon as all opened input streams become eligible (but their cleaning functions need not have fired yet). CleanableResource.run() (the ZipFile cleaning function) and CleanableResource.releaseInflater() (the ZipFileInflaterInputStream's cleaning function) may therefore be invoked in arbitrary order (maybe even concurrently if one of them is explicit cleanup and the other is automatic), so code must be prepared for that.
I have tried to capture all above in a modified webrev (taking your last webrev as a base):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.03/
Regards, Peter
Hi Sherman, I think this looks good now. Thanks. Regards, Peter On 11/01/2017 08:17 PM, Xueming Shen wrote:
Hi Peter,
I like the idea of moving get/releaseInflter() into CleanableResource, though I doubt how much it can really help the GC it should be a good thing to do to remove the strong reference of ZipFile from stream's cleaner, and the code appears a little cleaner as well.
I was debating with myself whether or not the ZipFile.close() should throw an UncheckedIOException (like those java.nio.file.Files methods do). But you're right it's not good to simply ignoring them silently. Now I catch/unwarp the potential UncheckedIOException from res.clean() and re-throw the embedded ioe.
I need to dig a little to recall the real reason of zsrc==null check in ensureOpen() the comment does not sound updated. res.zsrc is not final and it is set to null when clean up/close. So I keep it for now.
Most (if not all?) "minor nit" has been updated accordingly.
It seems like we might have have put the "finalize()" method back as an empty-body method for compatibility reason. So not done yet :-)
http://cr.openjdk.java.net/~sherman/8185582/webrev/
thanks, sherman
On 11/1/17, 5:04 AM, Peter Levart wrote:
Hi Sherman,
On 11/01/17 00:25, Xueming Shen wrote:
Hi Peter,
After tried couple implementations it seems the most reasonable approach is to use the coding pattern you suggested to move all pieces into ZSStream Ref. Given we are already using the internal API CleanerFactory it is attractive to go a little further to subclass PhantomCleanable directly (in which I would assume we can save one extra object), but I think I'd better to follow the "suggested" clean usage (to register a runnable into the cleaner) for now.
39 class ZStreamRef implements Runnable { 40 41 private LongConsumer end; 42 private volatile long address; 43 private final Cleanable cleanable; 44 45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer end) { 46 this.cleanable = CleanerFactory.cleaner().register(owner, this); 47 this.end = end; 48 this.address = addr.getAsLong(); 49 } 50
Similar change has been made for the ZipFile cleaner to follow the same coding pattern. The "cleaner" is now renamed from Releaser to CleanableResource.
http://cr.openjdk.java.net/~sherman/8185582/webrev/
Thanks, Sherman
This looks mostly fine. One minor nit is that ZStreamRef.address field does not have to be volatile. I checked all usages of ZStreamRef.address() and all of them invoke it while holding a lock on the ZStreamRef instance. The ZStreamRef.run() which modifies the address is also synchronized. The other minor nit is that the following ZipFile imports are not needed:
import java.nio.file.Path; import java.util.Map; import jdk.internal.misc.JavaIORandomAccessFileAccess; import static java.util.zip.ZipConstants.*;
(at least my IDEA colors them unused)
Cleaner is modified in this webrev (just one empty line deleted) - better not touch this file in this changeset
Additional nits in ZipFile:
- in lines 355, 356 you pull out two res fields into locals, but then not use them consistently (lines 372, 389) - line 403 has a TAB character (is this OK?) and shows incorrectly indented in my editor (should I set tab stops differently?) - line 457 - while should actually be if ? - in ensureOpen() the check for res.zsrc == null can never succeed (CleanableResource.zsrc is a final field. If CleanableResource constructor fails, there's no res object and there's no ZipFile object either as ZipFile constructor does not do anything for this to escape prematurely) - why don't you let IOExceptions thrown from CleanableResource.run() propagate out of ZipFile.close() ?
I would also rename static method Source.close(Source) to Source.release(Source) so it would not clash with instance method Source.close() which makes it ambiguous when one wants to use Source::close method reference (both methods apply). I would also make static methods Source.get() and Source.release(Source) package-private (currently one is public and the other is private, which needs compiler bridges to be invoked) and both are in a private nested class.
Inflater/Deflater/ZipFile now follow the coding pattern as suggested. But ZipFileInflaterInputStream still does not. It's not critical since failing to register cleanup which releases the inflater back into cache would simply mean that Inflater employs its own cleanup and ends itself.
And now another thing I would like to discuss. Why an initiative for using Cleaner instead of finalization()? Among drawbacks finalization has one of the more troubling is that the tracked referent survives the GC cycle that initiates its finalization reference processing pipeline, so the GC may reclaim the object (and referenced objects) only after the finalize() has finished in yet another GC round. Cleaner API separates the tracked object from the cleanup function and state needed to perform it into distinct instances. The tracked object can be reclaimed and the cleanup reference processing pipeline initiated in the same GC cycle. More heap may be reclaimed earlier.
Unless we are careful and create a cleaning function for one tracked object which captures (directly or indirectly) another object which registers its own cleaning function but we don't deal with explicit cleaning of the 2nd object in the 1st cleaning function.
Take for example the ZipFileInflaterInputStream's cleaning function. It captures the ZipFile in order to invoke ZipFile.releaseInflater instance method. What this means is that ZipFile will be kept reachable until all ZipFileInflaterInputStream's cleaning functions have fired. So we are back to the finalization drawback which needs at least 2 GC cycles to collect and clean-up what might be done in one go.
I suggest moving the getInflater() and releaseInflater() from ZipFile into the CleanableResource so that ZipFileInflaterInputStream's cleaning function captures just the CleanableResource and not the ZipFile. ZipFile therefore may become eligible for cleanup as soon as all opened input streams become eligible (but their cleaning functions need not have fired yet). CleanableResource.run() (the ZipFile cleaning function) and CleanableResource.releaseInflater() (the ZipFileInflaterInputStream's cleaning function) may therefore be invoked in arbitrary order (maybe even concurrently if one of them is explicit cleanup and the other is automatic), so code must be prepared for that.
I have tried to capture all above in a modified webrev (taking your last webrev as a base):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.03/
Regards, Peter
On 11/03/17 09:48, Peter Levart wrote:
Hi Sherman,
I think this looks good now. Thanks.
Regards, Peter
Just one more thing. Re-reading the code once more after a few days made me re-think about why in my last version I did what I did with CleanableResource.inflaters field. In CleanableResource.run() I reset it to null after 1st ending all cached inflaters. This makes ZipFileInflaterInputStream(s) that are cleaned after the ZipFile has been cleaned, release the inflaters immediately and not in next GC round: 623 void releaseInflater(Inflater inf) { 624 Deque<Inflater> inflaters = this.inflaters; 625 if (inflaters != null) { 626 synchronized (inflaters) { 627 // double checked! 628 if (this.inflaters == inflaters) { 629 inf.reset(); 630 inflaters.add(inf); 631 return; 632 } 633 } 634 } 635 // inflaters cache already closed - just end late comers 636 inf.end(); 637 } 639 public void run() { 640 // Release cached inflaters and close inflaters cache 1st 641 Deque<Inflater> inflaters = this.inflaters; 642 if (inflaters != null) { 643 synchronized (inflaters) { 644 // no need to double-check as only one thread gets a chance 645 // to execute run() (Cleaner guarantee)... 646 Inflater inf; 647 while ((inf = inflaters.poll()) != null) { 648 inf.end(); 649 } 650 // close inflaters cache 651 this.inflaters = null; 652 } 653 } For example, in the following code: { ZipFile zf = ... ZipEntry ze = zf.getEntry(...); InputStream is = zf.getInputStream(ze); ... } // 1st GC round ...we want all native resources to be released in 1st GC round after 'zf' and 'is' become unreachable. Regards, Peter
On 11/01/2017 08:17 PM, Xueming Shen wrote:
Hi Peter,
I like the idea of moving get/releaseInflter() into CleanableResource, though I doubt how much it can really help the GC it should be a good thing to do to remove the strong reference of ZipFile from stream's cleaner, and the code appears a little cleaner as well.
I was debating with myself whether or not the ZipFile.close() should throw an UncheckedIOException (like those java.nio.file.Files methods do). But you're right it's not good to simply ignoring them silently. Now I catch/unwarp the potential UncheckedIOException from res.clean() and re-throw the embedded ioe.
I need to dig a little to recall the real reason of zsrc==null check in ensureOpen() the comment does not sound updated. res.zsrc is not final and it is set to null when clean up/close. So I keep it for now.
Most (if not all?) "minor nit" has been updated accordingly.
It seems like we might have have put the "finalize()" method back as an empty-body method for compatibility reason. So not done yet :-)
http://cr.openjdk.java.net/~sherman/8185582/webrev/
thanks, sherman
On 11/1/17, 5:04 AM, Peter Levart wrote:
Hi Sherman,
On 11/01/17 00:25, Xueming Shen wrote:
Hi Peter,
After tried couple implementations it seems the most reasonable approach is to use the coding pattern you suggested to move all pieces into ZSStream Ref. Given we are already using the internal API CleanerFactory it is attractive to go a little further to subclass PhantomCleanable directly (in which I would assume we can save one extra object), but I think I'd better to follow the "suggested" clean usage (to register a runnable into the cleaner) for now.
39 class ZStreamRef implements Runnable { 40 41 private LongConsumer end; 42 private volatile long address; 43 private final Cleanable cleanable; 44 45 ZStreamRef (Object owner, LongSupplier addr, LongConsumer end) { 46 this.cleanable = CleanerFactory.cleaner().register(owner, this); 47 this.end = end; 48 this.address = addr.getAsLong(); 49 } 50
Similar change has been made for the ZipFile cleaner to follow the same coding pattern. The "cleaner" is now renamed from Releaser to CleanableResource.
http://cr.openjdk.java.net/~sherman/8185582/webrev/
Thanks, Sherman
This looks mostly fine. One minor nit is that ZStreamRef.address field does not have to be volatile. I checked all usages of ZStreamRef.address() and all of them invoke it while holding a lock on the ZStreamRef instance. The ZStreamRef.run() which modifies the address is also synchronized. The other minor nit is that the following ZipFile imports are not needed:
import java.nio.file.Path; import java.util.Map; import jdk.internal.misc.JavaIORandomAccessFileAccess; import static java.util.zip.ZipConstants.*;
(at least my IDEA colors them unused)
Cleaner is modified in this webrev (just one empty line deleted) - better not touch this file in this changeset
Additional nits in ZipFile:
- in lines 355, 356 you pull out two res fields into locals, but then not use them consistently (lines 372, 389) - line 403 has a TAB character (is this OK?) and shows incorrectly indented in my editor (should I set tab stops differently?) - line 457 - while should actually be if ? - in ensureOpen() the check for res.zsrc == null can never succeed (CleanableResource.zsrc is a final field. If CleanableResource constructor fails, there's no res object and there's no ZipFile object either as ZipFile constructor does not do anything for this to escape prematurely) - why don't you let IOExceptions thrown from CleanableResource.run() propagate out of ZipFile.close() ?
I would also rename static method Source.close(Source) to Source.release(Source) so it would not clash with instance method Source.close() which makes it ambiguous when one wants to use Source::close method reference (both methods apply). I would also make static methods Source.get() and Source.release(Source) package-private (currently one is public and the other is private, which needs compiler bridges to be invoked) and both are in a private nested class.
Inflater/Deflater/ZipFile now follow the coding pattern as suggested. But ZipFileInflaterInputStream still does not. It's not critical since failing to register cleanup which releases the inflater back into cache would simply mean that Inflater employs its own cleanup and ends itself.
And now another thing I would like to discuss. Why an initiative for using Cleaner instead of finalization()? Among drawbacks finalization has one of the more troubling is that the tracked referent survives the GC cycle that initiates its finalization reference processing pipeline, so the GC may reclaim the object (and referenced objects) only after the finalize() has finished in yet another GC round. Cleaner API separates the tracked object from the cleanup function and state needed to perform it into distinct instances. The tracked object can be reclaimed and the cleanup reference processing pipeline initiated in the same GC cycle. More heap may be reclaimed earlier.
Unless we are careful and create a cleaning function for one tracked object which captures (directly or indirectly) another object which registers its own cleaning function but we don't deal with explicit cleaning of the 2nd object in the 1st cleaning function.
Take for example the ZipFileInflaterInputStream's cleaning function. It captures the ZipFile in order to invoke ZipFile.releaseInflater instance method. What this means is that ZipFile will be kept reachable until all ZipFileInflaterInputStream's cleaning functions have fired. So we are back to the finalization drawback which needs at least 2 GC cycles to collect and clean-up what might be done in one go.
I suggest moving the getInflater() and releaseInflater() from ZipFile into the CleanableResource so that ZipFileInflaterInputStream's cleaning function captures just the CleanableResource and not the ZipFile. ZipFile therefore may become eligible for cleanup as soon as all opened input streams become eligible (but their cleaning functions need not have fired yet). CleanableResource.run() (the ZipFile cleaning function) and CleanableResource.releaseInflater() (the ZipFileInflaterInputStream's cleaning function) may therefore be invoked in arbitrary order (maybe even concurrently if one of them is explicit cleanup and the other is automatic), so code must be prepared for that.
I have tried to capture all above in a modified webrev (taking your last webrev as a base):
http://cr.openjdk.java.net/~plevart/jdk10-dev/8185582_ZIP.cleaner/webrev.03/
Regards, Peter
participants (8)
-
David Holmes
-
Florian Weimer
-
forax＠univ-mlv.fr
-
mandy chung
-
Peter Levart
-
Remi Forax
-
Roger Riggs
-
Xueming Shen