compiletest remove_and_create_dir_all may need retries

[jieyouxu]

cf. #134351. The current impl of remove_and_create_dir_all silently ignores the remove_dir_all failure and tries to create_dir_all, which can fail if remove_dir_all fails and a directory already exists.

fn remove_and_create_dir_all(path: &Path) {
    let _ = fs::remove_dir_all(path);
    fs::create_dir_all(path).unwrap();
}

compiletest cannot paper over create_dir_all failures (esp. if due to the directory already existing) to avoid running tests against outdated/invalid artifacts. So maybe a retry mechanism is needed. However, this should not be spot-fixed, and instead the retry mechanism should be shared between bootstrap/compiletest, preferrably in build_helpers with its own tests.

[Shourya742]

@rustbot claim

[Shourya742]

Hello @jieyouxu, Could you elaborate on the retry mechanism shared between bootstrap and compiletest? Are you suggesting creating a wrapper around methods that retries them a specific number of times until they return an Ok result, or should it simply return the output of the last attempt?
For example, are you proposing something like this?

pub fn retry_io<T, F>(func: F) -> io::Result<T>  
where  
    F: Fn() -> io::Result<T>,  
{  
    let mut attempt = 0;  
    loop {  
        match func() {  
            Ok(result) => return Ok(result),  
            Err(e) if attempt < 5 => {  
                attempt += 1;  
                thread::sleep(Duration::from_millis(10));  
            }  
            Err(e) => return Err(e),  
        }  
    }  
}

Or is the retry mechanism meant to be specific to remove_and_create_dir_all, implemented in build_helper like this?

pub fn remove_and_create_dir_all<P: AsRef<Path>>(path: P) -> io::Result<()> {  
    let path = path.as_ref();  
    for attempt in 0..5 {  
        if fs::remove_dir_all(path).is_ok() || !path.exists() {  
            return fs::create_dir_all(path);  
        }  
        std::thread::sleep(std::time::Duration::from_millis(60)));  
    }  
    fs::create_dir_all(path)  
}

Or both are wrong interpretation?

[jieyouxu]

• I was thinking sth like retrying remove_dir_all a few times but only if the remove_dir_all attempts don't error in NotFound.
• I said build_helper because build_helper can be shared between bootstrap and compiletest.
• I would not recommend retrying any I/O operations, but only for sth like remove_and_create_dir_all. Or remove_dir_all-ish cases. I would expect the outcome of the final attempt to be the final return value of this helper.
• The implementation needs to be careful to not traverse symlinks.

See also

rust/src/build_helper/src/fs/mod.rs

Line 37 in 00095b3

pub fn recursive_remove<P: AsRef<Path>>(path: P) -> io::Result<()> {

which doesn't currently perform retries.