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