Skip to content

Commit

Permalink
Render cyclic path in node error to avoid --print-exception-stacktrace (
Browse files Browse the repository at this point in the history 
pantsbuild#8422)

### Problem

See pantsbuild#5739: currently, rendering a cycle in the graph requires enabling `--print-exception-stacktrace`. But `--print-exception-stacktrace` is annoying for end users.

### Solution

Use the cyclic path that @illicitonion added in pantsbuild#7642 to render an error directly. And in a separate commit, remove `EntryKey`, which was only used for "recording" cycles in the `Graph`... from an era where we actually looked at the complete dump of the runtime graph, I think?

### Result

`--print-exception-stacktrace` is no longer necessary to see the "path" involved in a cycle. Fixes pantsbuild#5739.
  • Loading branch information
@stuhood
stuhood authored Oct 29, 2019
1 parent 4a337b5 commit 5750037
Show file tree
Hide file tree
Showing 6 changed files with 136 additions and 151 deletions.
164 changes: 67 additions & 97 deletions src/rust/engine/graph/src/entry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,26 +146,6 @@ impl<N: Node> EntryState<N> {
}
}

///
/// Because there are guaranteed to be more edges than nodes in Graphs, we mark cyclic
/// dependencies via a wrapper around the Node (rather than adding a byte to every
/// valid edge).
///
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub(crate) enum EntryKey<N: Node> {
Valid(N),
Cyclic(N),
}

impl<N: Node> EntryKey<N> {
pub(crate) fn content(&self) -> &N {
match self {
&EntryKey::Valid(ref v) => v,
&EntryKey::Cyclic(ref v) => v,
}
}
}

///
/// An Entry and its adjacencies.
///
Expand All @@ -174,7 +154,7 @@ pub struct Entry<N: Node> {
// TODO: This is a clone of the Node, which is also kept in the `nodes` map. It would be
// nice to avoid keeping two copies of each Node, but tracking references between the two
// maps is painful.
node: EntryKey<N>,
node: N,

state: Arc<Mutex<EntryState<N>>>,
}
Expand All @@ -185,15 +165,15 @@ impl<N: Node> Entry<N> {
/// the EntryId of an Entry until after it is stored in the Graph, and we need the EntryId
/// in order to run the Entry.
///
pub(crate) fn new(node: EntryKey<N>) -> Entry<N> {
pub(crate) fn new(node: N) -> Entry<N> {
Entry {
node: node,
state: Arc::new(Mutex::new(EntryState::initial())),
}
}

pub fn node(&self) -> &N {
self.node.content()
&self.node
}

///
Expand All @@ -216,7 +196,7 @@ impl<N: Node> Entry<N> {
///
pub(crate) fn run<C>(
context_factory: &C,
entry_key: &EntryKey<N>,
node: &N,
entry_id: EntryId,
run_token: RunToken,
generation: Generation,
Expand All @@ -228,77 +208,67 @@ impl<N: Node> Entry<N> {
{
// Increment the RunToken to uniquely identify this work.
let run_token = run_token.next();
match entry_key {
&EntryKey::Valid(ref n) => {
let context = context_factory.clone_for(entry_id);
let node = n.clone();

context_factory.spawn(future::lazy(move || {
// If we have previous result generations, compare them to all current dependency
// generations (which, if they are dirty, will cause recursive cleaning). If they
// match, we can consider the previous result value to be clean for reuse.
let was_clean = if let Some(previous_dep_generations) = previous_dep_generations {
let context2 = context.clone();
context
.graph()
.dep_generations(entry_id, &context)
.then(move |generation_res| match generation_res {
Ok(ref dep_generations) if dep_generations == &previous_dep_generations => {
// Dependencies have not changed: Node is clean.
Ok(true)
}
_ => {
// If dependency generations mismatched or failed to fetch, clear its
// dependencies and indicate that it should re-run.
context2.graph().clear_deps(entry_id, run_token);
Ok(false)
}
})
.to_boxed()
} else {
future::ok(false).to_boxed()
};

// If the Node was clean, complete it. Otherwise, re-run.
was_clean.and_then(move |was_clean| {
if was_clean {
// No dependencies have changed: we can complete the Node without changing its
// previous_result or generation.
context
.graph()
.complete(&context, entry_id, run_token, None);
future::ok(()).to_boxed()
} else {
// The Node needs to (re-)run!
let context2 = context.clone();
node
.run(context)
.then(move |res| {
context2
.graph()
.complete(&context2, entry_id, run_token, Some(res));
Ok(())
})
.to_boxed()
let context = context_factory.clone_for(entry_id);
let node = node.clone();

context_factory.spawn(future::lazy(move || {
// If we have previous result generations, compare them to all current dependency
// generations (which, if they are dirty, will cause recursive cleaning). If they
// match, we can consider the previous result value to be clean for reuse.
let was_clean = if let Some(previous_dep_generations) = previous_dep_generations {
let context2 = context.clone();
context
.graph()
.dep_generations(entry_id, &context)
.then(move |generation_res| match generation_res {
Ok(ref dep_generations) if dep_generations == &previous_dep_generations => {
// Dependencies have not changed: Node is clean.
Ok(true)
}
_ => {
// If dependency generations mismatched or failed to fetch, clear its
// dependencies and indicate that it should re-run.
context2.graph().clear_deps(entry_id, run_token);
Ok(false)
}
})
}));
.to_boxed()
} else {
future::ok(false).to_boxed()
};

EntryState::Running {
waiters: Vec::new(),
start_time: Instant::now(),
run_token,
generation,
previous_result,
dirty: false,
// If the Node was clean, complete it. Otherwise, re-run.
was_clean.and_then(move |was_clean| {
if was_clean {
// No dependencies have changed: we can complete the Node without changing its
// previous_result or generation.
context
.graph()
.complete(&context, entry_id, run_token, None);
future::ok(()).to_boxed()
} else {
// The Node needs to (re-)run!
let context2 = context.clone();
node
.run(context)
.then(move |res| {
context2
.graph()
.complete(&context2, entry_id, run_token, Some(res));
Ok(())
})
.to_boxed()
}
}
&EntryKey::Cyclic(_) => EntryState::Completed {
result: EntryResult::Clean(Err(N::Error::cyclic())),
dep_generations: Vec::new(),
run_token,
generation,
},
})
}));

EntryState::Running {
waiters: Vec::new(),
start_time: Instant::now(),
run_token,
generation,
previous_result,
dirty: false,
}
}

Expand Down Expand Up @@ -339,7 +309,7 @@ impl<N: Node> Entry<N> {
ref result,
generation,
..
} if self.node.content().cacheable() && !result.is_dirty() => {
} if self.node.cacheable() && !result.is_dirty() => {
return future::result(result.as_ref().clone())
.map(move |res| (res, generation))
.to_boxed();
Expand Down Expand Up @@ -374,10 +344,10 @@ impl<N: Node> Entry<N> {
"Re-starting node {:?}. It was: previous_result={:?}, cacheable={}",
self.node,
result,
self.node.content().cacheable()
self.node.cacheable()
);
assert!(
result.is_dirty() || !self.node.content().cacheable(),
result.is_dirty() || !self.node.cacheable(),
"A clean Node should not reach this point: {:?}",
result
);
Expand All @@ -394,12 +364,12 @@ impl<N: Node> Entry<N> {
entry_id,
run_token,
generation,
if self.node.content().cacheable() {
if self.node.cacheable() {
Some(dep_generations)
} else {
None
},
if self.node.content().cacheable() {
if self.node.cacheable() {
Some(result)
} else {
None
Expand Down Expand Up @@ -684,6 +654,6 @@ impl<N: Node> Entry<N> {
Some(Err(ref x)) => format!("{:?}", x),
None => "<None>".to_string(),
};
format!("{} == {}", self.node.content(), state).replace("\"", "\\\"")
format!("{} == {}", self.node, state).replace("\"", "\\\"")
}
}
Loading

0 comments on commit 5750037

Please sign in to comment.