RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[David Holmes]

Actually I'm having more doubts about this API.

Library writers use finalize() as a last ditch cleanup mechanism in case 
the user doesn't explicitly call any "cleanup" method. So as a library 
writer I would think I am now expected to register my instances with a 
Cleaner and provide a Runnable that does what finalize() would have 
done. But in that usage pattern the end user of my objects never has any 
access to my Cleanables so can never call clean() themselves - instead 
they should be calling the cleanup function directly, just as they would 
previously. So the whole "invoke at most once" for the clean() method 
seems somewhat unnecessary; and the way we should write the cleanup 
method and the Runnable need to be more cleary explained as the 
idempotentcy of the cleanup needs to be handled in the library writers 
code not the Cleaner/Clenable implementation.

David

On 8/12/2015 6:09 PM, David Holmes wrote:
> Hi Roger,
>
> Sorry I had no choice but to look at this more closely ... and apologies
> as this is very late feedback ... I only looked at the API not the
> details of the implementation.
>
> On 8/12/2015 4:50 AM, Roger Riggs wrote:
>> Hi David,
>>
>> Thanks for the comments,
>>
>> Updated the javadoc and webrev with editorial changes.
>>
>> [1]http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
>> [2]http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html
>
> Should cleaning and cleanables be mentioned as part of the package-doc
> for java.lang.ref? Else they seem to be an overlooked add-on not part of
> the core reference related functionality. Perhaps even state how they
> are preferred to use of finalization?
>
> Cleaner.Cleanable:
>
> It was unclear to me what the usage model was for this. I'm assuming
> that the intent is that rather than register a "thunk" (lets call it an
> "action") that can be invoked directly by user-code, the user should
> invoke the action via the call to clean(). In which case I think it
> should be explained somewhat more clearly - see below.
>
> I would describe the Cleanable class as:
>
> Cleanable: Represents an object that has been registered for cleanup by
> a Cleaner. The object can be cleaned directly, by a call to clean(), if
> it is no longer to be used, else it will be cleaned automatically when
> the object becomes phantom-reachable.
>
> Cleanable.clean: Unregisters this Cleanable and performs the cleanup
> action that was associated with it. If this Cleanable has already been
> unregistered nothing happens. The cleanup action is invoked at most once
> per registered Cleanable, regardless of the number of calls to clean().
>
> ---
>
> Looking at Cleaner ....
>
>
> "Cleaner manages a set of object references and corresponding cleaning
> functions"
>
> I would say "cleaning actions" rather than functions as they yield no
> value. This change needs to be made throughout.
>
> "The most efficient use is to explicitly invoke the clean method when
> the object is closed or no longer needed. The cleaning function is a
> Runnable to be invoked at most once when the object is no longer
> reachable unless it has already been explicitly cleaned."
>
> To me this doesn't quite capture the need to not use the Runnable
> directly. I would rephrase:
>
> "In normal use a object should be cleaned up when no longer required, by
> invoking the clean() method of the associated Cleanable. This guarantees
> that the cleaning action will be performed at most once per object:
> either explicitly, or automatically if it becomes phantom-reachable. If
> cleaned explicitly the object should not be used again. Note that the
> cleaning action must not refer to the object ..."
>
> ---
>
> Question: what happens if an object is registered simultaneously with
> multiple Cleaners? Do we need to warn the user against that?
>
> ---
>
> The phrase "process the unreachable objects and to invoke cleaning
> functions" doesn't quite seem right to me. The objects themselves are
> never processed, or even touched - right? So really the thread is
> started to "invoke the cleanup actions for unreachable objects".
>
> create(): can also throw SecurityException if not allowed to
> create/start threads.
>
> register(Object obj, Runnable thunk): thunk -> action
>
> Thanks,
> David
>
>
>>
>>
>> On 12/6/15 7:46 PM, David Holmes wrote:
>>> Hi Roger,
>>>
>>> Sorry to be late here but was trying not to get involved :)
>>>
>>> It is already implicit that ThreadFactory.newThread should return
>>> unstarted threads - that is what a new Thread is - so I don't think
>>> IllegalThreadStateException needs to be documented here as it is
>>> documenting behaviour of a broken ThreadFactory (and a broken
>>> ThreadFactory could throw anything) .
>> It does seem that new is fairly well understood but one can read of
>> ThreadFactory is as a bit ambiguous, lacking a direct reference to the
>> Thread.State of the new thread
>> and since it allows various attributes of the thread to be modified
>> after the constructor.
>> Since the runnable is supplied as an argument it is ready to be started,
>> why not.
>> It seemed useful to reinforce the salient points.
>>>
>>> Also the no-arg cleaner() can also throw SecurityException.
>> The thread construction is done in doPriv so it should not throw.
>> Did I miss some edge case?
>>>
>>> Also this:
>>>
>>> 127      * On each call the {@link ThreadFactory#newThread(Runnable)
>>> thread factory}
>>>  128      * should return a {@link Thread.State#NEW new thread} with
>>> an appropriate
>>>  129      * {@linkplain Thread#getContextClassLoader context class
>>> loader},
>>>  130      * {@linkplain Thread#getName() name},
>>>  131      * {@linkplain Thread#getPriority() priority},
>>>  132      * permissions, etc.
>>>
>>> then begs the questions as to what is "appropriate". I think this can
>>> be said much more simply as: "The ThreadFactory must provide a Thread
>>> that is suitable for performing the cleaning work". Though even that
>>> raises questions. I'm not sure why a ThreadFactory is actually needed
>>> here ?? Special security context? If so that could be mentioned, but I
>>> don't think name or priority need to be discussed.
>> It was intended to prod the client to be deliberate about the
>> threadFactory.
>> Since the client is integrating the Cleaner and respective cleaning
>> functions
>> with the client code, the ThreadFactory makes it possible for the
>> client to
>> initialize a suitable thread and the comments serve as a reminder.
>> I agree that the phrase 'suitable for performing the cleaning work' is
>> the operative one.
>>
>> Thanks, Roger
>>>
>>> Thanks,
>>> David
>>>
>>