Skip to content
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

Stack overflow on circular dependency #9073

Closed
thibault-martinez opened this issue Jan 13, 2021 · 1 comment · Fixed by #9075
Closed

Stack overflow on circular dependency #9073

thibault-martinez opened this issue Jan 13, 2021 · 1 comment · Fixed by #9075
Labels
C-bug Category: bug

Comments

@thibault-martinez
Copy link

Problem

cargo check --release crashes with stack overflow.

thread 'main' has overflowed its stack
fatal runtime error: stack overflow

Steps

  1. git clone https://github.com/thibault-martinez/bee.git
  2. cd bee/
  3. git checkout cargo-stack-overflow
  4. cargo check --release

Notes

cargo 1.49.0 (d00d64df9 2020-12-05)

There is a circular dependency bee-tangle->bee-snapshot->bee-ledger->bee-tangle.

@thibault-martinez thibault-martinez added the C-bug Category: bug label Jan 13, 2021
@alexcrichton
Copy link
Member

Thanks for the report! This is a bug in the implementation we have that checks for cycles from the resolver --

fn check_cycles(resolve: &Resolve) -> CargoResult<()> {
// Sort packages to produce user friendly deterministic errors.
let mut all_packages: Vec<_> = resolve.iter().collect();
all_packages.sort_unstable();
let mut checked = HashSet::new();
let mut path = Vec::new();
let mut visited = HashSet::new();
for pkg in all_packages {
if !checked.contains(&pkg) {
visit(resolve, pkg, &mut visited, &mut path, &mut checked)?
}
}
return Ok(());
fn visit(
resolve: &Resolve,
id: PackageId,
visited: &mut HashSet<PackageId>,
path: &mut Vec<PackageId>,
checked: &mut HashSet<PackageId>,
) -> CargoResult<()> {
path.push(id);
// See if we visited ourselves
if !visited.insert(id) {
anyhow::bail!(
"cyclic package dependency: package `{}` depends on itself. Cycle:\n{}",
id,
errors::describe_path(&path.iter().rev().collect::<Vec<_>>()),
);
}
// If we've already checked this node no need to recurse again as we'll
// just conclude the same thing as last time, so we only execute the
// recursive step if we successfully insert into `checked`.
//
// Note that if we hit an intransitive dependency then we clear out the
// visitation list as we can't induce a cycle through transitive
// dependencies.
if checked.insert(id) {
let mut empty_set = HashSet::new();
let mut empty_vec = Vec::new();
for (dep, listings) in resolve.deps_not_replaced(id) {
let is_transitive = listings.iter().any(|d| d.is_transitive());
let (visited, path) = if is_transitive {
(&mut *visited, &mut *path)
} else {
(&mut empty_set, &mut empty_vec)
};
visit(resolve, dep, visited, path, checked)?;
if let Some(id) = resolve.replacement(dep) {
visit(resolve, id, visited, path, checked)?;
}
}
}
// Ok, we're done, no longer visiting our node any more
path.pop();
visited.remove(&id);
Ok(())
}
}
-- and has been there for a very long time. Turns out your project is structured just right (and named just right b/c we sort things) to trigger this!

I'll take a look at how we can fix our cycle detection.

bors added a commit that referenced this issue Jan 18, 2021
Fix a bug in Cargo's cyclic dep graph detection

Cargo's cyclic dependency graph detection turns out to have had a bug
for quite a long time as surfaced by #9073. The bug in Cargo has to do
with how dev-dependencies are handled. Cycles are "allowed" through
dev-dependencies because the dev-dependency can depend on the original
crate. Our cyclic graph detection, however, was too eagerly flagging a
package as known to not have a cycle before we had processed everything
about it.

The fix here was basically to just simplify the graph traversal. Instead
of traversing the raw `Resolve` this instead creates an alternate
in-memory graph which has the actual edges we care about for cycle
detection (e.g. every edge that wasn't induced via a dev-dependency).
With this simplified graph we then apply the exact same algorithm, but
this time it should be less buggy because we're not trying to do funky
things about switching sets about what's visited halfway through a
traversal.

Closes #9073
@bors bors closed this as completed in 4b4dc0a Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants