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

[Roger Riggs]

Hi Chris,

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

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