This has the same problem, doesn't it? The bottom line is if the lambda is
() -> <access a field in any manner> you're getting a capture of `this`.
On Tue, Dec 8, 2015 at 5:08 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
> Hi,
>
> Another option that should always capture is to define a specific static
> method with the needed values as arguments:
>
>
> public static class CleanerExample implements AutoCloseable {
>
> FileDescriptor fd = ...;
>
> private static final Cleaner cleaner = Cleaner.create();
>
> private final Cleaner.Cleanable cleanable =
> cleaner.register(this,*() -> cleanup(fd)*);
>
> @Override
> public void close() {
> cleanable.clean();
> }
>
> *static void cleanup(FileDescriptor fd ...) {**
> ** fd.close();**
> ** }*
>
> }
>
> On 12/8/2015 4:24 PM, Peter Levart wrote:
>
>>
>>
>> On 12/08/2015 08:08 PM, Steven Schlansker wrote:
>>
>>> 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
>>>
>>
>> Yes, good idea. This is a pre-bound method reference (the part on the
>> left of '::' is evaluated immediately). Contrast to lambda, where
>> "fd.close()" is an expression in the lambda body which is evaluated when
>> lambda is invoked and that expression is composed of a field get + method
>> invocation. In order to get an instance field, the object containing it
>> must be captured.
>>
>> So for Roger's example, this will work:
>>
>>
>> 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();
>> }
>> }
>>
>>
>> ...if FileDescriptor.close() is an instance method with no arguments and
>> doesn't throw any checked exceptions.
>>
>>
>> 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
>>>>>>>>>>
>>>>>>>>>>
>>
>
