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

Upgrade to hyper 0.12 (WIP) #1008

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@schrieveslaach
Copy link

commented May 19, 2019

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

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2019

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

This comment has been minimized.

Copy link
Author

commented May 20, 2019

@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

This comment has been minimized.

Copy link
Owner

commented May 20, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

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:

  1. 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.
  2. 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.
  3. 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.
  4. 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.
  5. Work on making the API nicer. I am tired of repeatedly typing out things like Pin<Box<Future<Output = T> + Send + 'a>>, for example.
  6. 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

This comment has been minimized.

Copy link
Author

commented May 22, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

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

This comment has been minimized.

Copy link
Author

commented May 24, 2019

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

left a comment

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.

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

This comment has been minimized.

Copy link
@jebrosen

jebrosen May 25, 2019

Collaborator

I had dropped mime support entirely.

This comment has been minimized.

Copy link
@schrieveslaach

schrieveslaach May 28, 2019

Author

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

This comment has been minimized.

Copy link
@jebrosen

jebrosen May 29, 2019

Collaborator

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.

@@ -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"] }

This comment has been minimized.

Copy link
@jebrosen

jebrosen May 25, 2019

Collaborator

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

This comment has been minimized.

Copy link
@schrieveslaach

schrieveslaach May 28, 2019

Author

Okay, I can change this as well.

This comment has been minimized.

Copy link
@jebrosen

jebrosen May 29, 2019

Collaborator

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!

This comment has been minimized.

Copy link
@nicoburns

nicoburns Jun 25, 2019

@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:

  1. Rocket should (optionally) accept a tokio reactor to run on
  2. Rocket should be able to expose it's own reactor for use by other code
  3. The tokio runtime should be used (I think this allows for this).

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

fairings: Arc<Fairings>,
}

impl<Ctx> hyper::MakeService<Ctx> for Rocket {

This comment has been minimized.

Copy link
@jebrosen

jebrosen May 25, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@schrieveslaach

schrieveslaach May 28, 2019

Author

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

This comment has been minimized.

Copy link

commented May 25, 2019

really looking forward when this MR will be ready :)

@jhpratt

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.