Should AsyncRead/AsyncWrite required pinned self?

[Diggsey]

This is the current definition of AsyncRead (omitting provided methods):

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

Pin guarantees that the value will not be moved until it is dropped. For futures, this makes a lot of sense because once a future yields its value, it no longer has any use and can be dropped.

However, for AsyncRead/AsyncWrite types, I think the guarantee Pin makes is too strong.

We'll eventually want higher level APIs like these either on this trait, or on an extension trait:

fn read<'a>(&'a mut self, buf: &'a mut [u8]) -> impl Future<Output=io::Result<usize>> + 'a;
fn read_exact<'a>(&'a mut self, buf: &'a mut [u8]) -> impl Future<Output=io::Result<()>> + 'a;

But unless all AsyncRead/AsyncWrite implementations are Unpin, there will be no way for these methods to ensure that Self is pinned from within the future implementation.

The guarantee should be that Self is pinned whilst the asynchronous operation (which ends with the destruction of the returned future) is underway, not that Self is pinned forever.

One way to solve this problem would be to introduce a more powerful Future trait, eg.

trait FutureRead {
    type Output;
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut [u8]) -> Poll<Self::Output>;
}

And then have AsyncRead expose this method instead of poll_read:

fn read<'a>(&'a mut self) -> impl FutureRead<Output=io::Result<usize>> + 'a;

This still allows the caller to avoid keeping around a persistent buffer just in case there is data to be read, but it can also be safely adapted into an API based on std::future::Future at the cost of using a persistent buffer:

trait FutureRead {
    type Output;
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut [u8]) -> Poll<Self::Output>;
    fn into_future<'a>(self, persistent_buf: &'a mut [u8]) -> impl Future<Output=Self::Output> + 'a {
        PersistentFutureRead(self, buf)
    }

There is an additional benefit too: if the underlying implementation relies on there being a persistent buffer anyway (as I believe is the case on windows?) then the implementation could override the default "into_future" method to have the OS write directly into the provided buffer.

[taiki-e]

Refs: rust-lang/futures-rs#1454

[carllerche]

I believe the argument is that rust will gain support for async generators, and at that point AsyncRead could be implemented using async fns.

I'm personally hesitant to forward proof at this point and would be include to switch AsyncRead / AsyncWrite back to &mut self and re-evaluate once sufficient capabilities have landed in stable Rust and there is more experience using async generators.

At some point in the future, if it becomes obvious that these traits should take Pin, then we can issue a breaking change. Until then, there always is the option of boxing.

[carllerche]

cc @seanmonstar

[jimblandy]

After rust-lang/futures-rs#1454, the status quo in futures 0.3 is that:

• AsyncWrite::poll_write takes Pin<&mut Self>, but
• AsyncWriteExt helper methods like write_all and friends take &mut Self where Self: Unpin.

This is a weird compromise where:

• Functions generic over any T: AsyncWrite must actually write T: AsyncWrite + Unpin if they want to use the AsyncWriteExt functions, which of course everyone does. Naturally, that additional constraint has to be propagated as far as the value is being passed, so Unpin appears everywhere.

• Although it is possible for objects built with unboxed async fn futures to implement AsyncWrite (one of the goals of the compromise), you can't use the AsyncWriteExt methods on such objects, which is weird.

• More generally, you can't explain AsyncWriteExt to people as a simple extension trait, since it effectively adds the Unpin constraint. So documenting it (and just remembering how to use it) gets harder.

Using unboxed async fn futures sounds good, but it seems like we're creating a confusing situation that isn't really satisfying to anyone.

Of course, reconciling definitions of AsyncRead and AsyncWrite across the community is an important goal, so maybe the best resolution for this issue is to give up on elegance and follow the futures crate's lead.

[taiki-e]

• AsyncWriteExt helper methods like write_all and friends take &mut Self where Self: Unpin.

Many AsyncWriteExt methods don't take self because it may not actually be written to the underlying writer until flush is called. (AsyncRead is not, so it's probably ok to change some of the methods that currently take &mut self to take self. However, this is breaking change.)

And, afaik, you need boxing or Unpin bounds only if you still need ownership after you call that. Otherwise, you can use stack-pinning.

[Darksonn]

Note that the tokio::pin! and futures::pin_mut! macros can be used to avoid the Unpin requirement. Perhaps we need better documentation regarding this?