findutils icon indicating copy to clipboard operation
findutils copied to clipboard

Implement `-ignore_readdir_race` and `-noignore_readdir_race`.

Open hanbings opened this issue 1 year ago • 15 comments

This PR will pass the test about race conditions in BFS test. Our implementation does not seem to cause race conditions, so only add Config related to -ignore_readdir_race and -noignore_readdir_race without making other logical changes.

Closes #377

hanbings avatar Jul 04 '24 17:07 hanbings

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.29%. Comparing base (5be00c8) to head (2184e7c).

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 04 '24 17:07 codecov[bot]

Commit f4bb537b5511117023412c09cf7ffa57a25e74b2 has GNU testsuite comparison:

Run GNU findutils tests: GNU tests summary = TOTAL: 702 / PASS: 445 / FAIL: 254 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0 
Run GNU findutils tests: Gnu tests No changes
Run BFS tests: Changes from main: PASS +2 / SKIP +0 / FAIL -2
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 192 / SKIP: 1 / FAIL: 95

github-actions[bot] avatar Jul 04 '24 17:07 github-actions[bot]

could you please document the fact that this option doesn't do anything ?

sylvestre avatar Jul 04 '24 18:07 sylvestre

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 -exec, our main thread will always be blocked by the -exec executor, so there will be no race condition. But I guess this problem may not appear until https://github.com/uutils/findutils/issues/6 is implemented or https://github.com/uutils/findutils/issues/366 is fixed.

Wirte up

I usually start by checking the GNU tests and the BFS tests for this parameter. The test project for -ignore_readdir_race is at https://github.com/tavianator/bfs/blob/main/tests/gnu/ignore_readdir_race.sh

ignore_readdir_race.sh

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" {} \;

remove-sibling.sh

#!/usr/bin/env bash

for file in "${1%/*}"/*; do
    if [ "$file" != "$1" ]; then
        rm "$file"
        exit $?
    fi
done

exit 1

I then ran the script on my local machine:

/tmp/test_dir$ touch foo bar
# and copy remove-sibling.sh
/tmp/test_dir$ ls
foo bar remove-sibling.sh

# GNU find
/tmp/test_dir$ find . -mindepth 1 -links 1 -exec "./remove-sibling.sh" {} \;
find: ‘./bar’: No such file or directory
/tmp/test_dir$ ls
remove-sibling.sh

# uutils/find
/tmp/test_dir$ touch foo bar

/tmp/test_dir$ ls
foo bar remove-sibling.sh

/tmp/test_dir$ ... findutils/target/debug/find . -mindepth 1 -links 1 -exec "./remove-sibling.sh" {} \;
/tmp/test_dir$ ls
remove-sibling.sh

Then I realized that our implementation didn't seem to have this problem in the same test. OK, so let's look at the code.

First we jump here from the main function, then turns to the parser parse_args which parses the arguments. https://github.com/uutils/findutils/blob/main/src/find/mod.rs#L176

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 process_dir to traverse the directory. https://github.com/uutils/findutils/blob/main/src/find/mod.rs#L189

found_count += process_dir(
    &path,
    &paths_and_matcher.config,
    deps,
    &*paths_and_matcher.matcher,
    &mut quit,
);

Here, matcher.matches is executed once for each directory entry. So far, everything has been executed linearly, and no parallel processing has occurred. https://github.com/uutils/findutils/blob/main/src/find/mod.rs#L159

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 exec.rs that executes the instructions. When executing a command, we must wait for the command to complete before returning a result of whether the execution is completed (of course, an error message is output on the screen here.) https://github.com/uutils/findutils/blob/main/src/find/matchers/exec.rs#L87

       ...
       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 -exec, we always complete all work in sequence within one thread, and there will be no situation where instructions are not completed but uutils/findutils is still traversing.

hanbings avatar Jul 05 '24 10:07 hanbings

oh, sorry, i just meant "in the code. explaining why it isn't doing anything" :)

sylvestre avatar Jul 05 '24 12:07 sylvestre

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.

hanbings avatar Jul 06 '24 05:07 hanbings

Commit 74136375caf31aaee155c1a1abe487d821a96b74 has GNU testsuite comparison:

Run GNU findutils tests: GNU tests summary = TOTAL: 702 / PASS: 445 / FAIL: 254 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0 
Run GNU findutils tests: Gnu tests No changes
Run BFS tests: Changes from main: PASS +2 / SKIP +0 / FAIL -2
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 192 / SKIP: 1 / FAIL: 95

github-actions[bot] avatar Jul 06 '24 05:07 github-actions[bot]

Commit c9a36d00dcb5188c8b9c0261b7a9c4ad08abcfe7 has GNU testsuite comparison:

Run BFS tests: Changes from main: PASS +2 / SKIP +0 / FAIL -2
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 192 / SKIP: 1 / FAIL: 95
Run GNU findutils tests: GNU tests summary = TOTAL: 702 / PASS: 445 / FAIL: 254 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0 
Run GNU findutils tests: Gnu tests No changes

github-actions[bot] avatar Jul 06 '24 05:07 github-actions[bot]

Commit 9250adc7898307b49124ffb6811ee905bba93cef has GNU testsuite comparison:

Run GNU findutils tests: GNU tests summary = TOTAL: 702 / PASS: 445 / FAIL: 254 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0 
Run GNU findutils tests: Gnu tests No changes
Run BFS tests: Changes from main: PASS +2 / SKIP +0 / FAIL -2
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 192 / SKIP: 1 / FAIL: 95

github-actions[bot] avatar Jul 08 '24 12:07 github-actions[bot]

needs rebase

sylvestre avatar Jul 10 '24 08:07 sylvestre

Commit 2184e7c3a0d495237fbced271af49cf53a57f1b2 has GNU testsuite comparison:

Run BFS tests: Changes from main: PASS +2 / SKIP +0 / FAIL -2
Run BFS tests: BFS tests summary = TOTAL: 288 / PASS: 195 / SKIP: 1 / FAIL: 92
Run GNU findutils tests: GNU tests summary = TOTAL: 702 / PASS: 445 / FAIL: 254 / ERROR: 2
Run GNU findutils tests: Changes from main: PASS +0 / FAIL +0 / ERROR +0 / SKIP +0 
Run GNU findutils tests: Gnu tests No changes

github-actions[bot] avatar Jul 31 '24 13:07 github-actions[bot]

needs rebasing

sylvestre avatar Aug 17 '24 07:08 sylvestre

@hanbings do you have an update on this ? thanks :)

sylvestre avatar Sep 05 '24 08:09 sylvestre

@hanbings do you have an update on this ? thanks :)

There is nothing new here yet, and completing this PR requires completing the global error handling associated with this feature (which is https://github.com/uutils/findutils/issues/15). Before that I would like to complete https://github.com/uutils/findutils/pull/369 first. :)

hanbings avatar Sep 09 '24 16:09 hanbings

@hanbings any plan to finish it ? thanks :)

sylvestre avatar Apr 04 '25 07:04 sylvestre