RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization
David Holmes
david.holmes at oracle.com
Wed Dec 9 07:54:51 UTC 2015
On 9/12/2015 5:05 PM, Peter Levart wrote:
> Hi,
>
> I think the only way to try to prevent such things is with a good
> example in javadoc that "screams" of possible miss-usages.
Problem is that many people - myself included - would not have a clue
when a lambda "captures this"! Is that part of lambda expressions or a
consequence of the current implementation?
David
>
> public static class CleanerExample implements AutoCloseable {
>
> private static final Cleaner cleaner = ...; // preferably a
> shared cleaner
>
> private final PrivateNativeResource pnr;
>
> private final Cleaner.Cleanable cleanable;
>
> public CleanerExample(args, ...) {
>
> // prepare captured state as local vars...
> PrivateNativeResource _pnr = ...;
>
> this.cleanable = cleaner.register(this, () -> {
> // DON'T capture any instance fields with lambda since
> that would
> // capture 'this' and prevent it from becoming
> phantom-reachable!!!
> _pnr.close();
> });
>
> this.pnr = _pnr;
> }
>
> public void close() {
> cleanable.clean();
> }
>
>
> Regards, Peter
>
> On 12/09/2015 05:15 AM, Vitaly Davidovich wrote:
>> 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.Riggs at oracle.com
>> <mailto:Roger.Riggs at 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.levart at gmail.com
>> <mailto: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/
>>
>> <http://cr.openjdk.java.net/%7Erriggs/webrev-cleaner-8138696/>
>>
>> [2]http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html
>>
>> <http://cr.openjdk.java.net/%7Erriggs/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