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

[Chris Hegarty]

On 8 Jan 2016, at 19:57, Roger Riggs <Roger.Riggs at oracle.com> wrote:

> Hi Chris,
> 
> Updated webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/

Thanks Roger. This is starting to look like a nice, and useful, package ;-)

-Chris.

> On 1/8/2016 11:08 AM, Chris Hegarty wrote:
>> Hi Roger,
>> 
>> On 08/01/16 15:51, Roger Riggs wrote:
>>> Thanks for the review and comments:
>>> 
>>> While integrating the comments, some comments identified finalizers that
>>> were unneeded should be removed.
>>> I split that into a separate bug, as they are not being replaced, just
>>> removed. [2]
>>> 
>>> The CleaningFactory is moved to jdk.internal.ref so it can be exported
>>> selectively to specific
>>> OpenJDK modules.  The suggested refactoring of the subclassable
>>> XXXCleanables can be done separately
>>> if it turns out to be useful outside the base module.
>>> 
>>> Webrev for shared CleaningFactory and its uses:
>>> http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/
>> 
>> Thanks for moving this into its own package. Does is make sense to
>> move CleanerImpl? Not for it to be used by other modules, just
>> logically seems to belong there.
> Ok, moved CleanerImpl to jdk.internal.ref;  in the the process refactored it to expose fewer
> of the implementation methods as public to avoid temptations of misuse.
> The XXXCleanable classes are top level classes and not nested in CleanerImpl.
>> 
>> CleanerTest should have its @modules tag updated to include
>> java.base/jdk.internal.ref.
> Added
>> 
>> Trivially, "Cleaner for use within the java.base module" ->
>> 'OpenJDK modules'.
> Fixed
> 
> Thanks, Roger
> 
>> 
>> Otherwise, looks good to me.
>> 
>> Thanks,
>> -Chris.
>> 
>>> Webrev for removed finalizers:
>>> http://cr.openjdk.java.net/~rriggs/webrev-rmfinalizers-8146567/
>>> 
>>> Thanks, Roger
>>> 
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8146028 Common Cleaner for
>>> finalization replacements in OpenJDK
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8146567 Remove dead code
>>> finalizer methods
>>> 
>>> On 1/6/2016 2:02 PM, Peter Levart wrote:
>>>> 
>>>> 
>>>> 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