Implement IntoRawFd for SharedFd and File

[thomasbarrett]

This PR implements the IntoRawFd trait for both the SharedFd and File types.

let file = File::open();
let fd = file.into_raw_fd();
let file = unsafe { File::from_raw_fd(fd) };

See this issue for additional context.

[FrankReh]

This is probably very reasonable but three things come to mind.

[FrankReh]

1. Why do we go through so much trouble to protect close in the first place? It's the file descriptor in the kernel that is the critical resource and it won't be removed until there are no outstanding references to it.

To quote from https://man.archlinux.org/man/close.2.en

If fd is the last file descriptor referring to the underlying open file description (see open(2)), the resources associated with the open file description are freed; if the file descriptor was the last reference to a file which has been removed using unlink(2), the file is deleted.

[FrankReh]

2. I think this PR should sit, or be gated, while the fixed descriptor support #222 is outstanding, because how to handle trying to take a raw fd when the File or SharedFd is represented by a uring fixed slot needs to be thought through, and getting that PR accepted could already be tricky enough.

Maybe that PR should include a function that can be used to test whether a File is a regular file descriptor or a fixed slot file descriptor so code that would otherwise panic can be circumvented by the caller.

[FrankReh]

3. Was going to be "where's the comment explaining what this enables?". But I saw the top description again and I think the quoted example makes it clear enough.

[FrankReh]

I don't understand this comment that says the in-flight operations hold a reference to &self. The in-flight operations I think you mean actually hold a cloned SharedFd, like in

tokio-uring/src/io/write.rs

Line 10 in afab7f2

fd: SharedFd,

[thomasbarrett]

Ah yeah... when I was trying to wrap my head around SharedFd, I came to the following conclusion (please correct me If I am wrong): The SharedFd close method will hang forever if there is any in-flight IO. However, the actual File::close API does not appear to share this issue because the Rust borrow-checker already actively prevents in-flight asynchronous methods (or even just references to File) when transferring ownership of a tokio_uring::File (either by File::close or by File::into_raw_fd).

Example 1: OK

fn main() -> std::io::Result<()> {
    tokio_uring::start(async {
        let file = File::open("test.txt").await?;
        let fd = file.into_raw_fd();
        let file = unsafe { File::from_raw_fd(fd) };
        file.close().await?;

        Ok(())
    })
}

Example 2: Compilation Error

An async block that isn't awaited before calling into_raw_fd or close will result in an error.

fn main() -> std::io::Result<()> {
    tokio_uring::start(async {
        let file = File::open("test.txt").await?;

        tokio_uring::spawn(async {
            let buf: Vec<u8> = Vec::with_capacity(16);
            let (buf, res) = file.read_at(buf, 0).await;
        });

        let fd = file.into_raw_fd();
        let file = unsafe { File::from_raw_fd(fd) };
        file.close().await?;

        Ok(())
    })
}

Example 3: Compilation Error

cannot move out of an Rc

fn main() -> std::io::Result<()> {
    tokio_uring::start(async {
        let file = std::rc::Rc::new(File::open("test.txt").await?);
        let file2 = file.clone();
        tokio_uring::spawn(async move {
            let buf: Vec<u8> = Vec::with_capacity(16);
            let (buf, res) = file2.read_at(buf, 0).await;
        });

        let fd = file.into_raw_fd();
        let file = unsafe { File::from_raw_fd(fd) };
        file.close().await?;

        Ok(())
    })
}

Example 4: Ok

This compiles since we use try_unwrap (which ensures that there are no other references to File).

fn main() -> std::io::Result<()> {
    tokio_uring::start(async {
        let file = std::rc::Rc::new(File::open("test.txt").await?);
        let file2 = file.clone();
        tokio_uring::spawn(async move {
            let buf: Vec<u8> = Vec::with_capacity(16);
            let (buf, res) = file2.read_at(buf, 0).await;
        });

        match std::rc::Rc::try_unwrap(file) {
            Ok(file) => {
                let fd = file.into_raw_fd();
                let file = unsafe { File::from_raw_fd(fd) };
                file.close().await;
            },
            Err(err) => {
                panic!("unexpected io in-flight")
            }
        }

        Ok(())
    })
}

[thomasbarrett]

Example 5: In-flight IO possible only with (incorrect usage of) unsafe code.

fn main() -> std::io::Result<()> {
    tokio_uring::start(async {
        let file = File::open("test.txt").await?;
        let fd = file.into_raw_fd();

        tokio_uring::spawn(async move {
            let file = unsafe { File::from_raw_fd(fd) };
            let buf: Vec<u8> = Vec::with_capacity(16);
            let (buf, res) = file.read_at(buf, 0).await;
        });

        let file = unsafe { File::from_raw_fd(fd) };
        file.close().await;

        Ok(())
    })
}

[FrankReh]

Does the whole notion of going from File to raw and back to a new File, when the SharedFd can be cloned, and defining the close call to triggered when a SharedFd's Inner's refcount drops to zero, call into question our whole close model?

[thomasbarrett]

Why do we go through so much trouble to protect close in the first place? It's the file descriptor in the kernel that is the critical resource and it won't be removed until there are no outstanding references to it.

Does the whole notion of going from File to raw and back to a new File, when the SharedFd can be cloned, and defining the close call to triggered when a SharedFd's Inner's refcount drops to zero, call into question our whole close model?

I'm right with you on both points. I think that we should absolutely simplify the SharedFd close logic. If I am thinking about this correctly, it would be sufficient for SharedFd to just panic if there are still any in-flight operations when closed.

[FrankReh]

Well, I don't want to invoke potential panics - in the code or in the users. The user may choose to call the synchronous close(2) on the raw fd. The raw_fd is available without giving up ownership, and without using unsafe. So how hard do we try to protect the user from themself? I will probably pose this same question somewhere else. I don't think we can have a completely safe and locked down api and still provide all the flexibility some people will want when they see this crate and just want a tokio friendly way to use urings where they are prepared to navigate all or most of the risks on their own.

I'm thinking we should offer some functionality under unsafe and give people closer access to the uring primitives, without getting in their way all the time. The crate has the noblest intentions - to be a safe place to use urings. But it comes at the cost of being very inflexible when the first adaptors want to do things their own special way.

[thomasbarrett]

Well, I don't want to invoke potential panics - in the code or in the users. The user may choose to call the synchronous close(2) on the raw fd. The raw_fd is available without giving up ownership, and without using unsafe.

@FrankReh This is not the case, the RawFd type is useless on its own (you can't perform any IO, it isn't closed on drop, etc). Synchronously losing a RawFd requires either using libc (unsafe), File::from_raw_fd (unsafe), or some other unsafe (or unsound) library. My fifth example demonstrates this.

[FrankReh]

@thomasbarrett Yes, as in the other issue, I was incorrect thinking close on raw didn't require unsafe code.

[FrankReh]

How does panicking when there are in flight operations during a drop help? Isn't it better to wait for the in flight operations to finish?

[FrankReh]

Are you saying the current logic for waiting and then closing is broken? I wish you had referenced

• SharedFd doesn't call wake() #122.

If that is essentially the same issue, I would have agreed something should be done. Let me try to implement reproducer of the perceived problem.

[FrankReh]

@thomasbarrett And the comment you are proposing to add at line 860, "Since all in-flight operations hold a reference to &self".
That still isn't right is it? This discussion has gone through so many other pages, I don't want lose track of why this proposal looks like a problem otherwise it may just sit. Or have I misunderstood something else?

[thomasbarrett]

How does panicking when there are in flight operations during a drop help? Isn't it better to wait for the in flight operations to finish?

Good point. It would probably be more semantically correct to replace the panics with unreachable. I will update.

[thomasbarrett]

Are you saying the current logic for waiting and then closing is broken? I wish you had referenced

Ahhh I should have looked through the old issues :). This seems very likely related. I will test the example on my "Simplify SharedFd close logic" branch and see if I can reproduce it.

[thomasbarrett]

How does panicking when there are in flight operations during a drop help? Isn't it better to wait for the in flight operations to finish?

I see now that there was something that I didn't fully understand . The "in-flight" operations don't necessarily need to be associated with any future held by the library user... there may also be futures that the user dropped (cancelled) in-flight. I'm not sure what should be done. I found this old reddit link from when tokio_uring was first released. Although I can't find the original design doc, someone quoted the design doc in saying:

tokio-uring will close the resource in the background, avoiding blocking the runtime. The drop handler will move ownership of the resource handle to the runtime and submit cancellation requests for any in-flight operation. Once all existing in-flight operations complete, the runtime will submit a close operation.

[thomasbarrett]

Ownership over operation futures that are dropped by library users are transferred to the runtime, which is supposed to cancel them and drop. I will update this PR so that into_raw_fd waits for cancelled operations to complete instead of panic-ing once the fix for this is merged.