-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Graph.adjacentNodes
more transparent when a node gets deleted.
#6553
Make Graph.adjacentNodes
more transparent when a node gets deleted.
#6553
Conversation
When a node in the graph is deleted, it can sometimes cause confusion with the view provided by the `Graph.adjacentNodes` function. This happens because the view might have been created before the node was removed. The removeNode function in `StandardMutableGraph` (which calls `StandardMutableValueGraph.removeNode`) meant to eliminate the connections related to the node. However, the `{Directed,Undirected}GraphConnections.adjacentNodeValues` associated with the to-be-removed node might still be referred to by the view constructed by `Graph.adjacentNodes` (which calls `StandardValueGraph.adjacentNodes`). So, this can result in indirect references to the values of adjacent nodes (from the perspective of the node that was removed), even though they've been deleted. This makes the view less transparent, because it's showing information that isn't accurate anymore after the node has been removed. The solution is to cleanup the associated `GraphConnections` before finally removing the node from the graph. Also it is now better aligned to the implementation of `StandardMutableNetwork.removeNode` reusing the same mechanism of copying the view to an immutable collection to avoid modifying the underlying view while iterating over it.
for (N successor : connections.successors()) { | ||
// Since views are returned, we need to copy the successors that will be removed. | ||
// Thus we avoid modifying the underlying view while iterating over it. | ||
for (N successor : ImmutableSet.copyOf(connections.successors())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ImmutableSet
here and not ImmutableList
? Seems unnecessary to deduplicate at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, connection.successors()
returns with Set<N>
, so I would not expect further deduplication at this point. I would be though concerned about the possible reordering what could happen through a hash-based set. (It might not cause issue during this cleanup, but it is not that trivial to prove.)
Anyway I borrowed this approach from StandardMutableNetwork.removeNode
where elements are indeed copied to ImmutableList
, probably I just unintentionally changed the type.
Thank you, I have fixed it.
Fixed review finding: the approach was borrowed from `StandardMutableNetwork.removeNode` where elements are copied to `ImmutableList`. Making the immutable collection list here too, so it is safer as no deduplication or reordering might happen.
It seems to me that the direction with throwing views probably requires a bigger rework in the |
// requireNonNull is safe because the node is a successor. | ||
requireNonNull(nodeConnections.getWithoutCaching(successor)).removePredecessor(node); | ||
requireNonNull(connections.removeSuccessor(successor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a while since I've looked at this code, but I'm a bit suspicious of the three different ways in which we're now accessing the connections of a node in this method:
- direct reference (or alias) for the internal data structure:
nodeConnections.get(node)
(viaconnections
, line 127;nodeConnections
is declared in the superclass asMapIteratorCache<N, GraphConnections<N, V>> nodeConnections
) - direct reference without caching:
nodeConnections.getWithoutCaching()
(line 144) - (immutable) copy of the output of that structure (line 142)
We should (a) validate that we want those three different mechanisms, or use only the ones we actually want, and (b) document which one(s) we do want, and why.
There's a lot of clever stuff going on in this code, which makes it easy to make mistakes in updating it.
(To be clear, I think you've explained why you want the copy of the set in the places that you've made it, but the reason why we're using each of the above two mechanisms in different places is less clear.)
Thank you very much for looking into this. :) This change looks fine (as far as it goes) if we assume that the correct behavior for a graph view that's been invalidated due to the removal of an argument to Since there's an ongoing discussion in #6554 of what the correct behavior should be (which I'm about to weigh into), I think we should hold onto this change for the moment; if we decide that "act as though the view is empty" is correct, then we should merge it. |
FYI, we are implementing your change internally; I goofed when attempting to do the merge (because the test code had changed) and it's easier to just do a clean version of it internally and then push that out than to fix your branch. The internal change is tagged with you (@ineuwirth) as the original author so you will continue to get credit for it. :) Note that we're still considering (per the discussion in #6554) whether we ultimately want to throw in this case rather than returning an empty set, but in the meantime, returning an empty set is definitely better than returning a set whose membership is clearly incorrect. |
When a node in the graph is deleted, it can sometimes cause confusion with the view provided by the
Graph.adjacentNodes
function. This happens because the view might have been created before the node was removed.The removeNode function in
StandardMutableGraph
(which callsStandardMutableValueGraph.removeNode
) meant to eliminate the connections related to the node. However, the{Directed,Undirected}GraphConnections.adjacentNodeValues
associated with the to-be-removed node might still be referred to by the view constructed byGraph.adjacentNodes
(which callsStandardValueGraph.adjacentNodes
).So, this can result in indirect references to the values of adjacent nodes (from the perspective of the node that was removed), even though they've been deleted. This makes the view less transparent, because it's showing information that isn't accurate anymore after the node has been removed.
The solution is to cleanup the associated
GraphConnections
before finally removing the node from the graph. Also it is now better aligned to the implementation ofStandardMutableNetwork.removeNode
reusing the same mechanism of copying the view to an immutable collection to avoid modifying the underlying view while iterating over it.