Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Fallback to synchronous rm_dir call if path moving fails #27306

Merged
xiangzhu70 merged 1 commit into from
Aug 23, 2022
Merged

Fallback to synchronous rm_dir call if path moving fails #27306

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions core/src/validator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2056,28 +2056,26 @@ fn move_and_async_delete_path(path: impl AsRef<Path> + Copy) {
));

if path_delete.exists() {
debug!("{} exists, delete it first.", path_delete.display());
std::fs::remove_dir_all(&path_delete).unwrap();
}

if !path.as_ref().exists() {
info!(
"move_and_async_delete_path: path {} does not exist",
path.as_ref().display()
);
return;
}

std::fs::rename(&path, &path_delete).unwrap();
if let Err(err) = std::fs::rename(&path, &path_delete) {
warn!(
"Path renaming failed: {}. Falling back to rm_dir in sync mode",
err.to_string()
);
std::fs::remove_dir_all(&path).unwrap();
return;
Copy link
Copy Markdown
Contributor

@mvines mvines Aug 22, 2022

Choose a reason for hiding this comment

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

seems like we can easily avoid this early return by using an } else {

Copy link
Copy Markdown
Contributor Author

@xiangzhu70 xiangzhu70 Aug 22, 2022

Choose a reason for hiding this comment

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

I usually let the the function do the early return for various exceptions, and let the later main part of the function represent the more likely flow, and avoid too much indention in that part. I can make the change if less early return is really preferred.

Copy link
Copy Markdown
Contributor

@mvines mvines Aug 23, 2022

Choose a reason for hiding this comment

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

My pref in this case would be to remove the early return but I'm not going to attempt to force compliance.

}

Builder::new()
.name("solDeletePath".to_string())
.spawn(move || {
std::fs::remove_dir_all(&path_delete).unwrap();
info!(
Copy link
Copy Markdown
Contributor

@mvines mvines Aug 22, 2022

Choose a reason for hiding this comment

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

I did find this log mesage useful fwiw

"Cleaning path {} done asynchronously in a spawned thread",
path_delete.display()
);
})
.unwrap();
}
Expand Down