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

AsyncRead and AsyncWrite #5

Open
nrc opened this issue Dec 7, 2021 · 45 comments
Open

AsyncRead and AsyncWrite #5

nrc opened this issue Dec 7, 2021 · 45 comments
Labels
A-stdlib Area: a standard library for async Rust

Comments

@nrc
Copy link
Owner

nrc commented Dec 7, 2021

Tracking issue for work on AsyncRead and AsyncWrite. The eventual goal is that we have a single, standardised version of these traits in std which are used by all (or at least most mainstream) async runtimes.

Known technical issues:

  • should these traits use poll_read/poll_write functions or async fn read/async fn write
  • how to handle writing to uninitialised memory in AsyncRead
  • how to simultaneously read and write
  • how to do vectored IO
  • working in no_std scenarios (I believe this only requires moving the Error trait to libcore, which is work in progress
  • ensuring these traits work well as trait objects
  • using async drop for shutdown?

There are also several closely related traits such as AsyncSeek, and in various libraries, extension traits and AsyncBufRead.

Current implementations:

Smol re-exports the futures versions.

@nrc nrc added the A-stdlib Area: a standard library for async Rust label Dec 7, 2021
@nrc
Copy link
Owner Author

nrc commented Dec 7, 2021

Some resources:

@TennyZhuang
Copy link

TennyZhuang commented Dec 7, 2021

  • should these traits use poll_read/poll_write functions or async fn read/async fn write

async fn is better, but it was blocked by async-trait which is unlikely to be stabilized in the short term. 😞

@ibraheemdev
Copy link

ibraheemdev commented Dec 7, 2021

It's actually blocked on a dyn-safe (inline) async traits.

@nrc
Copy link
Owner Author

nrc commented Dec 8, 2021

I think we have a solution to dyn-safe async traits that doesn't require 'inline' async.

@nrc
Copy link
Owner Author

nrc commented Dec 9, 2021

ReadBuf (RFC 2930) has landed on nightly now (rust-lang/rust#81156)

@yoshuawuyts
Copy link

Async-std: Read Write
Smol re-exports the futures versions.

Note that async-std also re-exports the futures-io traits, but does so using an alias. This does mean the traits are compatible though. In hindsight we probably should've kept the Async{Read,Write} terminology, but we didn't know that at the time.

@yoshuawuyts
Copy link

Another known blocker that I haven't seen mentioned yet: the working group needs to make a decision on the feasibility of async overloading. This will have consequences for the shape and location of the async IO traits.

Async overloading is being tracked on the roadmap, but does not yet have an initiative owner.

@nrc
Copy link
Owner Author

nrc commented Dec 16, 2021

Note that async-std also re-exports the futures-io traits

From the docs, they appear to have different definitions? The async-std versions have significantly more methods. Not sure if it a re-export of an earlier version of the futures definition or something more complex than that.

@yoshuawuyts
Copy link

yoshuawuyts commented Dec 16, 2021

From the docs, they appear to have different definitions? The async-std versions have significantly more methods.

yeah, that touches on another thing we probably shouldn't have done: the methods on async_std::io::{Read,Write} don't actually exist on them; they are only compiled in for the docs, to create the appearance that they do. The way they are made available is by importing async_std::io::prelude::* which includes ReadExt and WriteExt. It's very much a hack, but it allowed us to keep using the futures-io types, while still providing a cohesive feel.

The reason why we did this is because we wanted to push the "async-std is an async version of std" idea as far as we could. We wanted to prove out that it is indeed possible to map std's abstractions and usage patterns 1:1 to async Rust. And it worked; we now know that that it indeed can be done - modulo some missing language features like "async closures", "async drop" and "async traits". But perhaps if we had to try this again, we could've just exposed it as async_std::io::{AsyncRead,AsyncReadExt}. Though obviously that's speaking with the benefit of hindsight.

@nrc
Copy link
Owner Author

nrc commented Feb 15, 2022

A proposed design: https://www.ncameron.org/blog/async-read-and-write-traits/

I should flesh out the alternatives with examples and/or why they don't meet the stated goals.

@uazu
Copy link

uazu commented Feb 15, 2022

Just one comment on the proposed design (which looks fine to me although I'm not really the target audience):

  • For the examples where let mut buf = [0; 1024]; is done after the ready call, I'm trying to figure out how this saves memory, since usually stack space is reserved based on the maximum extent required. Does this depend on buf not carrying over any .await, which means it goes onto the real stack instead of into the coroutine context structure? So the aim is to not have the buffer held across any .await?

@nrc
Copy link
Owner Author

nrc commented Feb 16, 2022

For the examples where let mut buf = [0; 1024]; is done after the ready call, ...

These are simplified examples, in real life we might use one shared buffer or allocate space on the heap to be used in other functions, etc.

@bestouff
Copy link

Shouldn't AsyncRead and AsyncReady be renamed Read and Ready in the "Complete version" code block ?

@nrc
Copy link
Owner Author

nrc commented Feb 16, 2022

Shouldn't AsyncRead and AsyncReady be renamed Read and Ready in the "Complete version" code block ?

Whoops! They absolutely should. Fixed now, thanks!

@nrc
Copy link
Owner Author

nrc commented Feb 18, 2022

Feedback from Fuchsia:

The proposal suggests that for optimal performance, Ready should be used followed by a read call later. I think that might be quite challenging for us to implement because it would require locking between the ready notification and the read (to prevent the kernel discarding pages under memory pressure) and AFAICT, there's no indication of how much should be locked in the ready call.

And wonder if we could add a byte count to Interest.

Discussion ongoing on Zulip

@nrc
Copy link
Owner Author

nrc commented Feb 18, 2022

Some discussion on a possible alternative where we have a type like smol's Async and some of the API is on that type rather than the io traits (I believe the benefit here is that we keep the differences between the sync and async APIs restricted to a single location).

@nrc
Copy link
Owner Author

nrc commented Feb 18, 2022

The read_with style helpers from smol::Async may also be helpful for making the memory-optimal path more ergonomic in some cases.

@nrc
Copy link
Owner Author

nrc commented Mar 17, 2022

I've started to write up the designs from the blog posts in https://github.com/nrc/portable-interoperable/tree/master/io-traits

@yoshuawuyts
Copy link

Filed #7 related to this issue.

@carllerche
Copy link

The current version of the traits described in the README include vectored methods. In practice, I have found this to be a mistake because it is not possible to have a good default implementation. The user of Read/Write must have a different implementation depending on whether the I/O handle can support vectored ops. What this means in practice, a library like Hyper that takes a T: Read + Write must assume the T does not support vectored ops and avoid calling those methods.

I think, for vectored ops, it should be a separate trait. Converting a T: Read + Write -> VectoredRead + VectoredWrite means wrapping it with a buffer.

@NobodyXu
Copy link

Do you think the

fn is_vectored_writeable() -> bool;

interface that can be found in existing AsyncWrite/AsyncRead/Write/Read solves the problem?

@carllerche
Copy link

Would it work? Possibly. However, once you start adding boolean checks to see if a trait impl supports a feature or not, this seems to strongly suggest there should be two traits.

@NobodyXu
Copy link

That's true.

But if it is split into two traits, then what if the user has an IoSlice and doesn't care about the efficiency anyway?

@NobodyXu
Copy link

@Noah-Kennedy I wonder is it possible for async fn read to work with io-uring without copying.

Suppose that:

  • The future returned by async fn read under io-uring is lazy and does not actually issue any SQE until it is polled (and pinned).
  • The future returned by async fn read is not Unpin (has PhantomPinned), thus it must be Pined. And Pined object must either be leaked, or must be droped.

With these two assumptions, can we implement async fn read in io-uring without copying it into an internal owned buffer?

@Noah-Kennedy
Copy link

Nope, because when dropping a future for an in-flight op, it would be unsound still.

@Noah-Kennedy
Copy link

You can still make this work via IORING_OP_POLL_ADD, which lets you do readiness-based IO via uring.

@NobodyXu
Copy link

Nope, because when dropping a future for an in-flight op, it would be unsound still.

Can we add a drop implementation that cancels and wait for cancellation/IO completion?

It will be great if we have async drop though.

@Noah-Kennedy
Copy link

No, because this would block the runtime.

@NobodyXu
Copy link

No, because this would block the runtime.

Thanks, sounds like this can only be fixed with async drop.

@Noah-Kennedy
Copy link

Yup

@nrc
Copy link
Owner Author

nrc commented Jul 14, 2022

Even async Drop does not fix it. See https://ncameron.org/blog/async-io-with-completion-model-io-systems/ (tl;dr: destructors are not guaranteed to run in Rust)

@Thomasdezeeuw
Copy link

Even async Drop does not fix it. See https://ncameron.org/blog/async-io-with-completion-model-io-systems/ (tl;dr: destructors are not guaranteed to run in Rust)

I didn't have time to read the post, but based on the tl;dir: it's okay if the destructors aren't run. The problem is if they are run while the OS is still using the buffer. Leaking the buffer would be fine, not ideal of course, but at least it won't be unsound.

I've been thinking about using a separate "clean up" future for this. It would be implemented as a queue to which items can be send, on receiving an item it would wait for it to complete and call the destructor, essentially spawning a future to the "async drop". For I/O uring this would for example send it a function/future that would cancel the I/O operation and drop/dealloc the buffer. Of course this design doesn't work with AsyncRead/AsyncWrite current borrowed buffers (&mut [u8]), but it would with owned versions.

@NobodyXu
Copy link

Even async Drop does not fix it. See https://ncameron.org/blog/async-io-with-completion-model-io-systems/ (tl;dr: destructors are not guaranteed to run in Rust)

If the returned future is not Unpin and has to be Pin, then it should be guaranteed that it is leaked or dropped right?

@Thomasdezeeuw
Copy link

If the returned future is not Unpin and has to be Pin, then it should be guaranteed that it is leaked or dropped right?

Pin doesn't guarantee anything regarding whether it's dropped or not, it's only guaranteed to be used in the same memory location.

@NobodyXu
Copy link

NobodyXu commented Jul 14, 2022

If the returned future is not Unpin and has to be Pin, then it should be guaranteed that it is leaked or dropped right?

Pin doesn't guarantee anything regarding whether it's dropped or not, it's only guaranteed to be used in the same memory location.

According to Pin::new_unchecked:

This constructor is unsafe because we cannot guarantee that the data pointed to by pointer is pinned, meaning that the data will not be moved or its storage invalidated until it gets dropped.

Also from std::pin module's drop guarantee:

To make this work, not just moving the data is restricted; deallocating, repurposing, or otherwise invalidating the memory used to store the data is restricted, too. Concretely, for pinned data you have to maintain the invariant that its memory will not get invalidated or repurposed from the moment it gets pinned until when drop is called. Only once drop returns or panics, the memory may be reused.

And this:

Notice that this guarantee does not mean that memory does not leak! It is still completely okay to not ever call drop on a pinned element (e.g., you can still call mem::forget on a Pin<Box>). In the example of the doubly-linked list, that element would just stay in the list. However you must not free or reuse the storage without calling drop.

Essentially, you can only forget a pinned data if it is Unpin or it is allocated on heap or is a global variable.
For a stack variable, you must call drop on it.

@Noah-Kennedy
Copy link

@Thomasdezeeuw been thinking about that as well actually

@NobodyXu
Copy link

NobodyXu commented Jul 19, 2022

@Noah-Kennedy @nrc IMO it might be a good idea to to standardise bytes, expose its internal vtable, add functions for creating Bytes and BytesMut from provided/managed buffers of io-uring and ability to detect whether they contains provided/managed buffers.

Then we can add the following interfaces:

trait Read {
    async fn get_managed_buffers(&mut self, n: NonZeroUsize) -> Result<BytesMut>;
    async fn read_owned(&mut self, n: NonZeroUsize, owned_buf: BytesMut) -> Result<BytesMut>;
    async fn read_into_provided_buf(&mut self, n: NonZeroUsize) -> Result<BytesMut>;
}
trait Write {
    async fn get_managed_buffers(&mut self, n: NonZeroUsize) -> Result<BytesMut>;
    async fn write_owned(&mut self, owned_buf: Bytes) -> Result<usize>;
}

That will enable efficient use of io-uring since it is zero-copy and we could also support provided buffer easily.

Though stablising bytes alone would take a tons of effort.

@nrc
Copy link
Owner Author

nrc commented Jul 19, 2022

@NobodyXu The trouble with this approach is that it is a long way from the sync versions of Read/Write, and is a pretty un-ergonomic API. There is more on the requirements, etc. here: https://github.com/nrc/portable-interoperable/tree/master/io-traits#requirements

@NobodyXu
Copy link

@NobodyXu The trouble with this approach is that it is a long way from the sync versions of Read/Write, and is a pretty un-ergonomic API. There is more on the requirements, etc. here: https://github.com/nrc/portable-interoperable/tree/master/io-traits#requirements

Yeah, but I don't see any other way to support completion based systems efficiently, since they are most efficient with managed/provided buffer.

Passing a reference to a buffer requires copying around to internal buffer and even if we fix that, it still cannot match the performance of provided buffer due to the optimizations that can be applied to provided/managed buffer.

Perhaps we can provide separate ReadCompletion and WriteCompletion trait to avoid breaking the symmetry?

@nrc
Copy link
Owner Author

nrc commented Jul 19, 2022

The proposal in the document is to support completion systems via BufRead and possibly a new OwnedRead trait

@NobodyXu
Copy link

The proposal in the document is to support completion systems via BufRead and possibly a new OwnedRead trait

Sorry that I missed that part.

@Noah-Kennedy
Copy link

@NobodyXu when you say "provided buffer", are you referring to owned buffers or kernel-managed buffer groups?

@NobodyXu
Copy link

@NobodyXu when you say "provided buffer", are you referring to owned buffers or kernel-managed buffer groups?

I am referring to kernel-managed buffer groups.

@Noah-Kennedy
Copy link

I think that this is something which, while very, very important, is probably out of scope of current standardization efforts.

@NobodyXu
Copy link

I think that this is something which, while very, very important, is probably out of scope of current standardization efforts.

Hmmm yeah, I can see that the existing proposal is already quite complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stdlib Area: a standard library for async Rust
Projects
None yet
Development

No branches or pull requests

10 participants