On Wed, 20 Jul 2022 22:21:35 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 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
