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

[Roger Riggs]

Hi Peter,

Thanks for the review and catching that.

Will fix.

Roger


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