<div dir="ltr">I also have concerns about the use of fullFence here. I believe it should be the case that reachabilityFence guarantees visibility of memory operations program-ordered before the reachabilityFence(p) to the Cleaner associated with p. Mutator-collector synchronization should normally ensure that. On rereading, it also appears to me that the current reachabilityFence() documentation does not make that as clear as it should.<div><br></div><div>It appears to me that this is addressing an instance of the problem for which I suggested a more general, though also not completely elegant, solution in <a href="https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing">https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing</a> .</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 22, 2022 at 3:27 PM Stuart Marks <<a href="mailto:smarks@openjdk.org">smarks@openjdk.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian <<a href="mailto:bchristi@openjdk.org" target="_blank">bchristi@openjdk.org</a>> wrote:<br>
<br>
>> Please review this change to replace the finalizer in `AbstractLdapNamingEnumeration` with a Cleaner.<br>
>> <br>
>> 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.<br>
>> <br>
>> Details of note: <br>
>> 1. Some operations need to change the state values (the update() method is probably the most interesting).<br>
>> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read `homeCtx` from the superclass's `state`.<br>
>> <br>
>> 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.<br>
>> <br>
>> 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`). <br>
>> <br>
>> Thanks.<br>
><br>
> Brent Christian has updated the pull request incrementally with one additional commit since the last revision:<br>
> <br>
>   remove some more tabs<br>
<br>
I think the main outstanding issues here are as follows.<br>
<br>
First, are we convinced that it's necessary to add a try/finally block with a memory fence and a reachability fence in pretty much every mutator method? I think the answer is Yes, but we ought to have this ratified by memory model and GC experts.<br>
<br>
A second issue is whether VarHandle::fullFence is the right memory fence operation. I'm not quite convinced that it is, but I'm not sure what the right alternative is either.<br>
<br>
Finally, we probably also need approval from the maintainers of this code. :-)<br>
<br>
-------------<br>
<br>
PR: <a href="https://git.openjdk.org/jdk/pull/8311" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/8311</a><br>
</blockquote></div>