On Wed, 20 Apr 2022 00:32:25 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.
I also agree with Roger's suggestions.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 73:
> 71: public void run() {
> 72: if (enumClnt != null) {
> 73: enumClnt.clearSearchReply(res, homeCtx.reqCtls);
It's a bit strange to see that there is no guard here to verify that `homeCtx
!= null`, when line 76 implies that it might. My reading is that `homeCtxt` is
not supposed to be `null` when `enumClnt` is not `null`. That could be
explained in a comment, or alternatively asserted just before line 73 (`assert
homeCtx != null;`)
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 83:
> 81: }
> 82:
> 83: private CleaningAction state;
I wonder if state should be final?
-------------
PR: https://git.openjdk.java.net/jdk/pull/8311
