RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

classic Classic list List threaded Threaded
52 messages Options
123
Reply | Threaded
Open this post in threaded view
|

RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Xueming Shen
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart
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
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

David Holmes
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
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart
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
>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Xueming Shen
In reply to this post by Peter Levart
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
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Mandy Chung
In reply to this post by Xueming Shen

Comment on the CSR:

On 9/26/17 11:35 PM, Xueming Shen wrote:
>
> csr:       https://bugs.openjdk.java.net/browse/JDK-8187485
>
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

||
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Xueming Shen
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:
>>
>> csr: https://bugs.openjdk.java.net/browse/JDK-8187485
>>
> 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
> ||

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart
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:
>>>
>>> csr: https://bugs.openjdk.java.net/browse/JDK-8187485
>>>
>> 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
>> ||
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Mandy Chung
In reply to this post by Peter Levart


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Xueming Shen
In reply to this post by Peter Levart
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


Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Remi Forax
In reply to this post by Mandy Chung
----- Mail original -----
> De: "mandy chung" <[hidden email]>
> À: "Peter Levart" <[hidden email]>, "Xueming Shen" <[hidden email]>, "core-libs-dev"
> <[hidden email]>
> 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#reachabilityFence-java.lang.Object-

>
> Mandy

Rémi
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Mandy Chung
In reply to this post by Xueming Shen


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart
In reply to this post by Mandy Chung


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

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Mandy Chung
In reply to this post by Remi Forax


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

roger riggs
In reply to this post by Xueming Shen
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.html

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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart
In reply to this post by Remi Forax
Hi Remi,

On 09/29/17 22:49, Remi Forax wrote:

> ----- Mail original -----
>> De: "mandy chung" <[hidden email]>
>> À: "Peter Levart" <[hidden email]>, "Xueming Shen" <[hidden email]>, "core-libs-dev"
>> <[hidden email]>
>> 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#reachabilityFence-java.lang.Object-
>
>> Mandy
> Rémi

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Mandy Chung
In reply to this post by Xueming Shen


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
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Remi Forax
In reply to this post by Peter Levart
> De: "Peter Levart" <[hidden email]>
> À: "Remi Forax" <[hidden email]>, "mandy chung" <[hidden email]>
> Cc: "Xueming Shen" <[hidden email]>, "core-libs-dev"
> <[hidden email]>
> 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:[hidden email] | <[hidden email]> ]
>>> À: "Peter Levart" [ mailto:[hidden email] | <[hidden email]> ]
>>> , "Xueming Shen" [ mailto:[hidden email] | <[hidden email]> ]
>>> , "core-libs-dev" [ mailto:[hidden email] |
>>> <[hidden email]> ] 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#reachabilityFence-java.lang.Object
>> |
>> http://docs.oracle.com/javase/9/docs/api/java/lang/ref/Reference.html#reachabilityFence-java.lang.Object
>> ] -

>>> Mandy

>> Rémi
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart
In reply to this post by roger riggs
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.html
>
> 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
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8185582, Update Zip implementation to use Cleaner, not finalizers

Peter Levart


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.html
>>
>> 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
>>>
>>>
>>
>

123