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 394: > 392: @SuppressWarnings("removal") > 393: protected final void finalize() { > 394: cleanup(); 🥳 src/java.naming/share/classes/com/sun/jndi/ldap/LdapSearchEnumeration.java line 59: > 57: // super() call (aka AbstrctLdapNamingEnumeration ctor) keeps > 'this' > 58: // reachable until end of Cleaner registration. Code after that > 59: // clearly does not touch cleanable state. @bchristi-git I fear this could be a brittle assumption. Would an extra reachabilityFence here hurt? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25242#discussion_r2137360174 PR Review Comment: https://git.openjdk.org/jdk/pull/25242#discussion_r2137365068
