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

[Roger Riggs]

On Fri, 27 May 2022 22:09:22 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 with a new target base due to a merge or a rebase. The pull request now contains 14 commits:
> 
>  - Merge branch 'master' into remove-finalizers
>  - Threading-related fixes
>  - NOW it builds
>  - Fix the merge fix
>  - Fix merge
>  - Merge
>  - Rename inner class to EnumCtx ; use homeCtx() in AbstractLdapNamingEnumeration for consistencty ; new instance vars can be final
>  - fix whitespace
>  - Merge branch 'master' into remove-finalizers
>  - Test changes to test new cleaner code
>  - ... and 4 more: https://git.openjdk.java.net/jdk/compare/ed8e8ac2...4af66bff

src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 216:

> 214:         } finally {
> 215:             // Ensure Cleaner does not run until after this method completes
> 216:             Reference.reachabilityFence(this);

I don't think there is any benefit to the `try{} finally {fence}`.
The reachabilityFence has no executable code. Its only purpose is to keep the reference in scope alive.

src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 409:

> 407:         } finally {
> 408:             // Ensure Cleaner does not run until after this method completes
> 409:             Reference.reachabilityFence(enumCtx);

Ditto, the try/finally is unnecessary.

src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 440:

> 438:             listArg = ne.listArg;
> 439:         } finally {
> 440:             // Ensure Cleaner does not run until after this method completes

Ditto try/finally is unnecessary.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8311