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

[Hans Boehm]

On Mon, Jul 25, 2022 at 10:30 PM Stuart Marks <smarks at openjdk.org> wrote:
...
>
> Hans Boehm wrote:
>
> > 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
.
>
> Hi Hans, thanks for looking at this. In the prior discussions on
reachabilityFence and premature finalization, I don't recall seeing any
mention of
> memory model issues. To my mind the discussions here are the first
mention of them. (Of course it's possible I could have missed something.)
> The memory model is quite subtle and it's difficult to be sure of
anything. However, as time goes on we've become more confident that there
_IS_
> an issue here with the memory model that needs to be addressed.
>
> Then there is a the question of what to do about it. One possibility is
to add some memory ordering semantics into reachabilityFence(p), as you
suggest.
> As convenient as it might seem, it's not obvious to me that we want
reachability fused with memory order. At least we should discuss them
separately
> before deciding what to do.
>
> This PR seems to be on a long journey :-) so let me describe some of the
background and where I think this ought to go.
>
> First, like most PRs, this started off as an effort to make some code
changes. In this case it's part of Brent's (@bchristi-git) effort to
convert finalizers
> in the JDK to Cleaners. This is related to discovering coding patterns
for how to do this effectively. For example, the entire object's state is
available
> to the finalizer. But in order to convert this to a Cleaner, the state
available to the cleaning action needs to be refactored into a separate
object from the
> "outer" object. The `EnumCtx` nested class serves this role here.
>
> Second, we're trying to address the reachability issues. You (Hans) have
been writing and speaking about this for some time, mostly in the context of
> finalization. We in the JDK group haven't prioritized fixing this issue
with respect to finalization (since it's old and deprecated and nobody
should be
> using it, yeah right). Now that we're converting things to use Cleaner,
which has the same issues, we're forced to confront them. Our working
position
> is that there needs to be a reachabilityFence within a try/finally block
in the "right" places; determining the definition of "right" is one of the
goals here.
Fully agreed. reachabilityFence use is really orthogonal to the mechanism
used for the cleanup action. It also exists for plain References.
Technically,
you don't even need ReferenceQueues to trigger the problem; there are
probably cases in which WeakReferences are polled such that you need
reachabilityFence calls.

>
> The third issue is memory ordering. For finalization-based objects, the
JLS specifies a happens-before edge between the constructor and the
> finalizer. (I think this works for objects whose finalizable state is
fully initialized in the constructor. But it doesn't work for objects, like
this one,
> whose finalizable state is mutable throughout the lifetime of the object.)
My recollection is that was added as a very minimal guarantee, that was not
intended to suffice in general. Even if the state is fully initialized
in the constructor, things will usually still break if the final method
call that uses the state does not happen before the execution of the
Cleaner.

> There is nothing specified for memory ordering edges for Cleaner or
> java.lang.ref references at all that I can see. Given the lack of such
specifications, we're faced with using the right low-level memory ordering
> mechanisms to get the memory order we require. We're using
VarHandle::fullFence as kind of a placeholder for this. (We've also
considered
> empty synchronized blocks, writes/reads to "junk" variables created
expressly for this purpose, and other VarHandle fence operations,
> but none seem obviously better. I'm sure there are other alternatives we
haven't considered.) I'd welcome discussion of the proper alternative.
I think the intent was that JLS 12.6.2 still applies, though that section
was always stated in terms of finalizers. And in the absence of
reachabilityFence, it's rather weak anyway.

>
> The fourth issue is, do we really want every programmer who uses Cleaner
to have to figure out all this reachabilityFence and VarHandle fence stuff?
> Of course not. It would be nice to have some higher-level construct (such
as a language change like the "Reachability Revisited" proposal), or
possibly
> to create some library APIs to facilitate this. At a minimum, I think we
need to adjust various specifications like Cleaner and java.lang.ref to
> address memory ordering issues. There is certainly more that needs to be
done though.
I clearly agree.

>
> The difficulty with trying to come up with language changes or library
APIs is that I don't think we understand this problem well enough to define
> what those mechanisms are actually supposed to do. So before we get to
that point, I think we should see attempt to write down a correct solution
> using the existing reachability and memory ordering mechanisms. That's
what Brent has done here. We should review and iterate on this and identify
> the places where the specs need to change, and arrive at a point where
the we believe the code, even if ugly and inconvenient, is correct under
that spec.
I'm not convinced that's feasible without some spec clarification.
Otherwise I would agree.

The reachabilityFence spec currently says: "the referenced object is not
reclaimable by garbage collection at least until after the invocation of
this method."

In my opinion, the only meaningful interpretation of this is that the
reachabilityFence call happens before any Reference to the argument object
is enqueued.
Preventing "garbage collection" per se isn't ever a goal. (And in the case
of finalizers, it happens much later than finalization,
so it's not the right notion anyway.) And the only sense of "after" that
makes sense in such contexts is the inverse of the happens-before relation.
In retrospect, this interpretation is definitely a stretch, and the spec
should be much clearer. And I suspect that if we had a clearer statement to
this
effect we might largely be able to get rid of 12.6.2, which would be a huge
win.

It sounds like you're applying an alternative reading that largely ignores
the clause I quoted, and simply goes by "Ensures that the object referenced
by
the given reference remains strongly reachable". Presumably this implies,
via the Reference spec, that References are not enqueued until later. Even
there, it's unclear to me what "later" means, except in terms of
happens-before.

AFAICT, implementations actually do comply with my reading, though an
OpenJDK expert should confirm. We won't enqueue a Reference to x, where
x was last referenced by reachabilityFence(x) in thread T, unless there is
an intervening GC that result in T at a safe-point past the
reachabilityFence.
The safepoint synchronization must ensure that the safepoint happens-before
the GC decision to enqueue, which will then happen-before the actual
enqueuing. Enforcing this ordering is cheap, and probably necessary for GC
correctness anyway. So in implementation terms, I think my interpretation
is safe.

In contrast, putting in explicit fullFences is clearly horribly expensive,
much more so than a reachabilityFences. And they only make sense if they
occur both at the use
sites and in the Cleaner. And without my "happens-before" interpretation,
we technically don't know that use site one happens before the one in the
Cleaner,
so I don't think we have a guarantee they help, except by an assumption
that also implies they're not needed.

>
> Maybe at that point we can contemplate a higher-level approach such as a
language mechanism or a library API (or both), but I don't think we're
quite there yet.
> Or maybe some alternative path forward might emerge.
We share that hope. None of the current options look beautiful to me
either. But I think it would also be useful to agree that reachabilityFence
implies memory ordering.
Even if we find a better mechanism for most case, I expect that
reachabilityFence will still make sense in corner cases.

Hans
>
> -------------
>
> 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/20220726/e034be9a/attachment-0001.htm>