Be resilient to most IO error and filesystem loop while walking dirs by weihangl...

Conversation

Copy link

Contributor

weihanglo commented on Dec 18, 2021 •

edited

Let PathSource::walk be resilient to most IO errors and filesystem loop.

This PR also

Add a test validating the resilience against filesystem loop to prevent regression.
Emit warning when filesystem loop found while walking the filesystem. This is the only way I can think of now to solve #9528

Fixes #10213.

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

Copy link

Member

alexcrichton commented 28 days ago

Is there a way we could detect and filter these cases from walkdir, such as ignoring broken symlinks entirely?

Copy link

Contributor

Author

weihanglo commented 28 days ago

Indeed we can recover from broken symlink and some IO errors. For filesystem loop, I guess we can simply emit a warning inside fn walk and skip the entry? At least user stuck in root can be informed that a lot filesystem loops happened.

The code snippet need to change as below:

        for entry in walkdir {
            match entry {
                Ok(entry) => {
                    if !entry.file_type().is_dir() {
                        ret.push(entry.into_path());
                    }
                },
                Err(e) => {
                    if e.loop_ancestor().is_some() {
                        // emit some warning!!!!!
                        continue
                    }
                    if let Some(path) = e.path() {
                        ret.push(path.to_path_buf());
                    }
                }
            }
        }

Copy link

Member

alexcrichton commented 27 days ago

If a loop is as common as node_modules installations we probably don't want to warn about it but otherwise handling it somewhat gracefully seems reasonable.

weihanglo

changed the title Revert "Detect filesystem loop during walking the projects"

Be resilient to most IO error and filesystem loop while walking dirs

27 days ago

Copy link

Contributor

Author

weihanglo commented 26 days ago

From my own experience, a normal Node.js project should not have any nested node_modules in its dependencies, so the warning might be rare. I'm ok to remove the warning though if so, #9528 needs a reopen.

src/cargo/sources/path.rs

Outdated

(_, Some(err)) if err.kind() == PermissionDenied => {

return Err(Error::new(PermissionDenied, err.to_string()).into());

}

(Some(path), _) => ret.push(path.to_path_buf()),

I'm curious, what's going on here? This looks like it's ignoring all errors that are not permission denied?

Copy link

Contributor

Author

weihanglo 12 days ago

If cargo doesn't handle permission error intentionally, this EACCES test case might pass silently. The old behavior before adopting walkdir returns errors from fs::read_dir calls directly, as well as it just unwraps DirEntry without handling Result.

let mut entries: Vec<PathBuf> = fs::read_dir(path)

.with_context(|| format!("cannot read {:?}", path))?

.map(|e| e.unwrap().path())

.collect();

With walkdir, cargo cannot tell whether the error is permission error or a broken link. Generally, walkdir emits three kinds of errors:

Symlink loop errors: Cargo already takes care of it and shows a warning in this PR.
IO errors with a path: I guess cargo can recover from the path, and later let the caller decide to ignore it or if the caller does access the path and hits the error.
IO errros without a path: Cargo can simply propagate it to callers. This is rare because walkdir only emits this kind for either opening the file to check symlink loop or handling the Result<DirEntry> generated from read_dir().

I've updated this PR to reflect aforementioned change. There is an enhancement can do such as emitting warnings for those recoverable errors, but I doubt its usefulness.

Copy link

Member