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 Generally looks good. I haven't verified whether all methods that need a reachabilityFence have it. This will require further analysis (from me). As Alan noted it would be really good if we could use try-with-resource, but I am not expert enough in LDAP search implementation to figure out whether that's feasible. Your proposed fix preseves the status quo - where a finalizer has been replaced by a cleaner - and asserting that the two are equivalent seems more trackable for now. src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java line 453: > 451: try { > 452: // Cleanup previous context first > 453: getHomeCtx().decEnumCount(); What is the reason for calling `getHomeCtx()` here when we use `enumCtxt.homeCtx` a few lines below? ------------- PR Review: https://git.openjdk.org/jdk/pull/25242#pullrequestreview-2914626239 PR Review Comment: https://git.openjdk.org/jdk/pull/25242#discussion_r2138506580
