fix(#2772): is_settled distinguishes "not finished" from "no such node"
Returning `false` for an unknown id gave the same answer as a node that is merely still running, so a caller polling a stale id would wait forever for a state that can never arrive. `Option<bool>` makes the two cases separate, matching `node()`'s convention that `None` means the id isn't in the graph.
This commit is contained in:
parent
4de5a8dd7c
commit
8c5de704be
2 changed files with 25 additions and 13 deletions
|
|
@ -480,7 +480,7 @@ impl JobQueue {
|
|||
inner
|
||||
.containers()
|
||||
.into_iter()
|
||||
.filter(|&c| !inner.sched.graph().is_settled(c))
|
||||
.filter(|&c| inner.sched.graph().is_settled(c) == Some(false))
|
||||
.count()
|
||||
}
|
||||
}
|
||||
|
|
@ -652,7 +652,7 @@ impl QueueInner {
|
|||
if !any_unsettled {
|
||||
return None;
|
||||
}
|
||||
let is_terminal = self.sched.graph().is_settled(container);
|
||||
let is_terminal = self.sched.graph().is_settled(container) == Some(true);
|
||||
Some(DagView {
|
||||
id: container.get(),
|
||||
source: meta.source,
|
||||
|
|
@ -695,7 +695,7 @@ impl QueueInner {
|
|||
let mut live: Vec<NodeId> = Vec::new();
|
||||
let mut terminal: Vec<(NodeId, i64)> = Vec::new();
|
||||
for c in self.containers() {
|
||||
if self.sched.graph().is_settled(c) {
|
||||
if self.sched.graph().is_settled(c) == Some(true) {
|
||||
terminal.push((c, self.dag_finished_at(c)));
|
||||
} else {
|
||||
live.push(c);
|
||||
|
|
|
|||
|
|
@ -488,13 +488,17 @@ impl<N, R> Graph<N, R> {
|
|||
self.nodes.iter().filter(|n| n.parent.is_none())
|
||||
}
|
||||
|
||||
/// Whether `id` has settled. A group root's state is its subtree's roll-up,
|
||||
/// so for a root this answers "is everything under it finished" — which is
|
||||
/// why callers don't scan the subtree themselves. Unknown ids are not
|
||||
/// settled.
|
||||
/// Whether `id` has settled, or `None` when there is no such node. A group
|
||||
/// root's state is its subtree's roll-up, so for a root this answers "is
|
||||
/// everything under it finished" — which is why callers don't scan the
|
||||
/// subtree themselves.
|
||||
///
|
||||
/// `None` rather than `false` for an unknown id: "this node is not finished"
|
||||
/// and "there is no such node" are different answers, and a caller that
|
||||
/// conflates them keeps polling an id that will never settle.
|
||||
#[must_use]
|
||||
pub fn is_settled(&self, id: NodeId) -> bool {
|
||||
self.node(id).is_some_and(|n| n.state.is_terminal())
|
||||
pub fn is_settled(&self, id: NodeId) -> Option<bool> {
|
||||
self.node(id).map(|n| n.state.is_terminal())
|
||||
}
|
||||
|
||||
/// Why `id`'s subtree failed: the error of the first `Failed` descendant
|
||||
|
|
@ -704,12 +708,20 @@ mod tests {
|
|||
fn is_settled_tracks_node_state() {
|
||||
let mut g: Graph<&str, String> = Graph::new();
|
||||
let n = g.insert("n", vec![], None).unwrap();
|
||||
assert!(!g.is_settled(n), "Pending is not settled");
|
||||
assert_eq!(g.is_settled(n), Some(false), "Pending is not settled");
|
||||
node_mut(&mut g, n).state = State::Finishing;
|
||||
assert!(!g.is_settled(n), "Finishing still has children running");
|
||||
assert_eq!(
|
||||
g.is_settled(n),
|
||||
Some(false),
|
||||
"Finishing still has children running"
|
||||
);
|
||||
node_mut(&mut g, n).state = State::Done;
|
||||
assert!(g.is_settled(n));
|
||||
assert!(!g.is_settled(NodeId(99)), "unknown id is not settled");
|
||||
assert_eq!(g.is_settled(n), Some(true));
|
||||
assert_eq!(
|
||||
g.is_settled(NodeId(99)),
|
||||
None,
|
||||
"an unknown id is not the same answer as `not settled`"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
Loading…
Reference in a new issue