Hi Peter,
It was a bit more involved than I expected, mostly in the tests to make
this change.
Is this what you expected? (just the deltas, I'll merge the patches
before pushing).
http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696-no-clear/
Thanks, Roger
On 12/15/2015 6:01 PM, Peter Levart wrote:
On 12/15/2015 11:48 PM, Roger Riggs wrote:
Hi Peter,
That will break up clearing the ref when the Cleanable is explicitly
cleaned.
Reference.clear() needs to be called from Cleanable.clean().
From PhantomCleanable (the superclass of PhantomCleanableRef):
253 @Override
254 public final void clean() {
255 if (remove()) {
256 super.clear();
257 performCleanup();
258 }
259 }
260
261 /**
262 * Unregister this PhantomCleanable and clear the reference.
263 * Due to inherent concurrency, {@link #performCleanup()}
may still be invoked.
264 */
265 @Override
266 public final void clear() {
267 if (remove()) {
268 super.clear();
269 }
270 }
... clean() calls super.clear(), which is "invokespecial" (not a
virtual dispatch).
Regards, Peter
it might be nice to block that but to do so we'd need to go back to
separate objects
for the Reference and the Cleanable and we worked hard to get to a
single object.
Roger
On 12/15/2015 5:38 PM, Peter Levart wrote:
Hi Roger,
Just one thing about implementation:
Since the type exposed to user is Cleaner.Cleanable that has only a
single method clean(), it would be good if the implementation class
(CleanerImpl.PhantomCleanableRef) overrode
CleanerImpl.PhantomCleanable.clear() method and threw
UnsupportedOperationException, otherwise users will be tempted to
cast the returned Cleaner.Cleanable to Reference and invoke clear()
method to de-register cleanup action without invoking it. This is
the only remaining public Reference method that is not disabled this
way.
Regards, Peter
On 12/09/2015 07:40 PM, Roger Riggs wrote:
Hi,
The example is revised to caution about inner classes and lambdas.
[1]http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/
[2]http://cr.openjdk.java.net/~rriggs/cleaner-doc/index.html
Thanks, Roger
On 12/9/2015 11:04 AM, Peter Levart wrote:
Hi Chris,
On 12/09/2015 04:03 PM, Chris Hegarty wrote:
Peter,
On 09/12/15 07:05, 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.
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
I assume that the WARNING should include anonymous inner classes too
( which I expect are quite common, though less now with lambda ) ?
Is "leaking" 'this' in a constructor a potential issue with respect
to the visibility of pnr? As well as causing red-squiggly lines in
the IDE ;-)
'this' only leaks to the 'referent' field of PhantomReference
where by definition is not accessible.
'this' can become phantom-reachable before CleanerExample
constructor ends. But this is harmless, because the code that may
execute at that time does not access the object any more, so the
object may be safely collected.
Cleanup action can run at any time after registration even before
CleanerExample constructor ends. But this is harmless too, because
it only accesses PrivateNativeResource which is fully constructed
before registration of cleanup action.
I see no issues apart from IDE(s) not seeing no issues.
Regards, Peter
-Chris.
phantom-reachable!!!
_pnr.close();
});
this.pnr = _pnr;
}
public void close() {
cleanable.clean();
}
Regards, Peter
