Upgrade to hyper 0.12 (WIP) #1008

schrieveslaach · 2019-05-19T17:08:24Z

In order to support websockets, it is necessary to upgrade to the newest hyper version (see this comment in #90). This PR is a simple proof of concept to upgrade to hyper 0.12 which was a bit difficult and I hope, that we can use this PR as a WIP PR to upgrade to hyper 0.12.

I hope that you can support me to finish the upgrade.

jebrosen · 2019-05-19T18:32:42Z

Seeing this open I realize I should have shared my progress sooner. I have an async fork of Rocket that supports async routes, FromData, and (IIRC) Response and Body but still has some serious blockers before a thorough review can be done. It looks like this PR leaves out a few key things, but I might not be able to do a real comparison for a while still.

schrieveslaach · 2019-05-20T06:50:35Z

@jebrosen, I didn't know that you are working on an update as well. BTW, it sounds very nice what you are doing.

Do you think we should continue this PR and should use your insights to finish this update? This PR has some issue in it and Rocket could benefit from your and others point of view. Based on the upgrade Rocket could start integrate async routes, etc.

SergioBenitez · 2019-05-20T20:36:28Z

@jebrosen @schrieveslaach Can I task you with working together to create a comprehensive PR? We should do this in the open to avoid duplicating efforts in the future. Perhaps we can start by having you share your work, @jebrosen, and perhaps you can take the lead to incorporate @schrieveslaach contributions into your ongoing effort.

jebrosen · 2019-05-20T22:47:07Z

I have rebased and updated the comments on my branch: https://github.com/SergioBenitez/Rocket/compare/master...jebrosen:async?expand=1. I have been working on it on and off for several months now, but churn around futures_api and await_macro made it a bit challenging to power through.

Here's what I was thinking, roughly in order:

Update at least core and codegen, but probably all crates in Rocket to the 2018 edition. I personally think it would be a good idea to also add #![warn(rust_2018_idioms)]. This is needed for the async_await feature which is a significant benefit for Rocket and crates that use it. cargo fix --edition works mostly fine for this task.
Combine the efforts of this PR with what I have worked on. I skimmed this PR and I think my work includes (in some form) most of these changes and more, with the exception of how MakeService is handled. I should be able to read though these changes more carefully this weekend at the latest and see if I missed anything else.
There are a few things in core that are unfinished and need to be worked on:
- Cookies now uses a weird Mutex hack that I'm pretty sure is wrong. I just did it to get past the errors and work on fixing other more interesting things
- Some low-level things including keep-alive, closing the data stream on drop, and retrieving the client address of an incoming connection
- Everything to do with Client, LocalRequest, etc. used everywhere in tests.
Response/Body needs some design work. As I have written it, all responses are buffered into a Vec before being sent to the client. This is extremely wrong (but at least it does it asynchronously! 😄)
- We additionally want to provide an AsyncWrite-like interface in particular. Hyper has Body::channel, which we probably don't want to use directly long-term but might be usable to hash out design or serve as some inspiration for something better.
Work on making the API nicer. I am tired of repeatedly typing out things like Pin<Box<Future<Output = T> + Send + 'a>>, for example.
Fix rocket_contrib and examples.

... and more after that. I have more notes and ideas of things that need to be done, but I have to leave it at this for right now.

schrieveslaach · 2019-05-22T06:11:14Z

@SergioBenitez, sure. 😃

@jebrosen, thanks for your comments.
In my PR I tried to make as less breaking changes as possible but I noticed very quickly that the buffering of the request body was necessary for this. 🙈

How do you propose to continue? Should I wait for your comments and than I should start to adopt this PR?

jebrosen · 2019-05-23T23:49:10Z

If you have the time and interest, I would appreciate if you could take a look at my work and share your opinions on anything we both did but in different ways. I'll do the same, but that might be a few days away still and this would help me know what to watch out for.

I will want to be careful about merging our code together, maybe in an "async" branch in this repository. You deserve credit for starting this, and I also want to keep the other work I've done.

schrieveslaach · 2019-05-24T05:36:43Z

Okay, I will have a look at your work.

You deserve credit for starting this, and I also want to keep the other work I've done.

Thanks for your appreciation. I agree with you that we should incorporate our development branches.

jebrosen

Reviewing this took less time than I thought; the large deletions and simple renames and TODOs inflated the numbers and tricked me into thinking it would take longer.

Overall, I think there are only a few things here that I haven't also done -- keeping From<Mime> for ContentType is one example and probably a good idea. I also like From<Response> for hyper::Response.

I think what I would like to do is 1) get Rocket master on 2018, 2) adopt your commit for most/all of the changes in http and some of the changes in rocket.rs, and 3) layer my work on top, and make that the async branch. Then we can re-assess what work remains to be done.

jebrosen · 2019-05-25T17:03:18Z

core/http/src/content_type.rs

@@ -281,11 +281,11 @@ impl From<Mime> for ContentType {
    #[inline]
    fn from(mime: Mime) -> ContentType {
        // soooo inefficient.
-        let params = mime.2.into_iter()


I had dropped mime support entirely.

From your comment above, I think you want to keep mime support, correct?

Yes, if there's no downside, at least. I think it would just add non-async-related churn to an async development branch for now.

jebrosen · 2019-05-25T17:03:45Z

core/http/Cargo.toml

@@ -21,7 +21,9 @@ private-cookies = ["cookie/secure"]
 [dependencies]
 smallvec = "0.6"
 percent-encoding = "1"
-hyper = { version = "0.10.13", default-features = false }
+hyper = { version = "0.12.28", default-features = false, features = ["runtime"] }


I chose to use tokio manually instead of via the runtime feature (for easier TLS handling, I think).

Okay, I can change this as well.

Feel free to change it in your branch, or not - I foresee a big project of a merge one of the coming weekends either way!

@jebrosen One thing to keep in mind is that people will definitely want to run stuff other than Rocket on the Tokio event loop (e.g. async database calls using tokio-postgres). So either:

Rocket should (optionally) accept a tokio reactor to run on

Rocket should be able to expose it's own reactor for use by other code

The tokio runtime should be used (I think this allows for this).

Probably #1 with an option for #3 would be ideal?

IIRC hyper 0.12 actually requires tokio runtime specifically (hyper 0.13 might not.) I definitely like the idea of allowing a user-configured runtime, though.

jebrosen · 2019-05-25T17:16:36Z

core/lib/src/rocket.rs

+    fairings: Arc<Fairings>,
+}
+
+impl<Ctx> hyper::MakeService<Ctx> for Rocket {


This is one of the key structural differences between our approaches - I used hyper::service_fn as in jebrosen@27a59ef#diff-a9ea99dbf2f3bc38a7d7e4c36a99a288R786. Do you have an opinion on which is better? I prefer hyper::service_fn because it's far shorter, but is there a benefit to explicitly specifying the associated types by implementing MakeService manually?

I had some struggle with hyper::service_fn and the idea of using the MakeService trait had seemed to me very handy and more robust against future changes in hyper. But that's only a gut feeling. Nevertheless, I think we have to use some kind of cloning of Arc<SomeRocketStruct> here. I use it in RocketHyperService, you use it in hyper::service_fn.

crusadergo · 2019-05-25T18:09:31Z

really looking forward when this MR will be ready :)

jhpratt · 2019-06-25T21:41:12Z

I probably wouldn't be of much help in porting the code to 0.12, but I can certainly take a look over code before merging if wanted.

SergioBenitez · 2019-06-26T13:08:48Z

Rocket is now on Rust 2018! Looking forward to seeing progress here.

@jebrosen What are the next steps?

jebrosen · 2019-06-26T14:42:51Z

@schrieveslaach, do you have any changes you made since this PR was opened?

The next step on my list is to rebase this PR on top of master, then my changes on top of that. That should give us master -> partial hyper 0.12 upgrade -> full async/await in core and codegen. Since it's a massive and incomplete breaking change that breaks tests left and right, I had the idea to start an async branch that can be pushed to / have PRs opened on.

Ideally we would have a way to divvy up tasks so there's not too much duplication of work and interested parties could pitch in -- @SergioBenitez do you have any opinions here as far as a tracking issue(s) or PR submission/review?

schrieveslaach · 2019-06-27T06:26:34Z

@jebrosen, @SergioBenitez, I'll have time on sunday and I'll work on it again. I keep you posted.

- Use hyper's MakeService implementation with futures API - Use tokio runtime to serve HTTP backend

schrieveslaach · 2019-06-30T14:52:47Z

@jebrosen, @SergioBenitez, I rebased my fork with the current master and I improved my code a bit. For example, I restored some code that I had to disable.

I also switched to tokio runtime. I used the code @jebrosen's fork.

This PR still uses MakeService with some helper structs (eg. RocketArcs and RocketHyperService). Do you think we should keep this way or should I switch to hyper::service_fn?

jebrosen · 2019-06-30T16:22:16Z

I will start working on merging these together now. I will likely make a few changes - e.g. using a single Arc<Rocket> instead of individual Arcs, so that it will be easier to combine them. But this looks great so far.

schrieveslaach · 2019-07-01T06:53:19Z

@jebrosen, I'm looking forward to get this done.

Just out of curiosity: do you create an async branch and do you accept this PR by merging it into the async branch?

apiraino · 2019-07-01T07:47:42Z

I also switched to tokio runtime. I used the code @jebrosen's fork

Just asking out of curiosity: why did you both end up choosing tokio instead of trying something else (e.g. Runtime)? Perhaps was the async story already started at that time and you just restarted from there?

Me too, I'm trying to figure out the "right" library to use and I find tokio pretty hard to use (disclaimer: not an experienced Rustacean).

Anyways, thank you so much for pushing this forward, I'm really looking forward to it 👍

lnicola · 2019-07-01T07:57:14Z

runtime is a very thin wrapper over tokio and romio, the latter being more of an experiment in API design for the new version of futures. Now tokio is moving to the standard library futures, and it's designed to offer much better performance and functionality than the alternatives (of which there aren't many, as I mentioned). So there's no reason not to use it.

In any case, the choice will be more or less invisible to Rocket users.

apiraino · 2019-07-01T08:10:45Z

awesome @lnicola thanks for explaining! As long as the async story is ergonomic to use for "the rest of us", I'm super happy with whatever choice ;-)

jebrosen · 2019-07-01T15:17:21Z

@schrieveslaach My async branch now includes your commit, but I still want to go back and re-resolve a few merges with "yours" instead of "mine".

I originally went with tokio because hyper uses it by default. Somehow I ended up thinking that hyper even depended specifically on tokio, which I don't think is the case. Assuming it works, we should allow the use of any runtime. And yes, for the vast majority of use cases the specific underlying runtime should not need to be worried about.

jebrosen · 2019-07-10T15:25:39Z

Update: I got busy with a few other things, bit I plan to come back to this very soon. Thank you everyone for your patience.

My async branch is pretty close to being a fully async/await API, but some features are still unimplemented or seriously unvetted. I also want to take a look at the master branch of hyper, which is now using futures 0.3 and might let us drop all uses of Compat.

jebrosen · 2019-08-02T01:10:47Z

I've merged this and my code as the async branch and opened #1065. Let's continue work starting there.

jebrosen reviewed May 25, 2019

View reviewed changes

This was referenced May 25, 2019

Rust 2018 #1013

Closed

Connection aborted with streams and request guards #892

Closed

jebrosen mentioned this pull request Jun 2, 2019

Performance problem with the new version of Rocket #1023

Closed

jebrosen mentioned this pull request Jun 24, 2019

Strange Results in The Benchmarker #1038

Closed

nicoburns mentioned this pull request Jun 25, 2019

Stabilize HTTP library #17

Closed

Upgrade to hyper 0.12:

751903e

- Use hyper's MakeService implementation with futures API - Use tokio runtime to serve HTTP backend

jebrosen mentioned this pull request Jul 20, 2019

Documentation about threading #1055

Closed

jebrosen mentioned this pull request Aug 2, 2019

Tracking Issue for Async I/O migration #1065

Closed

15 tasks

jebrosen closed this Aug 22, 2019

jebrosen added the pr: merged This pull request was merged manually. label Aug 22, 2019

fenhl mentioned this pull request Aug 30, 2019

Native WebSocket support #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to hyper 0.12 (WIP) #1008

Upgrade to hyper 0.12 (WIP) #1008

schrieveslaach commented May 19, 2019

jebrosen commented May 19, 2019

schrieveslaach commented May 20, 2019

SergioBenitez commented May 20, 2019

jebrosen commented May 20, 2019

schrieveslaach commented May 22, 2019

jebrosen commented May 23, 2019

schrieveslaach commented May 24, 2019

jebrosen left a comment

jebrosen May 25, 2019

schrieveslaach May 28, 2019

jebrosen May 29, 2019

jebrosen May 25, 2019

schrieveslaach May 28, 2019

jebrosen May 29, 2019

nicoburns Jun 25, 2019

jebrosen Jun 26, 2019

jebrosen May 25, 2019

schrieveslaach May 28, 2019 •

edited

Loading

crusadergo commented May 25, 2019

jhpratt commented Jun 25, 2019

SergioBenitez commented Jun 26, 2019

jebrosen commented Jun 26, 2019

schrieveslaach commented Jun 27, 2019

schrieveslaach commented Jun 30, 2019

jebrosen commented Jun 30, 2019

schrieveslaach commented Jul 1, 2019

apiraino commented Jul 1, 2019

lnicola commented Jul 1, 2019

apiraino commented Jul 1, 2019

jebrosen commented Jul 1, 2019

jebrosen commented Jul 10, 2019

jebrosen commented Aug 2, 2019

Upgrade to hyper 0.12 (WIP) #1008

Upgrade to hyper 0.12 (WIP) #1008

Conversation

schrieveslaach commented May 19, 2019

jebrosen commented May 19, 2019

schrieveslaach commented May 20, 2019

SergioBenitez commented May 20, 2019

jebrosen commented May 20, 2019

schrieveslaach commented May 22, 2019

jebrosen commented May 23, 2019

schrieveslaach commented May 24, 2019

jebrosen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schrieveslaach May 28, 2019 • edited Loading

Choose a reason for hiding this comment

crusadergo commented May 25, 2019

jhpratt commented Jun 25, 2019

SergioBenitez commented Jun 26, 2019

jebrosen commented Jun 26, 2019

schrieveslaach commented Jun 27, 2019

schrieveslaach commented Jun 30, 2019

jebrosen commented Jun 30, 2019

schrieveslaach commented Jul 1, 2019

apiraino commented Jul 1, 2019

lnicola commented Jul 1, 2019

apiraino commented Jul 1, 2019

jebrosen commented Jul 1, 2019

jebrosen commented Jul 10, 2019

jebrosen commented Aug 2, 2019

schrieveslaach May 28, 2019 •

edited

Loading