RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]

[Hans Boehm]

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.

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
https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing
.

On Fri, Jul 22, 2022 at 3:27 PM Stuart Marks <smarks at openjdk.org> wrote:

> On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian <bchristi at 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:
> >
> >   remove some more tabs
>
> I think the main outstanding issues here are as follows.
>
> 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.
>
> 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.
>
> Finally, we probably also need approval from the maintainers of this code.
> :-)
>
> -------------
>
> PR: https://git.openjdk.org/jdk/pull/8311
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20220724/fcf8d713/attachment.htm>