On Fri, 27 May 2022 22:00:24 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.
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
>   Threading-related fixes

`AbstractLdapEnumeration`'s mutable state brings the possibility of threading 
issues between the program and cleaner threads. I've added some 
threading-related changes to the fix. See my [comment in the bug 
 for additional background.

Since synchronization may now happen on the cleaner thread, I've changed 
`AbstractLdapEnumeration` to use its own `Cleaner` instance instead of the 
shared cleaner, for added safety. There have been deadlocks in ldap cleanup in 
the past.

The added `finally` blocks led to a lot of indentation changes. The "hide 
whitespace" option might help with viewing the changes.


PR: https://git.openjdk.java.net/jdk/pull/8311

Reply via email to