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

io: propose new AsyncRead / AsyncWrite traits #2716

Open
carllerche opened this issue Jul 29, 2020 · 27 comments
Open

io: propose new AsyncRead / AsyncWrite traits #2716

carllerche opened this issue Jul 29, 2020 · 27 comments
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-io Module: tokio/io

Comments

@carllerche
Copy link
Member

carllerche commented Jul 29, 2020

Previous attempt: #1744.

Summary

The proposal aims to improve the I/O traits in a few ways:

  • Enable reads into uninitialized memory without unsafe except in the leaf implementations (TcpStream, File, ...).
  • Remove the poll_read_buf and poll_write_buf functions from the traits

Improving the API to better support reads into uninitialized memory will be done by matching this RFC.

Removing poll_read_buf and poll_write_buf from the traits implies that vectored operations will no longer be covered by the AsyncRead / AsyncWrite traits. Instead, vectored operations will be provided as methods on structs that support them (TcpStream, ...).

Motivation

See the std RFC for the motivation behind better supporting reads into uninitialized memory.

As for poll_read_buf and poll_write_buf, these functions require function level generics. Including them on the trait causes problems when using trait objects. These functions were originally added to AsyncRead and AsyncWrite to provide vectored operation support at the trait level. However, vectored operations are not proving to be very useful on the traits.

First, very few implementations of AsyncRead / AsyncWrite actually implement these functions. Second, the default implementation of these functions is worse than just calling poll_read and poll_write. The default implementations just forward the first buffer to poll_read and poll_write. However, when vectored operations are not available, the best strategy is to create an intermediate buffer instead of many small buffers. The cost of copies is lower than the cost of many syscalls.

Including vectored operation functions on the trait makes it difficult for the caller to decide how to interact with the byte stream. The fact that callers need to behave differently with the byte stream when the byte stream supports vectored operation vs. when it doesn't implies that vectored vs. non-vectored operations are separate concerns.

In the future, we could provide vectored variants of AsyncRead / AsyncWrite (maybe AsyncVectoredRead and AsyncVectoredWrite). However, as adding this would be forwards compatible, this decision can be punted.

Proposal

The proposed traits are:

pub trait AsyncRead {
    fn poll_read(
        self: Pin<&mut Self>, 
        cx: &mut Context, 
        buf: &mut ReadBuf<'_>
    ) -> Poll<Result<()>>;
}

pub struct ReadBuf<'a> {
    buf: &'a mut [MaybeUninit<u8>],
    filled: usize,
    initialized: usize,
}

impl<'a> ReadBuf<'a> {
    // functions here, see std RFC
}
pub trait AsyncWrite {
    fn poll_write(
        self: Pin<&mut Self>,
        cx: &mut Context,
        buf: &[u8]
    ) -> Poll<Result<usize>>;
}

See the std rfc for the full ReadBuf type and examples of how to use it.

Changes for the end-user of Tokio

This change will not materially change how the end-user of Tokio interacts with read/write types. Most users go via the AsyncReadExt and AsyncWriteExt helper traits. The functions on these traits will stay the same for the most part. For example:

use tokio::io::AsyncReadExt;

let mut my_buf = vec![0; 1024];
let n = my_stream.read(&mut my_buf).await?;

Here the read fn remains the same. The implementation of the read fn wraps the supplied buffer, which is fully initialized in a ReadBuf.

To support reads into uninitialized memory, a new helper function is added to AsyncReadExt:

async fn read_buf<T: bytes::BufMut>(&mut self, buf: T) -> io::Result<usize> { ... }

This function uses the BufMut trait provided by the bytes crate to handle reading into uninitialized memory. The function bridges the trait with the ReadBuf struct.

Where should the traits be defined?

A common question is "where should the traits be defined?". Should they be in Tokio, the futures crate, or std?

Tokio aims to release version 1.0 by the end of the year. The 1.0 release will come with stability guarantees as well as a clear support timeline. To do this, all public types must provide at least as strong of a guarantee as Tokio. All types in std provide such a guarantee. As of now, the futures crate does not include stability guarantees.

If the linked RFC is accepted, the ReadBuf type will be included by std at some point. We could then evaluate if using the ReadBuf in std is possible.

We could also propose equivalent AsyncRead / AsyncWrite traits for std. The path would be to first land this proposal in Tokio 0.3 to gain experience and propose the traits as an RFC after that.

That said, there may be an argument to maintain the ReadBuf and traits in Tokio itself for future-proofing Tokio for possible io_uring integration.

Potential io_uring future-proofing

io_uring is a submission-based async I/O API in modern Linux kernels. It is still under active development but is already showing impressive results. Being a submission-based API makes it challenging to integrate w/ Tokio's current I/O model. Also, because it is still under active development and not available on most of today's Linux boxes, supporting io_uring is not a priority for 1.0. However, it would be nice to be able to support it at some level before Tokio 2.0.

At a high level, io_uring works by passing ownership of buffers between the process and the kernel. Submitting a read involves passing ownership of a buffer to the kernel to be filled with data. Submitting a write involves passing ownership of a buffer containing the data to write to the kernel. In both cases, when the kernel completes the operation, ownership of the buffer is passed back to the process.

If Tokio owns the ReadBuf type, it could provide a specialized API for io_uring resources to "take ownership" of the buffer:

use bytes::BytesMut;

struct ReadBuf<'a> {
   inner: enum Variant {
       Owned(&mut BytesMut),
       Slice(....),
   },
}

impl ReadBuf<'a> {
    fn try_take_buf(&mut self) -> Option<BytesMut> { ... }

    fn try_join_buf(&mut self, buf: BytesMut) -> Result<(), BytesMut> { ... }
}

A TcpStream implementation backed by io_uring would be able to try to take ownership of the buffer from the ReadBuf. If successful, the buffer could then be passed directly to the kernel without any additional copying. The caller would then call poll_read again once the read has completed. At that point, TcpStream would attempt to "re-attach" the buffer. This, again, would be a no-copy operation.

This works because BytesMut is a ref-counted structure. Re-attaching is done by ensuring that two BytesMut instances originally came from the same allocation. If re-attaching fails due to the caller passing a different buffer, then the read falls back to copying the data into the given read buffer.

The same could be done with AsyncWrite:

trait AsyncWrite {
    fn poll_write(self: Pin<&mut Self>, cx: &mut Context, buf: &mut WriteBuf<'a>) -> io::Result<usize>;
}

struct WriteBuf<'a> { ... }

impl WriteBuf<'a> {
    // If the WriteBuf is backed by `Bytes`, this is an Arc ref inc.
    fn try_clone_buf(&mut self) -> Option<Bytes> { ... }

    // No join is necessary
}

Risks

It is still much too early to know for sure if the future-proofed API will work well in supporting io_uring. However, the risk of future-proofing AsyncWrite is minimal. The WriteBuf wrapper around a &[u8] does not add overhead besides friction when using the traits. Most users would not interact directly with AsyncWrite and, instead, use the helper functions provided by AsyncWriteExt. As with AsyncReadExt, these functions would not expose the WriteBuf struct. Instead, they would bridge the input buffer with the WriteBuf struct.

Open questions

Should we attempt to future-proof AsyncRead / AsyncWrite traits for io_uring?

@carllerche carllerche added C-proposal Category: a proposal and request for comments A-tokio Area: The main tokio crate M-io Module: tokio/io labels Jul 29, 2020
@carllerche carllerche added this to the v0.3 milestone Jul 29, 2020
@carllerche carllerche mentioned this issue Jul 29, 2020
10 tasks
@hawkw
Copy link
Member

hawkw commented Jul 30, 2020

A common question is "where should the traits be defined?". Should they be in Tokio, the futures crate, or std?

I've heard at least a few people mention that they would prefer to use Tokio's AsyncRead/AsyncWrite, as opposed to the futures version, but cannot depend on Tokio for various reasons (such as requiring no_std, or concerns that feature flags on a tokio dependency will be enabled accidentally).

Have we considered factoring out just the IO traits (and perhaps the Buf traits) into a new version of the tokio-io crate? A single additional crate wouldn't introduce the same maintenance burden of the seven or so crates we maintained in the 0.1 ecosystem, and (as it would be extremely minimal) would be very stable. I imagine that the combinators and stuff could remain in the tokio crate.

This would hopefully result in more widespread ecosystem use of Tokio's IO traits, which could be useful for collecting design feedback, and good precedent if we do want to eventually propose them for inclusion in std.

Just a thought.

@Nemo157
Copy link
Contributor

Nemo157 commented Jul 30, 2020

I'm one of the people that have mentioned concerns that feature flags on a tokio dependency will be enabled accidentally (though probably not where @hawkw has seen it 😁). I found the old setup of having the traits in a separate crate useful when integrating hyper (which exposes Tokio's traits) with alternative IO runtimes. I could at a glance see that tokio itself was not being pulled into my cargo tree, only the tokio-io crate. With hyper 0.13 I'm now always wondering whether I'm accidentally wasting time compiling unused Tokio runtime support.

@carllerche
Copy link
Member Author

For me, the main reason to keep AysncRead / AsyncWrite traits within the tokio crate itself would be to enable experimenting with potential io_uring APIs. If we want to experiment with some of the ideas I mentioned towards the end of the issue, we would need both TcpStream and ReadBuf to exist in the same crate.

@carllerche
Copy link
Member Author

@Nemo157 We also could possibly do the reverse. Provide a tokio-io crate that depends on tokio with the correct feature flags set and re-exports the types.

@seanmonstar
Copy link
Member

I noticed a difference in return type of poll_read between what's suggested in this issue, and the std RFC:

  • This proposal has the return type usize.
  • The std RFC has the return type of (), assuming you can check the ReadBuf for how much was read.

@carllerche
Copy link
Member Author

Ah. I would go w/ the std RFC. There is no point in returning the same data twice. That would lead to errors. I can update the original issue.

@Nemo157
Copy link
Contributor

Nemo157 commented Aug 12, 2020

We also could possibly do the reverse. Provide a tokio-io crate that depends on tokio with the correct feature flags set and re-exports the types.

That doesn't help identifying whether more feature flags have been set by a dependency. I guess the real solution would be to enhance cargo-deny to be able to tell it to allow having tokio but deny having tokio/rt-core enabled.

@tmandry
Copy link

tmandry commented Aug 13, 2020

I'm in a similar predicament: We want to use the traits, but we can't use the tokio runtime on fuchsia. So (since we use cargo vendor) there's a big blob of mostly-unused code in our tree, with a big diff of unsafe code every time we try to update. We'd get compiler errors if the runtime were accidentally enabled on fuchsia, but it could happen silently on host.

It's really not a good situation when all we need is a couple traits.

carllerche pushed a commit that referenced this issue Aug 14, 2020
Works towards #2716. Changes the argument to `AsyncRead::poll_read` to
take a `ReadBuf` struct that safely manages writes to uninitialized memory.
carllerche added a commit that referenced this issue Sep 24, 2020
These functions have object safety issues. It also has been decided to
avoid vectored operations on the I/O traits. A later PR will bring back
vectored operations on specific types that support them.

Refs: #2879, #2716
carllerche added a commit that referenced this issue Sep 25, 2020
These functions have object safety issues. It also has been decided to
avoid vectored operations on the I/O traits. A later PR will bring back
vectored operations on specific types that support them.

Refs: #2879, #2716
@carllerche
Copy link
Member Author

An initial attempt at implementing TcpStream::by_ref() was not successful. The returned TcpStreamRef needed to be !Unpin in order to safely implement AsyncRead and AsyncWrite. This makes the API awkard. It is no longer possible to write: my_stream.by_ref().read(...).

One would need to do:

pin!(let stream_ref = my_stream.by_ref());
stream_ref.read(...).

This is a bit awkward.

A possible solution would be to have by_ref() own its own ScheduledIo in the I/O driver's resource slab.

@carllerche
Copy link
Member Author

Enough has been done for 0.3. The remaining questions can be addressed before 1.0.

@carllerche carllerche modified the milestones: v0.3, v1.0 Oct 8, 2020
@asomers
Copy link
Contributor

asomers commented Nov 29, 2020

The RFC link is dead. https://github.com/sfackler/rfcs/blob/read-buf/text/0000-read-buf.md . Could you please find a good link for it, and update the issue description?

@tesaguri
Copy link
Contributor

The read-buf RFC has been merged. Tracking issue: rust-lang/rust#78485.

@carllerche
Copy link
Member Author

Nothing left requires breaking changes. Removing the 1.0 tag.

@Arnavion
Copy link
Contributor

The OP was planning to remove vectored fns from AsyncRead and AsyncWrite and instead expose them via concrete types like tokio::net::TcpStream, but tokio 1.0 was released with AsyncWrite::poll_write_vectored, no AsyncRead::poll_read_vectored, and no _vectored functions on tokio::net::TcpStream.

AsyncReadExt::read_buf admits ButMut impls with multiple chunks, but it has no choice but to fill each chunk one at a time via the underlying AsyncRead::poll_read. Since AsyncReadExt is blanket-impled for all AsyncReads, it's also not possible for a concrete type like tokio::net::TcpStream to provide a vectored impl either.

Therefore, with 1.0 there's no way to do vectored reads, only writes.

What are the options now? Adding AsyncRead::poll_read_vectored for 1.x ? Waiting for 2.0 to remove AsyncWrite::poll_write_vectored and add _vectored functions on TcpStream ? Something else?

@Darksonn
Copy link
Contributor

Vectored read methods can still be added to AsyncRead. It was not done back then because vectored writes are a lot more useful than vectored reads, so we prioritized spending our time on them.

@AlexTMjugador
Copy link

Vectored reads are also useful for dealing with input data that is splitted into several buffers, which is the case when dealing with fixed-size header + variable size payload protocols, to minimize the number of syscalls. Currently (as of Tokio 1.9.0) it is already possible to do vectored reads for TcpStream, albeit at a fairly low level, via the try_read_vectored method. Are there any news since April about more general and high-level support for vectored reads?

@Darksonn
Copy link
Contributor

There are no news, but I see no reason against adding them.

@infogulch
Copy link

Is this implemented as of 1.22.0? https://docs.rs/tokio/latest/tokio/io/trait.AsyncRead.html

@Darksonn
Copy link
Contributor

Vectored reads? No.

@ileixe
Copy link

ileixe commented Aug 28, 2024

Hi, I'd like to ask current status for async trait from io-uring user perspective. Is there any ongoing discussion or plan to express completion API with async read trait variants? For example, ringbhan suggested AsyncBufRead to express callee managed buffer (so can be used in completion API context). https://github.com/ringbahn/ringbahn/blob/08e6464e64d8cc92e4af837dfe96778c85fc401c/src/fs.rs#L196

withoutboats mentioned the trait in his blog as well (https://without.boats/blog/io-uring/), and I think it makes much sense.

We've been hard user for tokio-uring, and current dedicated API requires owning buffer is one of the most painful design that user accepts and I want to fix this situation.

Any ideas would be appreicated.

@NobodyXu
Copy link
Contributor

I think the existing AsyncRead could work with io-uring.

Io-uring works best with uring managed buffer, so tokio could maintain that io-uring buffer and then copy data out of it once the read is done, and put that buffer back for reuse.

Same for AsyncWrite, the existing API can serve as a good starting point.

For proxy which just forward data, some special copy methods could be provided using io-uring sendfile/splice/copy_file_range, or tokio::io::copy might use some specialisation using castaway

For I/O that needs to further process the data (i.e. decryption/decompression), then yeah, I think it needs to expose the internal buffer somehow, so that it could do zero-copy decryption/decompression.

@Darksonn
Copy link
Contributor

I think the solution that looks best right now is this:

  • In the case of tokio::fs::File it already owns a buffer internally. It will keep doing that with io_uring and it should work just fine with the IO traits.
  • For tokio::net types, we will not submit read operations to io_uring. Instead, we will submit IORING_OP_POLL_ADD completions to wait for the resource to become readable and perform the actual read using a non-blocking read syscall as usual. This will be directly compatible with the IO traits as they exist today.

cc @Noah-Kennedy

@NobodyXu
Copy link
Contributor

For tokio::net types, we will not submit read operations to io_uring.

That's reasonable decision, since having an implicit copy changes behavior of existing program, and io-uring might not provide enough speedup to justify the additional memory usage/copy.

@ileixe
Copy link

ileixe commented Aug 29, 2024

I think the solution that looks best right now is this:

  • In the case of tokio::fs::File it already owns a buffer internally. It will keep doing that with io_uring and it should work just fine with the IO traits.
  • For tokio::net types, we will not submit read operations to io_uring. Instead, we will submit IORING_OP_POLL_ADD completions to wait for the resource to become readable and perform the actual read using a non-blocking read syscall as usual. This will be directly compatible with the IO traits as they exist today.

Ah, I did not aware that File::read is already accompanied by copy (which is surprising). Isn't there any discussion for zero copy File API? (Sorry to ask about very old context, it's been in this state for quite a while.)

@Darksonn
Copy link
Contributor

You can read about the status quo in the docs for tokio::fs.

@ileixe
Copy link

ileixe commented Aug 29, 2024

Thx. This also helps me to understand current status.

@Noah-Kennedy
Copy link
Contributor

I'm not really a fan of implicit buffering.

I think any new file IO we do with io_uring should probably have new traits which do not rely on buffering under the hood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-io Module: tokio/io
Projects
None yet
Development

No branches or pull requests