I haven't been following along with the long review thread, but I took
a look at the new Cleaner stuff and have some belated comments:
---
Please add missing semicolon:
* private final Cleaner.Cleanable cleanable
---
The below looks non-sensical; maybe should return EV_CLEAR if some
clearing, not cleaning, should occur?
* @return EV_CLEAR if the cleanup should occur;
* EV_CLEAN if the cleanup should occur;
---
private final int hash;
private final ConcurrentHashMap<WeakKey<K>, ?> map;
Cleaner.Cleanable cleanable;
Why isn't cleanable also private final?
---
Assert.assertEquals(map.get(k2), data, "value should be found
in the map");
key = null;
System.gc();
Assert.assertNotEquals(map.get(k2), data, "value should not be
found in the map");
I initially expected this to fail at least intermittently because a
cleaner may not yet have removed the entry from the map.
Why aren't we calling whitebox.fullGC() everywhere where System.gc()
is being called?
What's particularly confusing here is that the WeakKey is not actually
surely removed from the map, but instead two WeakKeys that may have
been equal become non-equal when they get cleared by the gc. Which is
generally bad practice. Which suggests we wouldn't implement a real
public concurrent map of weak keys this way.
---
WeakCleanable and friends are defined in jdk.internal, don't appear to
be used at all, and since there are fields in CleanerImpl, they impose
a hidden tax on all users of Cleaner. I don't see why there is both
WeakCleanable (using subclassing) and WeakCleanableRef (using a
provided Runnable). Is a compelling use case for WeakCleanables
forthcoming?
---
I keep vaguely hoping to see a CompletableFuture-based API for cleanup actions.
