Relax lifetime requirements for PollFd::new

[asomers]

Fixes #2118

[asomers]

cc @SteveLauC

[MaxVerevkin]

I think the argument should not be a reference at all, there is impl<T: AsFd> AsFd for &T.

[SteveLauC]

If it takes fd by value, then we should add a warning to the users in the doc comment to not let them pass the types that are owned file descriptors (e.g., File)

Kinda think we are facing this issue everywhere, here is also a thread discussing this stuff: #2115

[asomers]

I think the argument should not be a reference at all, there is impl<T: AsFd> AsFd for &T.

Actually, this is subtly wrong. And it's probably wrong for all constructors. If PollFd::new takes fd by value, then it immediately drops it, causing the file descriptor to be closed. The fact that it currently compiles at all means that there is probably something wrong with the lifetime handling; I'll double check that.

Now that I've reflected on this, the original approach of only taking a reference was a pretty good approach, because usually a PollFd isn't useful if it owns the file descriptor. After all, you usually want to do something else with the file other than just poll it. If we want PollFd to actually own it, then we should probably take an OwnedFd.

[asomers]

@MaxVerevkin Here's an example that demonstrates why PollFd must take its argument by reference. In this example, PollFd::new appears to take its argument by ownership, but it actually drops the OwnedFd at the end of the new method. This example compiles with the current state of the branch, but it ought to fail.

    /// ```compile_fail,E0597
    /// # use std::os::fd::{AsFd, AsRawFd, FromRawFd, OwnedFd};
    /// # use nix::{
    /// #     poll::{PollFd, PollFlags, poll},
    /// #     unistd::{pipe, read}
    /// # };
    /// let pfd = {
    ///     let (r, w) = pipe().unwrap();
    ///     let r = unsafe { OwnedFd::from_raw_fd(r) };
    ///     PollFd::new(r, PollFlags::POLLIN)
    /// };
    /// let mut fds = [pfd];
    /// poll(&mut fds, -1).unwrap();
    /// let mut buf = [0u8; 80];
    /// read(pfd.as_fd().as_raw_fd(), &mut buf[..]);
    /// ```

So either we must take by reference, or else the struct should include an OwnedFd. Or, it could take an OwnedFd and perform into_raw, and we could subsequently to a OwnedFd::from_raw_fd() during Drop. But that's complicated, and it wouldn't be generic for any AsFd argument.

[MaxVerevkin]

If PollFd::new takes fd by value, then it immediately drops it, causing the file descriptor to be closed.

Good observation. Yeah, by-value only make sense for BorrowedFd...

If we want PollFd to actually own it, then we should probably take an OwnedFd.

Please no :)

---

By the way, don't know if it deserves its own issue. How do you reuse the same fd buffer with this new API? If I have a Vec<PollFd>, it captures the lifetime and prevents me from using IO resources even if I .clear() the buffer. Storing this buffer in a struct along with these IO resources is impossible (self-referencing). This forces me to create a new Vec each time in most of my projects, which forced me to "downgrade" to libc::poll.

[asomers]

Ok, I just pushed changes that restore the reference, and also rewords @SteveLauC 's explanatory comment a bit.

[MaxVerevkin]

This doesn't work either:

let mut fds = Vec::new();

{
    let file = std::fs::File::open("/etc/hostname").unwrap();
    fds.push(PollFd::<'static>::new(&file, PollFlags::POLLIN));
    drop(file);
}

poll(&mut fds, -1).unwrap();

This compiles because File: 'static is satisfied. But previous approach (&'fd Fd) doesn't work with borrowed fds.

[asomers]

This doesn't work either:

let mut fds = Vec::new();

{
    let file = std::fs::File::open("/etc/hostname").unwrap();
    fds.push(PollFd::<'static>::new(&file, PollFlags::POLLIN));
    drop(file);
}

poll(&mut fds, -1).unwrap();

This compiles because File: 'static is satisfied. But previous approach (&'fd Fd) doesn't work with borrowed fds.

Ouch. That's a big problem. Maybe we should change Poll::new to take a BorrowedFd by value, just to be sure that the lifetime is specified?

[MaxVerevkin]

Maybe we should change Poll::new to take a BorrowedFd by value

That's definitely a solution. I wanted to propose it but I wasn't sure if it's a good one.

[SteveLauC]

Maybe we should change Poll::new to take a BorrowedFd by value

This approach LGTM