Merge futures-io into futures-core

[taiki-e]

• Consider flattening the crate and dropping -preview status #1356 (comment)

  It could make sense to reduce the number of crates drastically though. Maybe even as far as just 3 crates: futures-core as the almost 100% stable core, futures for everything that may require breaking changes and futures-test for testing specific utilities. Basically I guess that would be merging futures-io into futures-core (is it stable enough for this now?) and everything except futures-test directly into futures.

• https://rust-lang.zulipchat.com/#narrow/stream/187312-wg-async-foundations/topic/futures.20crate%28s%29/near/167501758

  futures-core and futures-io are meant to represent a subset that is stable enough for folks to rely on in public API boundaries

I agree that these are almost stable.

What I was thinking of as a possible change in the future was that the <'a> of AsyncBufRead would become unnecessary by rust-lang/rust#61207 (or alternative) being merged. EDIT: See #1688 (comment)

Another change I'm thinking about is to add poll_{read,write}_buf like tokio's same name methods (poll_read_buf, poll_write_buf). This makes tokio::io::Async* and futures::io::Async* almost identical, so that tokio probably can use the futures's io traits. (Stabilizing this is probably blocked by bytes 0.5 release, but since poll_{read,write}_buf have default implementations, it could be made to work well by adding a feature gate. )
If tokio team is interested in this... cc @carllerche @seanmonstar @LucioFranco @stjepang

r? @cramertj
cc @Nemo157 @yoshuawuyts

[Nemo157]

What I was thinking of as a possible change in the future was that the <'a> of AsyncBufRead would become unnecessary by rust-lang/rust#61207 (or alternative) being merged.

Since that's just elision it should be backwards compatible, implementors are allowed to explicitly fill in the elided lifetimes.

[taiki-e]

@Nemo157
Indeed. Thanks for pointing that out!

[LucioFranco]

@taiki-e this seems nice but I think we are still kinda experimenting with what the right api is within tokio-io, so maybe its right to hold off a bit once we've converted tokio and then make a decision then.

[cramertj]

@LucioFranco note that unlike tokio-io, futures-io does not include any combinators, so this is just making a promise about the signatures of the user-implemented methods. The combinators (which are more prone to churn) are versioned separately as part of futures-util.

[seanmonstar]

(I'd personally wish this started as an issue, just so I don't feel bad about the work you've already done ;_;)

note that unlike tokio-io, futures-io does not include any combinators, so this is just making a promise about the signatures of the user-implemented methods.

We've been thinking specifically about the required methods of AsyncRead and AsyncWrite, and wondering if they have issues. For instance, several methods provide optimizations, and it's very easy for a wrapper to forget to forward them, losing out on those optimizations. I've fallen prey to this a few times.

Also, have AsyncBufRead and AsyncSeek seen much use? They haven't really been available in a "more stable" release. From personal experience, I tried using std::io::BufRead a few years ago, and found it too limiting, and so stopped using it. It looks like AsyncBufRead has the same limitations...

My personal opinion is that the traits in futures-io aren't quite as stable as Stream. Does it hurt that much keeping them in futures-io for a while after Future and async/await are stable?

[Nemo157]

Also, have AsyncBufRead and AsyncSeek seen much use? They haven't really been available in a "more stable" release. From personal experience, I tried using std::io::BufRead a few years ago, and found it too limiting, and so stopped using it. It looks like AsyncBufRead has the same limitations...

I just migrated AsyncReadExt::copy_into from using an internal buffer to being built on top of AsyncBufReadExt::copy_buf_into using the BufReader adaptor. I find the main copy loop built on AsyncBufRead much easier to read than the one built on AsyncRead. Similarly AsyncBufRead was a very good fit for writing a wrapper to apply compression to an async byte stream. (I don't know how the trait fairs for implementors, but it seems like a decent API from the consumer side).

[yoshuawuyts]

In general this seems like a good idea! The futures::io::* traits see a lot of use, so making them conveniently available seems like it would be beneficial to many!

My only note here is that we've been experimenting with the signatures of the IO traits, and it seems that it would be possible to define them using async fn directly!

pub trait AsyncRead {
    async fn read(&mut self, buf: &mut [u8]) -> io::Result<usize>;
}

pub trait AsyncWrite {
    async fn write(&mut self, buf: &[u8]) -> io::Result<usize>;
}

pub trait Stream {
    type Item;
    async fn next(&mut self) -> Option<Self::Item>;
}

That's why I'd like to ask: what stability guarantees do we intend to provide?

It seems that even though io traits might make it into futures-core, we would want to reserve the rights to make breaking changes to APIs as we move async/await beyond an MVP state, and will gradually unlock new capabilities that will make it possible to write more ergonomic traits.

[cramertj]

It is not possible to define those traits in that fashion. There is no plan for async fn that would allow making those traits object-safe without allocation.

[yoshuawuyts]

@cramertj my understanding was that allowing such an API may be possible, but would require overcoming challenges similar to GATs. Discussing the merits of unallocated dyn async fns probably requires more time than we should probably dedicate right now.

Which brings me back to my question: does accepting this PR effectively mean we're locked into a set of APIs that cannot be changed?

[cramertj]

my understanding was that allowing such an API may be possible, but would require overcoming challenges similar to GATs

I've never heard a proposal that would work-- GATs aren't related to the issue here. The closest is my max-sized dyn-by-value thing, but that's a wacky hack that would require us specifying an upper bound on the size of the async body that implemented read before deferring to allocation, and would require that we always save a slot in the generator of that size when calling dynamically-dispatched read. This is also >3 years away from being implementable even if we decided to do it, which there would likely be a lot of opposition to.

does accepting this PR effectively mean we're locked into a set of APIs that cannot be changed?

No, it only means that the current versions of the Async{IoType} traits are versioned alongside the Stream trait, which itself is moderately unstable due to GATs. (I would actually argue the reverse problem here-- futures-io could drop its dep on futures-core to avoid being tied to Stream).

[yoshuawuyts]

LGTM!

[kpp]

My only note here is that we've been experimenting with the signatures of the IO traits, and it seems that it would be possible to define them using async fn directly!

@yoshuawuyts how about

pub async trait AsyncRead {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize>;
}

pub async trait AsyncWrite {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize>;
}

pub async trait Stream {
    type Item;
    fn next(&mut self) -> Option<Self::Item>;
}

(Sorry for offtop)

[yoshuawuyts]

@kpp I don't think that'd work; traits should be able to mix sync + async methods, which wouldn't be possible if the whole trait is marked as async.

[cramertj]

The discussion has stalled here and at this point I don't see much motivation to make a change like this, so closing. Feel free to reopen an issue for discussion!