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

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

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

Xueming Shen
Hi

Please review the revision to the change for

JDK-8187485: Update Zip implementation to use Cleaner, not finalizers

For compatibility with previous jdk releases, in this proposed revision
the corresponding
close()/end() methods will be called when the ZipFile/Inflater/Deflater
object has become
unreachable, if the ZipFile/Inflater/Deflater has been subclassed and
its close()/end() method
has been overridden. In this case, the finalization mechanism is used to
clean up the underlying
resources.  If the ZipFile/Inflater/Deflater has not been subclassed OR
the corresponding
close()/end() method has not been overridden, then the Cleaner mechanism
is used to do
the cleanup.


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

Thanks,
Sherman
Reply | Threaded
Open this post in threaded view
|

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

roger riggs
Hi Sherman,

Looking good.

Deflater(573) and Inflater(398), and ZipFile(890):
    For consistency, where is says "phantom-reachable" in the finalize
methods it can say "unreachable".

ZStreamRef:
  - line 92: FinalizableZStreamRef  - owner should be final

Regards,  Roger

On 12/4/2017 6:14 PM, Xueming Shen wrote:

> Hi
>
> Please review the revision to the change for
>
> JDK-8187485: Update Zip implementation to use Cleaner, not finalizers
>
> For compatibility with previous jdk releases, in this proposed
> revision the corresponding
> close()/end() methods will be called when the
> ZipFile/Inflater/Deflater object has become
> unreachable, if the ZipFile/Inflater/Deflater has been subclassed and
> its close()/end() method
> has been overridden. In this case, the finalization mechanism is used
> to clean up the underlying
> resources.  If the ZipFile/Inflater/Deflater has not been subclassed
> OR the corresponding
> close()/end() method has not been overridden, then the Cleaner
> mechanism is used to do
> the cleanup.
>
>
> 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
>
> Thanks,
> Sherman

Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Xueming Shen


On 12/4/17 3:14 PM, Xueming Shen wrote:
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8185582
> webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev
>

ZStreamRef.java 79 static ZStreamRef get(Object owner, LongSupplier
addr, LongConsumer end) {
It may be better to have two factory methods that take the specific type
(Deflater & Inflater) instead of this single method taking Object owner.
ZipFile.java 701 // List of cached Inflater objects for decompression
702 Deque<Inflater> inflaterCache;should this be a volatile field?
TestCleaner.java    27  * @summary Check the resources of Inflater, Deflater and ZipFile are always
   28  *          cleaned/released when the instane is not unreachable

typo: s/instane/instance and s/not unreachable/unreachable

Otherwise looks good.
Mandy

Reply | Threaded
Open this post in threaded view
|

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

Xueming Shen
On 12/7/17, 8:31 AM, mandy chung wrote:

>
>
> On 12/4/17 3:14 PM, Xueming Shen wrote:
>>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8185582
>> webrev: http://cr.openjdk.java.net/~sherman/8185582/webrev
>>
>
> ZStreamRef.java
>
>    79     static ZStreamRef get(Object owner, LongSupplier addr, LongConsumer end) {
>
> It may be better to have two factory methods that take the specific type (Deflater&  Inflater)
> instead of this single method taking Object owner.

Just feel it is not worth creating two methods, and then two subclasses
for two
almost identical implementations. Arguably even the ZStreamRef should
have two
separate versions. So I choose to pay the price of two runtime "if".

>
>
> ZipFile.java
>
>   701         // List of cached Inflater objects for decompression
>   702         Deque<Inflater>  inflaterCache;
>
> should this be a volatile field?
I read it again, still feel the "volatile" is not necessary in this
situation. Let me know
if I'm doing it wrong.

>
>
> TestCleaner.java
>    27  * @summary Check the resources of Inflater, Deflater and ZipFile are always
>    28  *          cleaned/released when the instane is not unreachable
>
> typo: s/instane/instance and s/not unreachable/unreachable
updated in webrev.

Thanks,
Sherman