RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization
Peter Levart
peter.levart at gmail.com
Wed Dec 9 07:05:32 UTC 2015
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.
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