On Fri, 22 Jul 2022 20:51:59 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: > > remove some more tabs 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 alternativ e. 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. ------------- PR: https://git.openjdk.org/jdk/pull/8311
