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.ri...@oracle.com
<mailto:roger.ri...@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.lev...@gmail.com
<mailto: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/
<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
