RFR 9: JDK-8146028 : Common Cleaner for finalization replacements in java.base

[Peter Levart]

On 01/05/2016 10:16 PM, Roger Riggs wrote:
> Hi Daniel,
>
> webrev updated to revert changes to MeteredStream as too risky.
>
> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html


Hi Roger,

I briefly skimmed over the webrev, and found the following issue in 
ProcessImpl:

  420         // Register a cleaning function to close the handle
  421         CleanerFactory.getCleaner().register(this, () -> 
closeHandle(handle));
  422

... 'handle' is an instance variable, which means that 'this' is 
captured by the lambda. You will have to assign handle to a local var 
and capture it instead to prevent 'this' to be captured.

Regards, Peter


>
> On 1/5/2016 1:45 PM, Daniel Fuchs wrote:
>> Hi Roger,
>>
>> Some early feedback:
>>
>> I see that prior to your changes, MeteredStream.close() would
>> be called by finalize.
>> This will no longer be the case after
>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html
> In the case where finalize would call close(), there can be no 
> queuedForCleanup activity
> because there can be no strong reference from the 
> KeepAliveStreamCleaner thread
> or the queue that holds the pending cleanups.
> It cannot be in the Cache of connections being kept alive or the 
> thread that keeps them alive.
>
> It might be possible that the underlying connection is still open; but 
> there is no advantage
> of trying to drain it.  There is the possibility of resurrecting it by 
> virtue of deciding that
> it should be queued for cleanup and starting a new thread to do the 
> cleanup.
>
> All in all, there is a risk of misunderstanding the dynamic behavior 
> and it would be safer
> to leave this using finalize.
>
>>
>> I see that MeteredStream has a subclass (KeepAliveStream) that
>> overrides close() - so KeepAliveStream probably requires a cleanup
>> as well - doesn't it?
>>
>> Finally I'd suggest making the variable 'pi' final - since you're
>> passing that to the lambda you don't want to allow its value to
>> be modified afterwards (fortunately it's not - and I believe it
>> should be good to ensure it won't be).
> ok, it would not go unnoticed, the compiler should complain if the 
> variable is no effectively final.
>
> Thanks, Roger
>
>>
>> I haven't looked at the other classes yet...
>>
>> best regards,
>>
>> -- daniel
>>
>> On 05/01/16 19:24, Roger Riggs wrote:
>>> The follow on work to adding the Cleaner is to replace uses of
>>> finalization with uses of the Cleaner.
>>> For the 'easy' cases in the java.base module, it is useful to introduce
>>> a private Cleaner for the
>>> java.base module.  It is proposed to be held weakly, to allow it to
>>> terminate on a lightly loaded
>>> system.
>>>
>>> Webrev for Review:
>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/
>>>
>>>
>>> The 2nd step is using the Cleaner.
>>>   - Empty finalize methods should (I think) be removed; but since they
>>> are part of the public spec
>>>     the process needs two full releases; so the proposal is to 
>>> deprecate
>>> them first.
>>>     (The JEP 277 necessary changes will be updated when JEP 277
>>> semantics are finalized)
>>>
>>> - In a few cases, the code in the finalizer is redundant and if removed
>>> would allow
>>>    an optimization of the handling of the finalizer and future removal
>>> of the finalize method.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html 
>>>
>>>
>>> Thanks for comments and suggestions,  Roger
>>>
>>>
>>>
>>>
>>
>