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

Updating to std::future::Future #1805

Closed
4 of 5 tasks
seanmonstar opened this issue Apr 30, 2019 · 17 comments
Closed
4 of 5 tasks

Updating to std::future::Future #1805

seanmonstar opened this issue Apr 30, 2019 · 17 comments
Labels
B-upstream Blocked: needs a change in a dependency or the compiler. C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Milestone

Comments

@seanmonstar
Copy link
Member

seanmonstar commented Apr 30, 2019

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:

@seanmonstar seanmonstar added B-upstream Blocked: needs a change in a dependency or the compiler. C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work. labels Apr 30, 2019
@seanmonstar seanmonstar added this to the 0.13 milestone Apr 30, 2019
@davidbarsky
Copy link
Contributor

davidbarsky commented Apr 30, 2019

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

@dekellum
Copy link

dekellum commented May 22, 2019

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
Copy link
Member Author

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.

@seanmonstar
Copy link
Member Author

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.

@seanmonstar seanmonstar mentioned this issue Jun 27, 2019
15 tasks
@softprops
Copy link
Contributor

https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html

@seanmonstar
Copy link
Member Author

#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.

@seanmonstar
Copy link
Member Author

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.

@jplatte
Copy link
Contributor

jplatte commented Jul 19, 2019

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
Copy link
Contributor

lnicola commented Jul 19, 2019

tokio 0.2 uses await AFAIK.

@jplatte
Copy link
Contributor

jplatte commented Jul 19, 2019

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

@abonander
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member Author

seanmonstar commented Aug 14, 2019

@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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-upstream Blocked: needs a change in a dependency or the compiler. C-feature Category: feature. This is adding a new feature. E-hard Effort: hard. Likely requires a deeper understanding of how hyper's internals work.
Projects
None yet
Development

No branches or pull requests

7 participants