RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization
Vitaly Davidovich
vitalyd at gmail.com
Wed Dec 9 13:45:39 UTC 2015
At the risk of going off topic with respect to this thread, I really think
java should allow specifying at least what should be captured explicitly
(if unspecified, behaves like today). It's counterintuitive that capturing
just a field of 'this' captures the entire object, although some other
languages do the same.
sent from my phone
On Dec 9, 2015 4:30 AM, "Peter Levart" <peter.levart at gmail.com> wrote:
>
>
> On 12/09/2015 08:54 AM, David Holmes wrote:
>
>> 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
>>
>
> I can't seem to find anything about capturing variables by lambda
> expressions in JLS (except the talk about definitely assigned local
> variables). I think lambdas ware meant to act like anonymous inner classes.
> What they do is they capture a variable not the value variable has at
> capture time. For locals this is the same, as lambdas and anonymous inner
> classes only allow capturing "effectively final" and "definitely assigned"
> local variables, which means only locals that don't change the value after
> they are captured. With fields this is different. Instance fields are never
> really final (even if declared final). And as fields are implemented, they
> can not exist without the object that contains them...
>
> Regards, Peter
>
> P.S. The difference between anonymous inner class creation expression and
> lambda expression is that in instance scope, anonymous inner class always
> "captures" the outer instance while lambda expression only if it has to.
>
>
>>
>>> 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