-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
Graphs: ensure and document consistent behavior for invalidated view-collections #6554
Comments
Same goes for directed graphs. |
Thanks for being eagle-eyed and reporting this. Although many Collection methods explicitly invite implementations to throw various unchecked exceptions, the reality is that any method can throw any unchecked exception at any time for any reason -- and in fact should if the alternative is to return a bogus value. Returning zero for So... it seems to me like what we should do is make sure that all the methods on view-collections in this circumstances do throw. And, we should document this situation much better in a couple ways. |
I am in strong agreement with the proposition to enhance the documentation. Specifically, what I found lacking in the Javadoc for
This phrasing could easily lead one to interpret that the method calculates the edges, places them into an immutable or unmodifiable container, and then returns them and that's all. I appreciate the fail-fast approach being adopted here. I'm curious, was this inspired by any lessons learned from using Multimap<String, String> multimap = HashMultimap.create();
multimap.put("N1", "N2");
Collection<String> viewOfN1 = multimap.get("N1");
multimap.keys().remove("N1");
assertThat(viewOfN1).isEmpty(); In this scenario, the view does not throw an exception if the element it was associated with no longer exists. I'm inclined to agree that employing the fail-fast approach and throwing exceptions when methods are called on a view that is no longer valid is likely the best solution. However, could we introduce a more specific exception type than |
I'm in agreement with your agreement :-)
That might have been a mistake. For one thing, the key can't get GC'd until that collection does, which is something the user would be unlikely to have thought about. I think we intentionally didn't want to repeat that for Graphs.
True point, this is what |
Thanks for your patience (I was out for a week with an injury and have only just now made the space to reply), and especially thanks for spotting and reporting this issue. That said, there are (I think) at least two different issues here which we should probably address individually, and perhaps in separate issues: Sets as views https://github.com/google/guava/wiki/GraphsExplained#accessor-behavior states that it is technically valid for an implementation of the
but goes on to say that the provided implementations use (2) (i.e., return unmodifiable views). This may be a case where we provided implementation flexibility that no one actually needs. OTOH, I suspect that if anyone did want to create their own implementation of these interfaces, they might well choose to go with (3) or (1); creating copies is less efficient (and often less convenient for the user), but it makes for a much simpler implementation. (This library's predecessor, JUNG used (3).) View invalidation Options in increasing order of implementation difficulty:
At this stage in Guava's development, I'm tempted to go with one of the first two options. Feedback/suggestions welcome. [0] Relevant methods [1] in all types:
Relevant methods,
[1] Very nitpicky related note: while
I don't think that there's anything to be done here; the (If you want to get even more of a headache, consider that we could invalidate a returned |
While I was investigating #6478, I encountered the following case that might be a bug:
In case of undirected graph the returning view of
Graph.incidentEdges
might not fully adhere the collection API contract as it can throw an exception it is not supposed to throw. This might happen asIncidentEdgeSet.size
callsBaseGraph.adjacentNodes
that throws IAE in case the node is no longer an element of the graph.The text was updated successfully, but these errors were encountered: