RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization
Peter Levart
peter.levart at gmail.com
Tue Dec 8 18:51:22 UTC 2015
On 12/08/2015 04:34 PM, Roger Riggs wrote:
> Hi Peter,
>
> Thanks for the example and explanations.
>
> For simple cleanup of specific state values, I probably would have
> used lambda instead of an explicit
> inner class but both styles use the same mechanism.
> The point about using a shared cleaner can be reinforced in the
> example too.
>
> public static class CleanerExample implements AutoCloseable {
>
> FileDescriptor fd = ...;
>
> private static final Cleaner cleaner = Cleaner.create();
>
> private final Cleaner.Cleanable cleanable =
> cleaner.register(this, () -> fd.close());
>
> @Override
> public void close() {
> cleanable.clean();
> }
> }
>
Sorry Roger, but this example is flawed. This is tricky! The lambda "()
-> fd.close()" captures 'this', not only 'fd' as can be seen by running
the following example:
public class Test {
int x = 0;
final IntSupplier xSupplier = () -> x;
public static void main(String[] args) {
Test t = new Test();
System.out.println(t.xSupplier.getAsInt());
t.x = 12;
System.out.println(t.xSupplier.getAsInt());
}
}
...which prints:
0
12
To correct that, but still use lambda, you would have to capture a local
variable, like:
public class Test {
int x = 0;
final IntSupplier xSupplier;
{
int xValue = x;
xSupplier = () -> xValue;
}
public static void main(String[] args) {
Test t = new Test();
System.out.println(t.xSupplier.getAsInt());
t.x = 12;
System.out.println(t.xSupplier.getAsInt());
}
}
...now prints:
0
0
Regards, Peter
>
> I'll work the example into the javadoc.
>
> Roger
>
>
> On 12/08/2015 07:25 AM, Peter Levart wrote:
>>
>>
>> On 12/08/2015 09:22 AM, David Holmes wrote:
>>> 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
>>
>> Hi David, (once again for the list)
>>
>> I agree that an example would be most helpful. Here's how a normal
>> finalizable object is typically coded:
>>
>> public class FinalizeExample implements AutoCloseable {
>>
>> private boolean closed;
>>
>> @Override
>> public synchronized void close() {
>> if (!closed) {
>> closed = true;
>> // cleanup actions accessing state of
>> FinalizeExample, executed at most once
>> }
>> }
>>
>> @Override
>> protected void finalize() throws Throwable {
>> close();
>> }
>> }
>>
>>
>> Re-factoring to use Cleaner is a process that extracts the state
>> representing native resource from the user-facing class into a
>> private nested static class and makes the user-facing object just a
>> facade that has access to the state object and is registered with a
>> Cleaner. The Cleaner.Cleanable instance is also made accessible from
>> the user-facing object, so it can provide the on-demand cleaning:
>>
>> public static class CleanerExample implements AutoCloseable {
>>
>> private static class State implements Runnable {
>> @Override
>> public void run() {
>> // cleanup actions accessing State, executed at most
>> once
>> }
>> }
>>
>> private static final Cleaner cleaner = Cleaner.create();
>>
>> private final State state = new State();
>> private final Cleaner.Cleanable cleanable =
>> cleaner.register(this, state);
>>
>> @Override
>> public void close() {
>> cleanable.clean();
>> }
>> }
>>
>>
>> Regards, Peter
>>
>>>
>>> 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
>>>>>>
>>>>>
>>
>
More information about the core-libs-dev
mailing list