On Dec 8, 2015, at 10:51 AM, Peter Levart <peter.lev...@gmail.com> wrote:
> On 12/08/2015 04:34 PM, Roger Riggs wrote:
>>
>> private final Cleaner.Cleanable cleanable = cleaner.register(this, ()
>> -> fd.close());
>
> 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:
> To correct that, but still use lambda, you would have to capture a local
> variable
It looks like using "fd::close" might also work, and is more concise:
IntSupplier x = () -> 10;
IntSupplier xS = x::getAsInt;
@Test
public void test() {
System.out.println(xS.getAsInt());
x = () -> 15;
System.out.println(xS.getAsInt());
}
10
10
>
>>
>> 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
>>>>>>>
>>>>>>
>>>
>>
>
