Skip to content

Do not block when handling dir containing a named pipe #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Do not block when handling dir containing a named pipe #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c4ac6b
Do not block when handling dir containing a named pipe
angxiang Feb 20, 2025
566eeda
Fix comments in scan_and_remove_entry_recursively
angxiang Feb 21, 2025
102674b
Remove unnecessary code
angxiang Feb 21, 2025
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
42 changes: 21 additions & 21 deletions src/_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,29 +178,29 @@ fn scan_and_remove_entry_recursively<I: io::Io>(
opts.read(true)
.write(fs_at::OpenOptionsWriteMode::Write)
.follow(false);
let child_result = opts.open_dir_at(dirfd, name);
let is_dir = match child_result {
// We expect is_eloop to be the only error
Err(e) if !I::is_eloop(&e) => return Err(e),
Err(_) => false,
Ok(child_file) => {
let metadata = child_file.metadata()?;
let is_dir = metadata.is_dir();
if is_dir {
remove_dir_contents_recursive::<I>(child_file, &dir_debug_root, parallel)?;
#[cfg(feature = "log")]
log::trace!("rmdir: {}", &dir_debug_root);
let opts = fs_at::OpenOptions::default();
opts.rmdir_at(dirfd, name).map_err(|e| {
#[cfg(feature = "log")]
log::debug!("error removing {}", dir_debug_root);
e
})?;
}
is_dir
let child_result = opts.open_at(dirfd, name);
let child_directory = match child_result {
// If we get EISDIR, we open it as a directory
Err(e) if e.raw_os_error() == Some(libc::EISDIR) => {
Some(opts.open_dir_at(dirfd, name)?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This approach is doubling the number of syscalls - we're opening as a file then reopening again as a directory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

... is it open_dir_at that is blocking?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, open_dir_at always passes the O_RDONLY flag, which blocks in the case of a named pipe (until it is opened for writing). open_at gets the correct flags, but throws EISDIR in case of a directory, which is why there is a second syscall for opening directories specifically.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will need to look at this somewhat more closely; if we can avoid the extra syscall that would be much better - this change would double the number of opens occuring.

Copy link
Author

Choose a reason for hiding this comment

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

That sounds good, thank you for looking into it! Apologies for slow replies, I am currently on vacation for another 10 days, I will try and check this thread when I can 👍

Copy link
Owner

Choose a reason for hiding this comment

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

@angxiang Friendly ping :) No rush just thought you might like the reminder.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, thanks for the ping! Apologies for the late reply, I had misinterpreted your message and was waiting on an update in this thread 😄

One option that could work is if the original code is used, but with an added O_NONBLOCK flag. However I am unsure what the possible side effects of this could be.

If that is not viable, then I don't know how to avoid a second syscall since passing the flags as they are does not work with directories, a special handling is needed. I will say that the doubling of syscalls is "only" a worst-case scenario, since this is specific to directories. If the expected contents of the dir to be removed is mostly non-directory files, then the amount of syscalls between the master-state code and this proposed change is comparable.
What do you think?

}
// The below options cover files that are not directories
// We expect is_eloop to be the only other error
Err(e) if !I::is_eloop(&e) => return Err(e),
Err(_) => None,
Ok(_) => None,
};
if !is_dir {
if let Some(child_directory) = child_directory {
remove_dir_contents_recursive::<I>(child_directory, &dir_debug_root, parallel)?;
#[cfg(feature = "log")]
log::trace!("rmdir: {}", &dir_debug_root);
let opts = fs_at::OpenOptions::default();
opts.rmdir_at(dirfd, name).map_err(|e| {
#[cfg(feature = "log")]
log::debug!("error removing {}", dir_debug_root);
e
})?;
} else {
#[cfg(feature = "log")]
log::trace!("unlink: {}", &dir_debug_root);
opts.unlink_at(dirfd, name).map_err(|e| {
Expand Down
9 changes: 9 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,10 +341,19 @@ mod tests {
let ours = tmp.path().join("t.mkdir");
let file = ours.join("file");
let nested = ours.join("another_dir");
let fifo = ours.join("fifo");
fs::create_dir(&ours)?;
fs::create_dir(nested)?;
File::create(&file)?;
File::open(&file)?;
#[cfg(unix)]
{
unsafe {
use std::ffi::CString;
let fifo_cstr = CString::new(fifo.to_str().unwrap()).unwrap();
libc::mkfifo(fifo_cstr.as_ptr(), 0o644);
};
}
Ok(Prep {
_tmp: tmp,
ours,
Expand Down