Implement -ignore_readdir_race and -noignore_readdir_race.#411
Implement -ignore_readdir_race and -noignore_readdir_race.#411hanbings wants to merge 7 commits intouutils:mainfrom
-ignore_readdir_race and -noignore_readdir_race.#411Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
==========================================
+ Coverage 66.25% 66.29% +0.03%
==========================================
Files 34 34
Lines 4004 4017 +13
Branches 911 911
==========================================
+ Hits 2653 2663 +10
- Misses 996 998 +2
- Partials 355 356 +1 ☔ View full report in Codecov by Sentry. |
|
Commit f4bb537 has GNU testsuite comparison: |
|
could you please document the fact that this option doesn't do anything ? |
Sorry for the late reply. In short, taking the test https://github.com/tavianator/bfs/blob/main/tests/gnu/ignore_readdir_race.sh as an example, during the execution of Wirte upI usually start by checking the GNU tests and the BFS tests for this parameter.
cd "$TEST"
"$XTOUCH" foo bar
# -links 1 forces a stat() call, which will fail for the second file
invoke_bfs . -mindepth 1 -ignore_readdir_race -links 1 -exec "$TESTS/remove-sibling.sh" {} \;
#!/usr/bin/env bash
for file in "${1%/*}"/*; do
if [ "$file" != "$1" ]; then
rm "$file"
exit $?
fi
done
exit 1I then ran the script on my local machine: Then I realized that our implementation didn't seem to have this problem in the same test. First we jump here from the main function, then turns to the parser fn do_find<'a>(args: &[&str], deps: &'a dyn Dependencies<'a>) -> Result<u64, Box<dyn Error>> {
let paths_and_matcher = parse_args(args)?;
...
}After getting all the matchers that meet the parameters, use found_count += process_dir(
&path,
&paths_and_matcher.config,
deps,
&*paths_and_matcher.matcher,
&mut quit,
);Here, while let Some(result) = it.next() {
match result {
Err(err) => {
uucore::error::set_exit_code(1);
writeln!(&mut stderr(), "Error: {dir}: {err}").unwrap()
}
Ok(entry) => {
let mut matcher_io = matchers::MatcherIO::new(deps);
if matcher.matches(&entry, &mut matcher_io) {
found_count += 1;
}
...
}
}
}Finally, there is the part of ...
match command.status() {
Ok(status) => status.success(),
Err(e) => {
writeln!(&mut stderr(), "Failed to run {}: {}", self.executable, e).unwrap();
false
}
}
...Therefore, in the case of |
|
oh, sorry, i just meant "in the code. explaining why it isn't doing anything" :) |
Ok, I've submitted the description in my latest commit. |
|
Commit 7413637 has GNU testsuite comparison: |
|
Commit c9a36d0 has GNU testsuite comparison: |
|
Commit 9250adc has GNU testsuite comparison: |
|
needs rebase |
| } | ||
| // In our implementation, including the `-exec` parameter, | ||
| // it is always run in a single thread. | ||
| // Therefore, there is no race condition for now. |
There was a problem hiding this comment.
The "race condition" that this flag is talking about is another command simultaneously modifying the directory while find is reading it, something like
tavianator@graphene $ touch {1..10000}
tavianator@graphene $ find -size 1 & rm ./*
[1] 65991
find: ‘./10000’: No such file or directory
[1] + 65991 exit 1 find -size 1
tavianator@graphene $ touch {1..10000}
tavianator@graphene $ find -ignore_readdir_race -size 1 & rm ./*
[1] 66034
[1] + 66034 done find -ignore_readdir_race -size 1
There was a problem hiding this comment.
Sorry, I forgot about this. The race condition does happen when deleting a large number of files, and I'm rewriting a piece of code that implements this feature.
There was a problem hiding this comment.
Terribly sorry, I did misunderstand the meaning of readdir_race.
I rechecked the code and read the GNU documentation. The -ignore_readdir_race parameter requires a way to suppress file not found errors globally in the software, so I need to complete #15 to centralize the error handling logic before I can return here.
I will finish them as soon as possible. :)
|
Commit 2184e7c has GNU testsuite comparison: |
|
needs rebasing |
|
@hanbings do you have an update on this ? thanks :) |
|
@hanbings any plan to finish it ? thanks :) |
3 participants
This PR will pass the test about race conditions in BFS test. Our implementation does not seem to cause race conditions, so only add
Configrelated to-ignore_readdir_raceand-noignore_readdir_racewithout making other logical changes.Closes #377