RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]

Sat Jul 30 11:47:12 UTC 2022

On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian <bchristi at 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