Add AsyncRead::poll_read_buf and AsyncWrite::poll_write_buf

[taiki-e]

Closes #1818

[taiki-e]

I have no strong opinion about whether to block this with bytes 0.5-alpha.

[taiki-e]

cc @carllerche @seanmonstar @LucioFranco

[yoshuawuyts]

I don't think this is a right addition for futures-rs for several reasons. In this comment I try and explain why.

Direction for futures-rs

We have 29 days left until the next Rust beta which will include async/await without a feature flag. I think we should be using the coming weeks before 1.39 to focus on making futures-rs ready for a release. In my view this means moving away from the (somewhat) rapid pace of iteration and additions that we've enjoyed for the past while, and move more towards defining which parts we'd like to stabilize.

How this applies to tokio-rs/bytes

Part of moving towards a more stable futures-rs library is that it opens up the path for starting to move core traits into stdlib. Future, Poll, Waker et al have already been moved into the stdlib, and it's not unreasonable to consider Stream, AsyncRead and AsyncWrite finding its way into the standard library eventually too. If anything I'd like us to be able to have those conversations.

tokio-rs/bytes doesn't currently seem to be on a path towards moving into the stdlib. This means that whichever additions are made to stdlib, the exposure of bytes in the API cannot be part of it. But even before these traits are moved into the standard library, I think it'd be preferable (but not required) if they were somewhat stable in futures-rs beforehand.

Technical feedback on the patch

Somewhat related, but I'm not too thrilled about the naming proposed in this patch either. In the stlib methods with buf commonly refer to operating on [u8], which is different from the use in this patch.

Similary I noticed this is being added to futures-io directly, rather than being part of futures-util. This would be a problem in the context of #1688, which would mean bytes would become a direct dependency of futures-core, which looks like it would be against the purpose of the futures-core crate all together.

Solutions to tokio using futures-rs traits

However I'm sympathetic to tokio expressing the intent to using futures-io directly rather than exposing a fork. I don't think there are any great solutions available, but there are still a few options to make it so futures-io traits are used by tokio:

• Expose Ext traits for tokio-specific functionality -- this is similar to how futures-preview currently separates the core trait from the combinators. If a tokio::prelude would be introduced, this could make for a reasonably nice user experience and mostly solve the compat problems.
• In async-std we've taken a different route, and expose a generic trait bound between async_std::io::Write and futures::io::AsyncWrite. We tell people to only ever implement the futures::io traits in libraries, but they can import our traits in application code to use them.

It's hard for me to tell which approach would work best for tokio, but hopefully this provides a path forward to adopting the futures_rs::io traits in tokio.

Conclusion

In this post I've outlined why I don't think it makes sense to add bytes-specific methods to futures-rs because of overall crate direction, timeline, and technical reasons. And I've also shared alternative approaches that would allow tokio to still achieve the goals outlined in #1818.

I hope this was useful, albeit a bit lengthy. I figured I'd try and be as thorough as I could be. Thanks heaps for reading!

[LucioFranco]

@yoshuawuyts thank you for spending the time to write this up. I think there are some really good ideas here!

tokio-rs/bytes doesn't currently seem to be on a path towards moving into the stdlib. This means that whichever additions are made to stdlib, the exposure of bytes in the API cannot be part of it.

I think this is very reasonable and makes sense. 👍

However I'm sympathetic to tokio expressing the intent to using futures-io directly rather than exposing a fork.

Yes, this is something we really do want to achieve because I think this is and will become a large pain point throughout the ecosystem. The tough part is that we have certain requirements that we want to maintain and this has become hard once the io traits got moved out of tokio. Which is why we still have our own versions since we have not had the full opportunity to explore them yet in relation to new futures.

Expose Ext traits for tokio-specific functionality

I think this approach would work fine for our use case and maybe the correct approach here. Though I'd like to hear what @taiki-e has to say.

We have 29 days left until the next Rust beta which will include async/await without a feature flag. I think we should be using the coming weeks before 1.39 to focus on making futures-rs ready for a release.

Now circling back to this section. What are the plans around this? I don't think the ecosystem has had enough time to really explore all the traits besides std::future::Future and Stream? Do we have a solid and guided path for stabilizing futures? I personally have not seen much around this and of course, I would like to help move this forward. Since this was used as a point to not include this PR I'm curious what other thoughts you might have around stabilizing traits? I don't think we will see io traits within std anytime soon since they are still kind of new in relation to other runtimes etc. I'm happy to move this to another issue but thought I might as well bring it up and maybe its something we can discuss in today's async wg meeting?

Again, thanks for the write-up and I do hope soon we can all live on the same traits 😄

[LucioFranco]

So another thing related to the extension trait that I totally forgot was that we can no longer specialize implementations with that method. Meaning we can't override the poll fn. This is something we would like to do but I'm not sure how we can support that without this. 😞

[cramertj]

@LucioFranco can you give more information about the kinds of overriding implementations you'd like to provide that the current system doesn't support? Maybe we can find another way to make those work nicely.

[carllerche]

@cramertj one use case is that the caller can avoid going through the IoSlice path if the read / write implementations do not support it. This is useful when the caller has a complex chain of buffers.

Edit: and besides just a binary "supported" or not, it also allows the AsyncRead / AsyncWrite implementation to decide the size of the IoSlice

[cramertj]

it also allows the AsyncRead / AsyncWrite implementation to decide the size of the IoSlice

We discussed this before, and I still don't think i ever quite understood why this is something you'd want-- Read implementations don't get to decide the size of the buffer provided to them, so I wouldn't expect AsyncRead to do anything different.

[cramertj]

I think that's the basic crux of my argument here-- I'm not opposed to adding this as an unstable optional feature, but I'm opposed to ever having functions on AsyncRead and AsyncWrite that people rely on which we wouldn't expect to see if/when these traits are moved into the standard library.

[LucioFranco]

I'm opposed to ever having functions on AsyncRead and AsyncWrite that people rely on which we wouldn't expect to see if/when these traits are moved into the standard library.

I think this is a perfectly valid thing and I actually agree! But this just leads me to think that these traits may not be ready for std yet? I'm happy to help think through this further and I should be attending the next foundation meetings.

So from that, I think it may make sense to close this or throw it behind unstable.

[cramertj]

I think this is a perfectly valid thing and I actually agree! But this just leads me to think that these traits may not be ready for std yet?

I'm not sure how that conclusion relates-- these traits seem fine to me, and correspond pretty precisely to std::io::Read/Write. This PR would make them no longer match, which concerns me.

[LucioFranco]

Sure, then I say we close this and we can explore, for now, in our own traits.