-
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
Confusion on contents of saved Graph.adjacentNodes(n)
if n
is eventually removed
#6478
Comments
It seems to me that the set view of adjacent nodes of a node that was since removed from the graph is invalid, and its behavior could be undefined. It might be nice to invalidate that set-view, such that all calls throw |
That is an interesting one, for sure. Logically, the behavior is undefined (that is, we've never formally defined that behavior and I think that there are a couple of reasonable choices we could make, including both throwing ISE and returning the empty set), but we've never formally declared it to be undefined. We might consider that solution, although it's not very satisfying. As a practical matter, I am surprised that Since I don't have the bandwidth to look into this right now, so if you'd like to take a look we'd appreciate your insights. :) |
@jrtom Sure! I've just had a look, and I think it's breaking down somewhere at the end of I've observed that when the respective line of code is run... nodeConnections.remove(node); ...then that is when subsequent calls to I don't know how to achieve this yet, as I'd need to understand how |
@jbduncan Thanks for taking a look. I think that where you started to look is the right place to start. That said, note that line 139 (same file, a few lines previous) is a loop that removes all the predecessors* from the node to be removed, and that's the operation that I would have expected to clean up those relationships. In other words, before we remove the node, we remove all the incident edges...which should mean that any views that we have of the predecessors/successors of that node should be empty. So it appears that the view we're constructing is not as transparent as it should be. It might be illuminating to try that same test with
That might help to narrow down the source of the problem. *The subsequent loop removes all the successors, for directed graphs. |
I've delved into this matter and might have reached the bottom of it. Please find my findings described in comment and code in this PR: #6553. In short: the confusion arises because the |
@ineuwirth Wow, thank you very much for looking into this, much appreciated! I'll leave it to the Guava maintainers to decide the next steps, but in my experience this sort of bug fix is usually accepted after being reworked a bit internally at Google, so fingers crossed. 🤞 |
I recently found myself writing this code.
I was surprised by the contents of
adjacentNodesView
at the end, because ifgraph.edges()
is cleared, then I intuitively expected the adjacent nodes to be cleared too.But then I thought about it some more and realised this is a weird grey area, because if a user calls
graph.adjacentNodes(1)
again, then an IAE is rightfully thrown to prevent bugs.I'm torn between
adjacentNodesView
being[]
or[2]
in this case.@jrtom, what do you think?
The text was updated successfully, but these errors were encountered: