hyperium/hyper

Updating to std::future::Future

seanmonstar opened this issue · 17 comments

seanmonstar commented

With std::future::Future stabilizing in Rust 1.36, it is a goal to upgrade from futures@0.1 to std::future::Future in the next breaking change (hyper@0.13).

Status:

  • Update hyper lib to std::future #1836
  • Update doc tests to std::future #1850
  • Update examples to std::future #1849
  • Update tests to std::future #1848
  • Update h2 to std::future #1851
davidbarsky commented

I might be missing something, but is there a tracking issue on Tower to support 0.3 futures?

dekellum commented

You say you are just considering it, but would TLS (Thread Local Storage, I presume) play well with Tokio's multi-threaded executor and, for example with conditional calls to tokio_threadpool::blocking in (Body) Streams and Sinks?

seanmonstar commented

That should be fine. When calling poll methods, we'd still need to materialize the Context before doing so, so there'd be no relying on other libraries keeping things on the same thread. It's more just wanting to reduce the noise of having to pass a Context to many methods of proto::h1::Conn that don't always need it, but might poll the IO.

  • 👍1
seanmonstar commented

Most of the dependencies needed are updated in tokio's std-future branch. Disabling tokio-threadpool and h2 should hopefully be enough to start a branch here.

It's probably not worth figuring out some TLS Context thingy, and just passing the argument everywhere.

  • 🎉8
seanmonstar commented

#1836 is the PR for this, it's pretty close. Most things work (minus h2), and I have some local work to remove much of added unsafety, which I hope to have ready Monday. I'll likely merge that into master with docs and tests disabled, to allow those things to be worked on in parallel.

  • ❤️6
  • 🚀4
seanmonstar commented

Master is now tracking 0.13, and #1836 was merged into it. It lacks HTTP2 support, an unsafe pin audit, and all the tests and examples were disabled, but hopefully those can be worked on in parallel.

  • ❤️8
jplatte commented

Thanks everyone for working on this! I'm a bit confused about one thing though - is the move to std::future::Future intentionally also a move to async / await? I was assuming I could build a library with MSRV 1.36 that's going to be using hyper 0.13, but a bunch of code has already been committed that relies on #![feature(async_await)] instead of only the Future trait. Is it just too much hassle to convert to the new Future first, and start using async / await later? (which should purely be an implementation detail)

lnicola commented

tokio 0.2 uses await AFAIK.

jplatte commented

Thanks. I asked on the tokio Gitter channel; apparently they want to only release 0.2 once async_await is stabilized.

  • 👍1
abonander commented

@seanmonstar are there plans to migrate Body to AsyncRead instead of being a Stream<Item = Chunk>? If it uses AsyncRead I can add async implementations to multipart and abandon multipart-async. I'm wondering if I should maintain two different crates anyway as the ecosystem seems to be migrating wholesale to async.

davidbarsky commented

@abonander Might be a tricky tricky to do so for a few reasons:

  • Body isn't really a Stream<Item = Chunk>, it's a Payload.
  • AsyncRead doesn't—to the best of my knowledge—have the equivalent of Payload::poll_trailers, which is needed H/2 connections. The closest thing seems to be http-body, but that isn't AsyncRead.

Personally, I'd love to have a shared interface/trait at the moment for the reasons you outlined, but I'm not confident any of the shared interfaces—HttpBody or AsyncRead—tick off all the requirements someone might have. Maybe a blanket implementation for either trait could work/bridge some gaps in the interface?

abonander commented

@davidbarsky I mostly meant if there were plans to add the impl even though I used "instead of". I didn't mean removing any other API.

davidbarsky commented

@abonander Ah, sorry about that; I misread what you said. Maybe? I'm not 100% sure how that could be implemented without needing specialization, but I might be missing something!

abonander commented

@davidbarsky how would specialization be necessary? There's only one possible impl, Body isn't generic. Digging through the code it looks like it uses AsyncRead underneath and then switches to producing Bytes in proto::h1::io.

seanmonstar commented

@abonander

Are there plans to migrate Body to AsyncRead instead of being a Stream<Item = Chunk>?
[..]
I mostly meant if there were plans to add the impl even though I used "instead of". I didn't mean removing any other API.

No plans so far... Here's some reasons of the top of my head why not to:

  1. The Body cannot freely implement Stream and AsyncRead. In order to provide AsyncRead, it would need to add a slot to cache a Chunk, in case the AsyncRead::poll_read buffer wasn't big enough to copy the full thing. This would make the struct bigger and add another branch to Stream::poll_next.
  2. The Stream impl can include more specific errors. I suppose an AsyncRead could just translate those to std::io::ErrorKind::Other, though...

If there are good reasons to consider otherwise, we should open a new specific issue to discuss, ya?

seanmonstar commented

All the original "features" on master now use std::future, so I'm going to close this. Some tests and docs still need updating, thought.

  • 🎉8
  • 🚀4