Hi Mandy,
On 12/16/2015 3:07 PM, Mandy Chung wrote:
The change
On Dec 16, 2015, at 11:47 AM, Roger Riggs <roger.ri...@oracle.com> wrote:
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/
This change looks good. Having clear() throwing UOE is right thing to do and
prevent user code to cast and call Reference::clear.
Nit: There is no return value and this @return is not needed.
* @return does not return
right
In the test:
228 Object o = r.get();
229 if (!(r instanceof PhantomReference) && !cleaned) {
230 Reference<?> expectedRef = test.getRef();
231 Assert.assertEquals(expectedRef.get(), o,
232 "Object reference incorrect");
233 }
Curious on this test case: Is r.get() calling the overridden get() method that
always throws null?
The verifyGetRef is used to test both the subclassable XXXCleanable refs and
the PhantomCleanableRef that is exposed using Cleaner.register.
verifyGetRef can be simplified since it is only used (now) to verify the
ref before it is cleaned
so get returns non-null, except for phantoms.
Thanks, Roger
Mandy
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
