net: add types for named Unix pipes#5351
Conversation
Remove integration with process Remove stdio tests Remove unnecessary imports
|
Thanks for the great work!
I would definitely love to see annoymous pipe support in tokio, if this PR is merged. Also, if you check the dependents of tokio-pipe, a crate for annoymous pipe only, you can find quite some crates dependent on it. tokio-pipe also provides splice/tee support in addition to write/read. |
|
It might be worthwhile to rethink whether Basic support for anonymous pipes probably boils down to just adding the fn new_pair() -> io::Result<(Sender, Receiver)>Additional support like splice/tee from tokio-pipe can be done later if needed. |
I agree, I think that's the right place for unix pipes. If someone want to provide portable named pipe over unix and windows named pipe, then can simply provide an abstraction in
That's probably enough, but would be great if we can also set flags such as
👍 |
|
I need to take some time to go through this. |
tokio/src/net/pipe.rs
Outdated
| /// The runtime is usually set implicitly when this function is called | ||
| /// from a future driven by a tokio runtime, otherwise runtime can be set | ||
| /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. | ||
| pub fn from_file(file: File) -> io::Result<Sender> { |
There was a problem hiding this comment.
I suggest that we could add some extra checks on whether it's writeable and actually a pipe fd here, similar to how tokio_pipe::PipeFd::from_raw_fd_checked performs the check.
There was a problem hiding this comment.
I'm not really sure that this is necessary.
There was a problem hiding this comment.
I'm not really sure that this is necessary.
While it's not absolutely necessary, checking it will rule out some stupid mistakes, e.g. user opened a regular file and passed it to Sender::from_file, or they opened the read end of a pipe and passed it here.
It's still runtime error, but better than no error.
There was a problem hiding this comment.
Also, I think it would be better to use OwnedFd here instead of File:
fn from_owned_fd(fd: Into<OwnedFd>) -> io::Result<Sender>;
since impl From<File> for OwnedFd makes it possible to create OwnedFd from File.
There are also From<ChildStdin>, From<ChildStdOut> and From<ChildStdErr> implementation, which would makes this function more useful.
There was a problem hiding this comment.
I think it would be best to do both File and OwnedFd separately. For now I don't think we can support OwnedFd though due to MSRV.
Regarding the check, I'm hesitant as I've seen plenty of weird cases before where people use different weird linux APIs and then open them as tokio types which use the same system calls under the hood for interaction. I could totally see people trying to use this to open different things that aren't strictly pipes but are readable or writable only. Do we want to disqualify this use case?
If we decide to do a check now, it will be a breaking change to remove it, so we need to think this through.
There was a problem hiding this comment.
I could totally see people trying to use this to open different things that aren't strictly pipes but are readable or writable only. Do we want to disqualify this use case?
They already have AsyncFd for that and tokio also provides unix socket, tcp/udp socket, etc.
I don't see why people would want to create Sender from File, since regular file are blocking, other special files mostly the same.
The only thing that I can think of is people trying to open a named unix socket, but it is already taken care of by UnixStream::connect, not to mentio that Sender does not provide several functions that is available in UnixStream and functionalities that will be included (e.g. send fd over unix socket).
There was a problem hiding this comment.
I think it would be best to do both File and OwnedFd separately. For now I don't think we can support OwnedFd though due to MSRV.
Oops, then it makes sense to have two separate functions.
It's a shame we can't just have one from_owned_fd function.
There was a problem hiding this comment.
The same argument about the extra checks can be made against open methods, since they also don't check if the file is a FIFO file. My problem with such validation is that it costs additional syscalls, which are probably unnecessary most of the time. At first, I wanted to make a pair of open/open_unchecked functions for each type like here, but now I think it would be better to avoid syscall overhead by default. The purpose of adding from_file was to let the user do all sort of checks with a FIFO file if they need to, and then do the conversion. It's not possible to do those checks independently before calling Sender::open because of TOCTTOU.
Note that the docs for from_file warn users to make sure the access mode is correct etc. Maybe this should be more emphasized, or it should be renamed to from_file_unchecked and then we can add a from_file variant with checks.
There was a problem hiding this comment.
It's not possible to do those checks independently before calling Sender::open because of TOCTTOU.
That's true, it's only possible after the file is opened.
Maybe this should be more emphasized, or it should be renamed to from_file_unchecked and then we can add a from_file variant with checks.
IMHO renaming it to from_file_unchecked and adding a new from_file would be better.
tokio/src/net/pipe.rs
Outdated
| /// The runtime is usually set implicitly when this function is called | ||
| /// from a future driven by a tokio runtime, otherwise runtime can be set | ||
| /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. | ||
| pub fn from_file(file: File) -> io::Result<Receiver> { |
There was a problem hiding this comment.
Same suggestion as Sender::from_file.
tokio/src/net/pipe.rs
Outdated
| unsafe { &mut *(dst as *mut _ as *mut [std::mem::MaybeUninit<u8>] as *mut [u8]) }; | ||
|
|
||
| // Safety: We trust `mio_pipe::Receiver::read` to have filled up `n` bytes in the | ||
| // buffer. |
There was a problem hiding this comment.
Note that since dst is [MaybeUninit<u8>], we should also add that we trsut mio_pipe::Receiver::read to not read from dst.
Noah-Kennedy
left a comment
There was a problem hiding this comment.
I'm gonna take another look tomorrow before I approve, but this looks great. I think it's probably fine to merge as is.
tokio/src/net/pipe.rs
Outdated
| /// The runtime is usually set implicitly when this function is called | ||
| /// from a future driven by a tokio runtime, otherwise runtime can be set | ||
| /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. | ||
| pub fn open_rw<P>(path: P) -> io::Result<Sender> |
There was a problem hiding this comment.
Why not lock this behind #[cfg(linux)]?
There was a problem hiding this comment.
It's probably a good idea, but while only Linux explicitly allows using FIFOs in O_RDWR access mode, it may also be possible on other systems. For example, it seems possible on FreeBSD, but it's lacking documentation or I couldn't find it. So, an experienced user may want to use open_rw on other systems than Linux.
But I think locking it to Linux is the better choice since it will probably prevent more mistakes, I will change it in the next iteration.
There was a problem hiding this comment.
We could also start with Linux and allow this on other platforms as we find out more
tokio/src/net/pipe.rs
Outdated
| /// The runtime is usually set implicitly when this function is called | ||
| /// from a future driven by a tokio runtime, otherwise runtime can be set | ||
| /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. | ||
| pub fn from_file(file: File) -> io::Result<Sender> { |
There was a problem hiding this comment.
I'm not really sure that this is necessary.
tokio/src/net/pipe.rs
Outdated
| /// The runtime is usually set implicitly when this function is called | ||
| /// from a future driven by a tokio runtime, otherwise runtime can be set | ||
| /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. | ||
| pub fn from_file(file: File) -> io::Result<Sender> { |
There was a problem hiding this comment.
I'm not really sure that this is necessary.
While it's not absolutely necessary, checking it will rule out some stupid mistakes, e.g. user opened a regular file and passed it to Sender::from_file, or they opened the read end of a pipe and passed it here.
It's still runtime error, but better than no error.
tokio/src/net/pipe.rs
Outdated
| /// The runtime is usually set implicitly when this function is called | ||
| /// from a future driven by a tokio runtime, otherwise runtime can be set | ||
| /// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function. | ||
| pub fn from_file(file: File) -> io::Result<Sender> { |
There was a problem hiding this comment.
Also, I think it would be better to use OwnedFd here instead of File:
fn from_owned_fd(fd: Into<OwnedFd>) -> io::Result<Sender>;
since impl From<File> for OwnedFd makes it possible to create OwnedFd from File.
There are also From<ChildStdin>, From<ChildStdOut> and From<ChildStdErr> implementation, which would makes this function more useful.
|
It may be a good idea to also add the As an example, consider a scenario where a reader wants to receive commands from a named pipe, which are periodically sent by a writer. If it is done like this: let mut reader = pipe::Receiver::open(path)?;
loop {
let mut buf = vec![0; 0x100];
reader.read_exact(&mut buf)?;
/* handle the command */
}and the writer closes the file, then the reader will fail in the next iteration with If the reader is opened in the read-write access mode, then he also holds an open writing end and therefore there won't be any EOF. The reader can asynchronously wait for the next writer to open the file without sleeping loops and re-opening the file. Note that this is a Linux-specific solution. |
Motivation
This change provides types to asynchronously read and write to named Unix pipes (FIFOs). Currently, the only way to use named pipes is either using
tokio::fs::File, which spawns blocking tasks, or usingAsyncFd, which does not implementAsyncReadandAsyncWrite.See also #5318.
Solution
This change exposes a new module:
tokio::net::pipewithSenderandReceivertypes, which represent a writing end and a reading end of a Unix pipe, respectively. Both types encapsulate correspondingSenderandReceivertypes from mio, which in turn usestd::fs::Fileunderneath to handle reads and writes.SenderisAsyncWriteandReceiverisAsyncRead. Following the pattern of other types fromnetsuch as ReadHalf/WriteHalf for socket streams,Receiveradditionally has:poll_read_readyreadablereadytry_readtry_read_vectoredtry_read_bufand
Senderhas:poll_write_readywritablereadytry_writetry_write_vectoredI considered omitting
readymethods, since the communication is already one-way and there arereadable/writablefunctions for waiting for readiness. However,readymethod allows inspecting readiness events and distinguishingREAD_CLOSEDfromREADABLEevents, what might be the reason why types such asOwnedReadHalfhavereadyfunctions in the first place. Therefore, I decided to follow the pattern and includereadymethods as well.Opening a FIFO
Creating a pipe from a FIFO file is done by opening the file with a proper access mode flag, see fifo(7).
openmethods onSender/Receivertypes open FIFO files for writing/reading in non-blocking mode and register them in the event loop.However, there is a requirement that the reading end has to be opened before any writing end. If a
Senderopens a FIFO with no readers, the open will immediately fail withENXIO. For that reason, docs forSender::openinclude an example presenting how to wait for the reading end by sleeping in a loop.On Linux, it is also possible to open a FIFO file in access mode for both reading and writing. Such open will never fail and can be used as a workaround to this problem. For Linux users
Sender::open_rwcan open a FIFO file in theO_RDWRaccess mode. Note that the reading access won't be used in practice, asSenderhas no methods/traits to read from a pipe and is not registered in the event loop with interest for read events.Sender/Receiveralso havefrom_filemethods which handle conversion from stdFiles holding a file descriptor to a pipe.from_fileis necessary for users which are not sure whether a file is a special FIFO file that can be opened with theopenmethod. In such case they can useFilemethods to inspect file's metadata and then convert it to a pipe.Next steps
Since on a Unix system anonymous pipes and named pipes (FIFOs) are only different in the way they are created (pipe() syscall vs. mkfifo + open),
Sender/Receivertypes can provide one abstraction for both cases. Usage of anonymous pipes is almost exclusively restricted to communication with spawned processes, and for that there already is an async interface intokio::processwith types likeChildStdinetc. A user may wish to have one type for pipes used to communicate with children processes and for pipes created from FIFO files in a filesystem. I think it's reasonable to add integration with types fromtokio::process. This can be done in the following way:tokio::io::unix. This type can be done the same way asChildStdioi.e. useSourceFdinstead of types frommio::unix::pipe.netand for anonymous pipes inprocess.From<ChildStdin>forSenderandFrom<ChildStdout>,From<ChildStderr>forReceiver. This conversion can be infallible since the pipe is already registered in the event loop.