On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian <bchri...@openjdk.org> wrote:
>> Please review this change to replace the finalizer in
>> `AbstractLdapNamingEnumeration` with a Cleaner.
>>
>> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
>> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
>> there, the change is fairly mechanical.
>>
>> Details of note:
>> 1. Some operations need to change the state values (the update() method is
>> probably the most interesting).
>> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
>> `homeCtx` from the superclass's `state`.
>>
>> The test case is based on a copy of
>> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test
>> case might be possible, but this was done for expediency.
>>
>> The test only confirms that the new Cleaner use does not keep the object
>> reachable. It only tests `LdapSearchEnumeration` (not
>> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are
>> subclasses of `AbstractLdapNamingEnumeration`).
>>
>> Thanks.
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove some more tabs
Changes requested by plevart (Reviewer).
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 82:
> 80: public void run() {
> 81: // Ensure changes on the main/program thread happens-before
> cleanup
> 82: VarHandle.fullFence();
no need for memory fence as explained in various finally blocks...
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 93:
> 91: homeCtx.decEnumCount();
> 92: homeCtx = null;
> 93: }
Cleaner's task (run() method) is guaranteed to be called at most once. So
there's no need for above "idempotent" logic. Unless some of those fields are
optional when the instance is constructed....
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 172:
> 170:
> 171: // Ensure writes are visible to the Cleaner thread
> 172: VarHandle.fullFence();
I don't think any memory fences are needed here. Reachability fence is enough.
Cleaner runs in its own thread by polling enqued PhantomReference(s) from the
ReferenceQueue. There is a happens-before edge between ReferenceHandler thread
enqueue-ing "pending" PhantomReferences and Cleaner thread dequeue-ing them.
There is a happens-before edge between GC thread clearing a PhantomReference
and linking it into a "pending" list and ReferenceHandler thread unlinking it
from the "pending" list and enque-ing it into a ReferenceQueue. There is a
happens-before edge before a call to Reference.reachabilityFence(referent) and
a GC thread discovering a phantom-reachable referent and clearing the
PhantomReference and linking it into a "pending" list.
So there you have a happens-before chain which makes all writes before a call
to eference.reachabilityFence(this) visible to a Cleaner task that is
initialized to observe reachabillity of this....
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 192:
> 190:
> 191: // Ensure writes are visible to the Cleaner thread
> 192: VarHandle.fullFence();
Same as above. Memory fences are not needed.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 277:
> 275: } finally {
> 276: // Ensure writes are visible to the Cleaner thread
> 277: VarHandle.fullFence();
The memory fence is not needed (as explained above), but...
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 279:
> 277: VarHandle.fullFence();
> 278: // Ensure Cleaner does not run until after this method
> completes
> 279: Reference.reachabilityFence(enumCtx);
The reachability fence should take 'this' as a parameter, not enumCtx. Since
'this' reachability is tracked by Cleaner and not 'enumCtx' reachability.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 296:
> 294: } finally {
> 295: // Ensure writes are visible to the Cleaner thread
> 296: VarHandle.fullFence();
The memory fence is not needed (as explained above), but...
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 298:
> 296: VarHandle.fullFence();
> 297: // Ensure Cleaner does not run until after this method
> completes
> 298: Reference.reachabilityFence(enumCtx);
The reachability fence should take 'this' as a parameter, not enumCtx. Since
'this' reachability is tracked by Cleaner and not 'enumCtx' reachability.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 343:
> 341: VarHandle.fullFence();
> 342: // Ensure Cleaner does not run until after this method
> completes
> 343: Reference.reachabilityFence(enumCtx);
The same as above: no need for memory fence and reachability fence should take
'this' as parameter.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 382:
> 380: VarHandle.fullFence();
> 381: // Ensure Cleaner does not run until after this method
> completes
> 382: Reference.reachabilityFence(enumCtx);
Same as above: no need for memory fence and reachability fence should take
'this' as parameter.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 415:
> 413: } finally {
> 414: // Ensure writes are visible to the Cleaner thread
> 415: VarHandle.fullFence();
No need for memory fence.
src/java.naming/share/classes/com/sun/jndi/ldap/LdapBindingEnumeration.java
line 107:
> 105: } finally {
> 106: // Ensure writes are visible to the Cleaner thread
> 107: VarHandle.fullFence();
no need for memory fence.
src/java.naming/share/classes/com/sun/jndi/ldap/LdapBindingEnumeration.java
line 121:
> 119: } finally {
> 120: // Ensure writes are visible to the Cleaner thread
> 121: VarHandle.fullFence();
no need for memory fence.
src/java.naming/share/classes/com/sun/jndi/ldap/LdapNamingEnumeration.java line
76:
> 74: } finally {
> 75: // Ensure writes are visible to the Cleaner thread
> 76: VarHandle.fullFence();
no need for memory fence.
src/java.naming/share/classes/com/sun/jndi/ldap/LdapNamingEnumeration.java line
90:
> 88: } finally {
> 89: // Ensure writes are visible to the Cleaner thread
> 90: VarHandle.fullFence();
no need for memory fence
src/java.naming/share/classes/com/sun/jndi/ldap/LdapSearchEnumeration.java line
198:
> 196: } finally {
> 197: // Ensure writes are visible to the Cleaner thread
> 198: VarHandle.fullFence();
memory fence is not needed.
src/java.naming/share/classes/com/sun/jndi/ldap/LdapSearchEnumeration.java line
224:
> 222: } finally {
> 223: // Ensure writes are visible to the Cleaner thread
> 224: VarHandle.fullFence();
no need for memory fence.
-------------
PR: https://git.openjdk.org/jdk/pull/8311
