On Fri, 27 May 2022 22:09:22 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 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
