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.

Sign up for GitHub

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

Jump to bottom

path/filepath, io/fs: undocumented behavior in WalkDir about the second call reporting the readdir failure #51617

Closed
liuycsd opened this issue Mar 11, 2022 · 4 comments
Closed

path/filepath, io/fs: undocumented behavior in WalkDir about the second call reporting the readdir failure #51617

liuycsd opened this issue Mar 11, 2022 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@liuycsd
Copy link

liuycsd commented Mar 11, 2022

With the new WalkDir API, if the readdir fails, the walkDirFn will be called for a second time, with the readdir error.

The doc says

// The error result returned by the function controls how Walk continues.
// If the function returns the special value SkipDir, Walk skips the
// current directory (path if info.IsDir() is true, otherwise path's
// parent directory). Otherwise, if the function returns a non-nil error,
// Walk stops entirely and returns that error.

If the walkDirFn do return a SkipDir for this second call, it would be returned(

go/src/path/filepath/path.go

Lines 394 to 402 in 7b1ba97

dirs, err := readDir(path)
if err != nil {
// Second call, to report ReadDir error.
err = walkDirFn(path, d, err)
if err != nil {
return err
}
}
) to the outer loop and break(

go/src/path/filepath/path.go

Lines 403 to 411 in 7b1ba97

for _, d1 := range dirs {
path1 := Join(path, d1.Name())
if err := walkDir(path1, d1, walkDirFn); err != nil {
if err == SkipDir {
break
}
return err
}
}
) it. So it would not cause Walk to stop walking the entire tree and not only skips the current directory, but also skips path's parent directory even though info.IsDir() is true.

And the doc also says

// The err argument reports an error related to path, signaling that Walk
// will not walk into that directory.

But there is a way letting it walk into the possible partial dentry list by returning nil from the walkDirFn.

These behaviors are different from the Walk func. Is it intentional for them to work like this? If it is, it would be better to have them documented out.
@ianlancetaylor
Copy link
Contributor

I think we should change the code so we ignore SkipDir from the second call. The current behavior doesn't make any sense. Want to send a patch?

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation labels Mar 11, 2022
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Mar 11, 2022
@liuycsd
Copy link
Author

liuycsd commented Mar 12, 2022

I think we should change the code so we ignore SkipDir from the second call. The current behavior doesn't make any sense. Want to send a patch?

Sorry, I'm not so familiar with the whole project.
I would appreciate it if someone wants to send a patch for this.

@ianlancetaylor
Copy link
Contributor

Thanks, I sent a patch.

@gopherbot
Copy link

Change https://go.dev/cl/392154 mentions this issue: io/fs, path/filepath: honor SkipDir on second WalkDirFunc error call

@golang golang locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@ianlancetaylor @liuycsd @gopherbot