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

[Brent Christian]

On Wed, 20 Jul 2022 22:21:35 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 26 commits:
> 
>  - remove tab
>  - Merge conflicts
>  - Convert fix to use VarHandle fences
>  - Merge branch 'master' into remove-finalizers
>  - reference emunCtx and homeCtx consistently in constructor
>  - Restore EnumCtx to being an inner class, to keep all the state together in the code
>  - Restore comments in ldap capture file
>  - Update test files - add copyright, etc
>  - added getters to EnumCtx, and moved it to top level ; use getters + local variables to reduce code changes
>  - test comment udpate
>  - ... and 16 more: https://git.openjdk.org/jdk/compare/b1817a30...dc444a54

I've updated how the reachability and memory visibility issues (per [comment](https://bugs.openjdk.org/browse/JDK-8283660?focusedCommentId=14498566&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14498566)) are addressed:

**Additional reachability fences**

Unless it's _immediately obvious_ that _**no**_ cleanable state is **_accessed_** during the course of a method's execution, all methods should include a `reachabilityFence` to ensure that cleanable state is not cleaned up while a method is still running. This applies to any class that uses a `Cleaner` (or a finalizer, really).

**`VarHandle.fullFence` to ensure memory visibility, instead of synchronized methods**
This makes for simpler code, and better reflects our intentions.

Unless it's _immediately obvious_ that **_no_** cleanable state is **_modified_** during the course of the method's execution, all methods should include a `fullFence` to ensure changes made on the main/program thread are seen by the cleanup thread. This applies to any class that uses a `Cleaner` (or a finalizer, really) where the state to be cleaned can be mutated during the course of execution.

Other possibilities for triggering the necessary volatile operations could be:
* empty synchronized blocks
* adding some "junk" variable to EnumCtx, along with volatile reads/writes of the variable where needed
---
One could perhaps use deep code tracing to determine that cleanable state is not accessed/written during the course of a method's execution, and omit fences based on that knowledge. However I believe that leaves too large a burden on future maintainers and reviewers to remember to re-perform the code tracing and re-confirm non-use of cleanable state, even when changing possibly "far away" code.

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

PR: https://git.openjdk.org/jdk/pull/8311