Do not block when handling dir containing a named pipe#83
Conversation
|
Thank you for your PR! Would you also be able to add a test that checks for this? |
|
@XAMPPRocky I have reused the existing tests for this purpose, you can see the test setup in |
The last case would always return None since we already checked whether this is a directory. In case we were handling a file and no error (with the exception of is_eloop) occurred, set the file handle to None. For clarity, the file handle variable has been renamed to child_directory.
| 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)?) |
There was a problem hiding this comment.
This approach is doubling the number of syscalls - we're opening as a file then reopening again as a directory.
There was a problem hiding this comment.
... is it open_dir_at that is blocking?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
@angxiang Friendly ping :) No rush just thought you might like the reminder.
There was a problem hiding this comment.
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?
3 participants
This PR aims to address the issue described in Issue #82 .