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

[Peter Levart]

On Tue, 26 Jul 2022 05:26:31 GMT, 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.
> 
> 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.) 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 alternat
 ive.
> 
> 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.
> 
> 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.
> 
> 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.

Hi,

Sorry for not reading up to this message before starting reviewing the changes... As I was trying to explain in the comments, I don't think there is any issue about memory ordering here. As Cleaner is implemented currently in OpenJDK, there a 5 threads involved in a typical scenario of a cleanable object:
T1 - initializing thread that registers a Cleaner action
T2 - some thread that accesses/mutates object state including parts that are accessed/cleaned up by Cleaner action and guards against premature Cleaner action execution with reachability fence
T3 - GC thread that discovers a phantom-reachable referent of a PhantomReference that is tracked by the Cleaner and clears the PhantomReference and links it into a "pending" list.
T4 - a ReferenceHandler thread that waits for "pending" Reference(s) and enqueues them to ReferenceQueue(s)
T5 - a single Cleaner thread that dequeues PhantomReference(s) from the Cleaner's ReferenceQueue and executes cleanup actions

There is a synchronization action between T1 and T5, so there is no doubt that writes in the construrctor of a tracked object that typically preced Cleaner registration are visible to Cleaner action.

There is a synchronization action between T3 and T4 and there is a synchronization action between T4 and T5 (both can be verified in code). The only question here remaining is whether there is synchronization between threads that mutate object state and GC thread that discovers phantom-reachable referents..

I'm not qualified to answer that question. Especially with the advent of new fully concurrent GC algorithms such as ZGC and Shenandoah. So perhaps this is a question for GC experts.

-------------

PR: https://git.openjdk.org/jdk/pull/8311