On Wed, 20 Apr 2022 13:48:21 GMT, Daniel Fuchs <[email protected]> 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.
>
> 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;`)
Yes, it is strange -- that code came from the finalizer. I will add an assert.
> 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?
Makes sense to me. `cleanable` can be final, too.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8311
