Skip to content
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.

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

Add new API for named fifo #416

Closed
NobodyXu opened this issue Jul 24, 2024 · 6 comments
Closed

Add new API for named fifo #416

NobodyXu opened this issue Jul 24, 2024 · 6 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@NobodyXu
Copy link

NobodyXu commented Jul 24, 2024

Proposal

Problem statement

Annoymous and named pipe are widely used, there is crate os_pipe which just provides an abstraction for pipe on unix and has 15 million download.

While having a third-party crate for it is enough for many crates, I think it'd be better if it's in stdlib so that it can be used without having to another dependency for this.

It would also enable better integration with the std::process API, since users might want to pipe output of multiple processes to one pipe and read them.

Motivating examples or use cases

jobserver-rs, for example, is used by cc-rs and pulled in as build-dependencies quite often.

It internally implements all kinds of API for named fifo, contains a bunch of unsafe code for this and quite some code for just managing the fifo.

It'd be great if we could move them to stdlib and make jobserver-rs easier to maintain.
It might also speedup jobserver-rs compilation since it could've drop the libc dependency.

tokio, the widely used async executor, already provide pipe support in mod tokio::net::unix::pipe.

Solution sketch

mod std::fifo {
    #[derive(Debug, Default, Clone)]
    pub struct FifoOpenOptions { ... }
    impl FifoOpenOptions {
        pub fn new() -> Self;

        pub fn create(&mut self, create: bool) -> &mut Self;
        pub fn create_new(&mut self, create_new: bool) -> &mut Self;

        pub fn open<P: AsRef<Path>>(&self, path: P) -> io::Result<Fifo>;
    }

    #[derive(Debug)]
    pub struct Fifo(/* private fields */);

    impl Fifo {
        pub fn try_clone(&self) -> io::Result<Self>;
    }

    #[cfg(unix)]
    impl AsFd for Fifo { ... }
    #[cfg(unix)]
    impl AsRawFd for Fifo { ... }

    #[cfg(windows)]
    impl AsHandle for Fifo { ... }
    #[cfg(windows)]
    impl AsRawHandle for Fifo { ... }

    // Use TryFrom here, because not every owned fd is a valid pipe
    #[cfg(unix)]
    impl From<OwnedFd> for Fifo { ... }
    #[cfg(windows)]
    impl From<OwnedHandle> for Fifo { ... }

    #[cfg(unix)]
    impl From<Fifo> for OwnedFd { ... }
    #[cfg(windows)]
    impl From<Fifo> for OwnedHandle { ... }

    impl From<Fifo> for Stdio { ... }

    #[cfg(unix)]
    impl FromRawFd for Fifo { ... }
    #[cfg(unix)]
    impl IntoRawFd for Fifo { ... }

    #[cfg(windows)]
    impl FromRawHandle for Fifo { ... }
    #[cfg(windows)]
    impl IntoRawHandle for Fifo { ... }

    impl<'a> Read for &'a Fifo { ...}
    impl Read for Fifo { ... }

    impl<'a> Write for &'a Fifo { ...}
    impl Write for Fifo { ... }
}

Alternatives

Alternatively, we could let the user to use File to open a fifo, however File has different characteristics from a fifo and the user would still need a third-party to create a fifo.

Or we could just leave them up to third-party crates, which is the current status-quo, which is OK-ish but not good enough
for users who needfifo, they would have to grab a third-party crate for this.

Links and related work

@NobodyXu NobodyXu added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jul 24, 2024
@NobodyXu NobodyXu changed the title (My API Change Proposal) Add new API for named fifo Jul 24, 2024
@kennytm
Copy link
Member

kennytm commented Jul 24, 2024

    // Use TryFrom here, because not every owned fd is a valid pipe
    #[cfg(unix)]
    impl TryFrom<OwnedFd> for Fifo { ... }
    #[cfg(windows)]
    impl TryFrom<OwnedHandle> for Fifo { ... }

We allow implementing From<OwnedFd> for UdpSocket and TcpStream etc which clearly "not every owned fd is a valid socket". I don't see why the Fifo requires special treatment.

If you turn a non-pipe fd to a Fifo, when using the pipe-specific functions the underlying OS API should be able to recognize the invalid input and return an error.

@NobodyXu
Copy link
Author

Thanks, I've updated the proposed API

@Amanieu
Copy link
Member

Amanieu commented Jul 30, 2024

We discussed this in the libs-api meeting. A question was raised of whether this is sufficiently different from a File to justify introducing a new type. We could instead just expose mkfifo as a unix-specific filesystem method and then let people open it like a normal file.

@NobodyXu
Copy link
Author

NobodyXu commented Jul 31, 2024

A question was raised of whether this is sufficiently different from a File to justify introducing a new type.

@Amanieu There are quite some differences:

  • fifo on unix sometimes needs to be opened with readable and writable, otherwise it would block until the other end is opened, which is quite surprising behavior
  • fifo does not implement Seek
  • fifo does not support sync
  • fifo support additional methods: linux-specific tee, unix-specific set pipe size
  • fifo supports non-blocking read (which I might open a separate issue for)

It'd be better to create a dedicated abstraction for fifo.

@joshtriplett
Copy link
Member

joshtriplett commented Sep 3, 2024

fifo on unix sometimes needs to be opened with readable and writable, otherwise it would block until the other end is opened, which is quite surprising behavior

Surprising but easily documented: people can open it with a File as read/write.

fifo does not implement Seek

Neither do various other things that you can turn into a File or open as a File; if you try to seek you'll get an error.

fifo support additional methods: linux-specific tee, unix-specific set pipe size

If we expose those at all, we could expose them as methods taking AsFd. And having a distinct type doesn't particularly prevent calling these methods on an unexpected kind of file descriptor.

fifo supports non-blocking read (which I might open a separate issue for)

So do some other kinds of file descriptor.

@joshtriplett
Copy link
Member

We discussed this in today's @rust-lang/libs-api meeting.

We don't want to accept this ACP as written, and in particular don't want a separate Fifo type.

We would, however, accept a PR adding std::os::unix::mkfifo (accepting an AsRef<Path> and returning std::io::Result<()>).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

4 participants