8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner #8311
Conversation
|
|
|
@bchristi-git The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| /* This class maintains the pieces of state that need (or are needed for) | ||
| * cleanup, which happens by calling cleanup(), or is done by the Cleaner. | ||
| */ | ||
| private static class CleaningAction implements Runnable { |
There was a problem hiding this comment.
This nested class might be more aptly named EnumCtx since it is used during the enumeration process.
The mention of the cleanup() method in the nested class javadoc is a bit ambiguous, it seems there should be a cleanup() method in the class, but it is in AbstractLdapNamingEnumeration.
The state field might also be renamed enumCtx.
| this.homeCtx = homeCtx; | ||
| homeCtx.incEnumCount(); | ||
| enumClnt = homeCtx.clnt; // remember | ||
| this.state.homeCtx.incEnumCount(); |
There was a problem hiding this comment.
Readability might be improved by using the new homeCtx() method instead of state.homeCtx in this file.
It would then be consistent with how subclasses access it.
It depends which style you prefer to be more consistent with.
| @Override | ||
| public void run() { | ||
| if (enumClnt != null) { | ||
| enumClnt.clearSearchReply(res, homeCtx.reqCtls); |
There was a problem hiding this comment.
It's a bit strange to see that there is no guard here to verify that homeCtx != null, when line 76 implies that it might. My reading is that homeCtxt is not supposed to be null when enumClnt is not null. That could be explained in a comment, or alternatively asserted just before line 73 (assert homeCtx != null;)
There was a problem hiding this comment.
Yes, it is strange -- that code came from the finalizer. I will add an assert.
There was a problem hiding this comment.
It appears that the update() method can set 'homeCtx' for 'ne' to null while leaving 'enumClnt' non-null (~L410).
Perhaps here, the clearSearchReply() call should only happen if both are non-null.
| } | ||
| } | ||
|
|
||
| private CleaningAction state; |
There was a problem hiding this comment.
I wonder if state should be final?
There was a problem hiding this comment.
Makes sense to me. cleanable can be final, too.
…umeration for consistencty ; new instance vars can be final
|
Since synchronization may now happen on the cleaner thread, I've changed The added |
| try { | ||
| LdapResult newRes = homeCtx().getSearchReply(enumCtx.enumClnt, enumCtx.res); | ||
| enumCtx.setRes(newRes); | ||
| if (enumCtx.res == null) { |
There was a problem hiding this comment.
This looks odd, setting the value using synchronized, but reading it without.
There was a problem hiding this comment.
I've added getters to EnumCtx, and a comment explaining why only setters are synchronized.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
Show resolved
Hide resolved
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
Show resolved
Hide resolved
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
Show resolved
Hide resolved
|
Mailing list message from Hans Boehm on core-libs-dev: On Wed, Jun 1, 2022 at 2:47 PM Roger Riggs <rriggs at openjdk.java.net> wrote:
I agree that this is true with the right implementation assumptions, which tmp = a.field; Consider the full moon case. The reachabilityFence spec says: "the And indeed, a compiler could conceivably rewrite this to if (moon_phase() != FULL) { in which case this might, on very rare occasions, actually fail in the Hans |
|
Hans, thank for the detailed example. I had not fully considered the flow of control in the throwing case. |
trivial comments.
The commented out printf/println's should be removed before committing.
| homeCtx.incEnumCount(); | ||
| enumClnt = homeCtx.clnt; // remember | ||
| this.enumCtx.homeCtx.incEnumCount(); | ||
| this.cleanable = LDAP_CLEANER.register(this, enumCtx); |
There was a problem hiding this comment.
Use enumCtx.getHomeCts() above.
Use this.enumCtx in call to register.
For consistency.
|
@bchristi-git This change is no longer ready for integration - check the PR body for details. |
Do you mean the pre-existing |
|
|
@bchristi-git This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
I've updated how the reachability and memory visibility issues (per comment) are addressed: Additional reachability fences Unless it's immediately obvious that no cleanable state is accessed during the course of a method's execution, all methods should include a
Unless it's immediately obvious that no cleanable state is modified during the course of the method's execution, all methods should include a Other possibilities for triggering the necessary volatile operations could be:
One could perhaps use deep code tracing to determine that cleanable state is not accessed/written during the course of a method's execution, and omit fences based on that knowledge. However I believe that leaves too large a burden on future maintainers and reviewers to remember to re-perform the code tracing and re-confirm non-use of cleanable state, even when changing possibly "far away" code. |
LGTM
(Except for a TODO and some tabs that should be spaces)
(Probably there's a higher level lesson to be learned about designing this kind of interaction with a server and the teardown needed).
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
Outdated
Show resolved
Hide resolved
| // are reachability fences to ensure that the registered object remains | ||
| // reachable. | ||
| // TODO: Is anything else needed so that this constructor | ||
| // "happens-before" the cleaning action ? |
There was a problem hiding this comment.
I think the resolution of this TODO is to modify the specification of Cleaner to require that:
- the object being registered is guaranteed reachable until after registration has completed; and
- a "memory consistency effects" clause should also be added to Cleaner.register. For an example, see the class specification of CopyOnWriteArrayList.
It may be that reachabilityFences are already in the right place in Cleaner. Great. But last time I looked it doesn't have any memory visibility operations. I'd suggest opening a bug on Cleaner for the spec change and accumulating notes there.
|
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. :-) |
|
Hans Boehm wrote:
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 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 alternative. 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. |
| // overridden to access the cleanable state without the proper fences. | ||
|
|
||
| // Ensure writes are visible to the Cleaner thread | ||
| VarHandle.fullFence(); |
There was a problem hiding this comment.
I don't think any memory fences are needed here. Reachability fence is enough. Cleaner runs in its own thread by polling enqued PhantomReference(s) from the ReferenceQueue. There is a happens-before edge between ReferenceHandler thread enqueue-ing "pending" PhantomReferences and Cleaner thread dequeue-ing them. There is a happens-before edge between GC thread clearing a PhantomReference and linking it into a "pending" list and ReferenceHandler thread unlinking it from the "pending" list and enque-ing it into a ReferenceQueue. There is a happens-before edge before a call to Reference.reachabilityFence(referent) and a GC thread discovering a phantom-reachable referent and clearing the PhantomReference and linking it into a "pending" list.
So there you have a happens-before chain which makes all writes before a call to eference.reachabilityFence(this) visible to a Cleaner task that is initialized to observe reachabillity of this....
| // Same situation as nextElement() - see comment above | ||
|
|
||
| // Ensure writes are visible to the Cleaner thread | ||
| VarHandle.fullFence(); |
There was a problem hiding this comment.
Same as above. Memory fences are not needed.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
Show resolved
Hide resolved
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
Show resolved
Hide resolved
| } | ||
| } finally { | ||
| // Ensure writes are visible to the Cleaner thread | ||
| VarHandle.fullFence(); |
There was a problem hiding this comment.
The memory fence is not needed (as explained above), but...
| return (AbstractLdapNamingEnumeration<? extends NameClassPair>)refCtx.list(listArg); | ||
| } finally { | ||
| // Ensure writes are visible to the Cleaner thread | ||
| VarHandle.fullFence(); |
| return sr; | ||
| } finally { | ||
| // Ensure writes are visible to the Cleaner thread | ||
| VarHandle.fullFence(); |
There was a problem hiding this comment.
memory fence is not needed.
| searchArgs.name, searchArgs.filter, searchArgs.cons); | ||
| } finally { | ||
| // Ensure writes are visible to the Cleaner thread | ||
| VarHandle.fullFence(); |
| @Override | ||
| public void run() { | ||
| // Ensure changes on the main/program thread happens-before cleanup | ||
| VarHandle.fullFence(); |
There was a problem hiding this comment.
no need for memory fence as explained in various finally blocks...
| if (homeCtx != null) { | ||
| homeCtx.decEnumCount(); | ||
| homeCtx = null; | ||
| } |
There was a problem hiding this comment.
Cleaner's task (run() method) is guaranteed to be called at most once. So there's no need for above "idempotent" logic. Unless some of those fields are optional when the instance is constructed....
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: 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. |
|
...while waiting for a GC expert to confirm that writes, in program order preceding a call to reachability fence, are visibile to GC thread that discovers a phantom reachable referent, here is an example which illustrates that some writes at least "must" be visible for GC to function properly: var ar = new Object[1];
var e = new Object();
ar[0] = e;
Reference.reachabilityFence(e);
// GC kicks-in at this point
var e2 = ar[0];
// use e2A write to an array slot ar[0] must be visible to a GC thread when it searches for root(s) after the mutator thread's call to reachability fence. If it was not, it could miss that fact that object 'e' is still reachable. So GC must do something so that at least writes to reference variables are visible. If it does not distinguish reference writes from primitive writes, then it does the same for all writes. |
|
Hi Peter, thanks for contributing to this. I think you're observing that as a practical matter, the implementations of GC and of various libraries such as reference queues must see a consistent view of memory in order for anything to work. This seems right. However, I'm concerned about what the specification says, or ought to say. Hans seems to think that the JLS assertions about finalization also apply to reference processing. I'm not so sure... that seems to me to be a rather generous interpretation. On the face of it, I cannot find anything explicit in the specifications that supports it. It certainly seems reasonable (to me) that reachabilityFence(x) ought to HB the corresponding reference being dequeued. If so I would like the specification to say that. (The fact that there is a GC thread that enqueues the reference is part of the implementation, which is opaque to the specification.) It may also be the that any point where the referent is reachable HB its corresponding reference is dequeued. Clearly the GC implementation needs to make sure this is the case; the questions in my mind are whether, where, and how this should be specified! It may be that the VarHandle fences aren't necessary. However, if they end up driving the right updates to the specifications, they will have served their purpose. Setting this aside, it does seem like all uses of a cleanable object need to have a try/finally statement, with at least an RF in the finally clause. Is there any evidence that shows that this construct isn't needed? |
|
Mailing list message from Hans Boehm on core-libs-dev: I also have concerns about the use of fullFence here. I believe it should It appears to me that this is addressing an instance of the problem for On Fri, Jul 22, 2022 at 3:27 PM Stuart Marks <smarks at openjdk.org> wrote: -------------- next part -------------- |
|
Mailing list message from Hans Boehm on core-libs-dev: On Mon, Jul 25, 2022 at 10:30 PM Stuart Marks <smarks at openjdk.org> wrote:
should be the case that reachabilityFence guarantees visibility
the Cleaner associated with p. Mutator-collector
appears to me that the current reachabilityFence() documentation does not
which I suggested a more general, though also not
https://docs.google.com/document/d/1yMC4VzZobMQTrVAraMy7xBdWPCJ0p7G5r_43yaqsj-s/edit?usp=sharing
reachabilityFence and premature finalization, I don't recall seeing any
mention of them. (Of course it's possible I could have missed something.)
anything. However, as time goes on we've become more confident that there
to add some memory ordering semantics into reachabilityFence(p), as you
reachability fused with memory order. At least we should discuss them
background and where I think this ought to go.
changes. In this case it's part of Brent's (@bchristi-git) effort to
for how to do this effectively. For example, the entire object's state is
available to the cleaning action needs to be refactored into a separate
been writing and speaking about this for some time, mostly in the context of
with respect to finalization (since it's old and deprecated and nobody
which has the same issues, we're forced to confront them. Our working
in the "right" places; determining the definition of "right" is one of the
JLS specifies a happens-before edge between the constructor and the
fully initialized in the constructor. But it doesn't work for objects, like
My recollection is that was added as a very minimal guarantee, that was not
specifications, we're faced with using the right low-level memory ordering
VarHandle::fullFence as kind of a placeholder for this. (We've also
expressly for this purpose, and other VarHandle fence operations,
haven't considered.) I'd welcome discussion of the proper alternative.
to have to figure out all this reachabilityFence and VarHandle fence stuff?
as a language change like the "Reachability Revisited" proposal), or
need to adjust various specifications like Cleaner and java.lang.ref to
done though.
APIs is that I don't think we understand this problem well enough to define
that point, I think we should see attempt to write down a correct solution
what Brent has done here. We should review and iterate on this and identify
the we believe the code, even if ugly and inconvenient, is correct under The reachabilityFence spec currently says: "the referenced object is not In my opinion, the only meaningful interpretation of this is that the It sounds like you're applying an alternative reading that largely ignores AFAICT, implementations actually do comply with my reading, though an In contrast, putting in explicit fullFences is clearly horribly expensive,
language mechanism or a library API (or both), but I don't think we're
We share that hope. None of the current options look beautiful to me Hans |
|
Mailing list message from Hans Boehm on core-libs-dev: On Tue, Aug 2, 2022 at 7:43 PM Stuart Marks <smarks at openjdk.org> wrote:
I think the reachabilityFence is clearly needed with current I would also guess that the try-finally is required by some current Hans -------------- next part -------------- |
Please review this change to replace the finalizer in
AbstractLdapNamingEnumerationwith a Cleaner.The pieces of state required for cleanup (
LdapCtx homeCtx,LdapResult res, andLdapClient enumClnt) are moved to a static inner class . From there, the change is fairly mechanical.Details of note:
homeCtx; I added ahomeCtx()method to readhomeCtxfrom the superclass'sstate.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(notLdapNamingEnumerationorLdapBindingEnumeration, though all are subclasses ofAbstractLdapNamingEnumeration).Thanks.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8311/head:pull/8311$ git checkout pull/8311Update a local copy of the PR:
$ git checkout pull/8311$ git pull https://git.openjdk.org/jdk pull/8311/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8311View PR using the GUI difftool:
$ git pr show -t 8311Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8311.diff