On Thu, 15 May 2025 23:59:41 GMT, Brent Christian <bchri...@openjdk.org> wrote:
>> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with Cleaner. >> >> (The [first PR](https://github.com/openjdk/jdk/pull/8311) for this fix >> started some substantial discussions, leading to, among other things, the >> [8314480](https://bugs.openjdk.org/browse/JDK-8314480) `java.lang.ref` >> memory ordering spec update.) >> >> In standard fashion, pieces of state required for cleanup (`LdapCtx >> homeCtx`, `LdapResult res`, and `LdapClient enumClnt`) are moved into a >> _Context_ object. 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). Use of `reachabilityFence()` ensures memory >> visibility on the Cleaner thread (per the aforementioned spec update). >> 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`. It confirms that the >> use of Cleaner does not keep an `LdapSearchEnumeration` object reachable. >> The other `AbstractLdapNamingEnumeration` subclasses >> (`LdapNamingEnumeration` and `LdapBindingEnumeration`) can be expected to >> behave the same. >> >> Thanks. >> >> **Edit: (Re)viewers: due to there being a lot of indentation changes, you >> might consider enabling the "Hide whitespace" option on the "Files changed" >> tab. To my eye, it gives a better view of the changes.** > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > update copyright src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 142: > 140: this.enumCtx = new EnumCtx(homeCtx, answer, homeCtx.clnt); > 141: // Ensures that context won't get closed from underneath us > 142: this.enumCtx.homeCtx.incEnumCount(); @bchristi-git Would it make sense to increment this in the EnumCtx ctor instead? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25242#discussion_r2137343209
