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

pwalk, pwalkdir: fix walk vs remove race #204

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

kolyshkin
Copy link
Collaborator

In some cases, when file walk is racing with removal, the ENOENT is
received by filepath.Walk[Dir], rather than by the callback.

Do ignore such errors, except for the root directory (Walk argument).

Modify the doc accordingly, and add a couple of test cases.

The "Race" test case, when testing the code before the fix, fails 100%
of times for pwalk and 90% for pwalkdir. Alas, I was unable to make it
fail 100% of the times without significantly increasing the test
complexity.

Fixes: #199

@rhatdan
Copy link
Collaborator

rhatdan commented Aug 26, 2023

LGTM
This is one where the tests probably took 10 times as much time to write as the fix.
@thaJeztah PTAL

rhatdan
rhatdan previously approved these changes Aug 26, 2023
thaJeztah
thaJeztah previously approved these changes Aug 26, 2023
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM (nice!), but found 2 redundant defers, and left a suggestion.

don't think they're necessarily show-stoppers, but I'll let @kolyshkin have a look before merging 👍

pkg/pwalkdir/pwalkdir_test.go Outdated Show resolved Hide resolved
pkg/pwalkdir/pwalkdir_test.go Outdated Show resolved Hide resolved
pkg/pwalkdir/pwalkdir_test.go Show resolved Hide resolved
pkg/pwalk/pwalk_test.go Outdated Show resolved Hide resolved
pkg/pwalk/pwalk_test.go Show resolved Hide resolved
Use t.Helper and t.Cleanup in prepareTestSet to simplify tests setup and
cleanup (no need to call defer os.Remove or check error).

Signed-off-by: Kir Kolyshkin <[email protected]>
In some cases, when file walk is racing with removal, the ENOENT is
received by filepath.Walk[Dir], rather than by the callback.

Do ignore such errors, except for the root directory (Walk argument).

Modify the doc accordingly, and add a couple of test cases.

The "Race" test case, when testing the code before the fix, fails 100%
of times for pwalk and 90% for pwalkdir. Alas, I was unable to make it
fail 100% of the times without significantly increasing the test
complexity.

Fixes: 199

Signed-off-by: Kir Kolyshkin <[email protected]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@rhatdan PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: add error handling in pkg/pwalk
3 participants