RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to usealternative to finalization
Vitaly Davidovich
vitalyd at gmail.com
Wed Dec 9 03:04:44 UTC 2015
sent from my phone
On Dec 8, 2015 9:52 PM, "Timo Kinnunen" <timo.kinnunen at gmail.com> wrote:
>
> Hi,
>
>
>
> Not encouraging the use of lambdas won’t stop people discovering lambda
usage for this on their own anyways, then writing blog posts about their
discovery, promoting it, extolling its simplicity, encouraging others to
use it too, etc… etc..
I know I know :).
>
>
>
> Let’s say I’ve forgotten about this issue and written some wrong/bad code
like this:
>
>
>
> Utils.cleaning.register(this, () ->
Arrays.stream(this.colors).forEach(Color::dispose));
>
>
>
>
>
> Question(s): What is the failure mode that I would be seeing? And would
it be immediately apparent that the code isn’t working properly, or how
soon?
You leak the object in the sense that it won't become unreachable to begin
with. It's subtle and likely to not be easily detected.
>
>
>
>
> --
> Have a nice day,
> Timo
>
> Sent from Mail for Windows 10
>
>
>
>
>
>
> From: Vitaly Davidovich
> Sent: Wednesday, December 9, 2015 00:09
> To: Peter Levart
> Cc: core-libs-dev
> Subject: Re: RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to
usealternative to finalization
>
>
>
>
>
> 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.levart at gmail.com> wrote:
>
>
>
> >
>
> >
>
> > On 12/08/2015 08:08 PM, Steven Schlansker wrote:
>
> >
>
> >> On Dec 8, 2015, at 10:51 AM, Peter Levart <peter.levart at 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
>
> >>>>>>>>>
>
> >>>>>>>>>
>
> >
>
>
>
>
More information about the core-libs-dev
mailing list