Can we enhance java to allow specifying lambda capture and how the value is captured? :)
This is very subtle to the point where I maybe wouldn't even encourage use of lambda in this context. sent from my phone On Dec 8, 2015 4:25 PM, "Peter Levart" <peter.lev...@gmail.com> 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 >>>>>>>>> >>>>>>>>> >