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

Update hyper/tokio/futures dependencies #3214

Merged
merged 10 commits into from
Feb 18, 2020

Conversation

jaspervdm
Copy link
Contributor

@jaspervdm jaspervdm commented Jan 30, 2020

In preparation for #3206, update the hyper, tokio and futures dependencies to versions supporting async/.await. This simplifies some of our code and in some places reduces the number of heap allocations that are done.

There are some improvements that can be made to the different services we are running (p2p, api, stratum, webhook). Particularly, it would be nice to have all of them under a common runtime (instead of creating a multi-threaded runtime for each of them) and add clean shutdown to all of them. But this is something I'm planning on tackling in a separate PR in the future.

@jaspervdm jaspervdm marked this pull request as ready for review January 31, 2020 21:48
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this a quick look through and 👍 on tackling this and the overall approach.
Let me take some time today and over the next couple of days to have a closer look and do some testing locally.

Response::builder()
.status(self.0)
let code = self.0;
Box::pin(async move {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for using pin here? I get what pin does in general terms but I have no experience with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find exactly in the hyper code where the requirement is placed, but it seems like the hyper server requires the future to implement Unpin. As an exercise, i replaced every instance of Pin<Box<..>> with a regular Box<..> and it gives the following trait bound error:

   --> api/src/rest.rs:207:39
    |
207 | let server = Server::bind(&addr).serve(make_service_fn(move |_| {
    |                                  ^^^^^ the trait `std::marker::Unpin` is not implemented for `(dyn core::future::future::Future<Output = std::result::Result<http::response::Response<hyper::body::body::Body>, hyper::error::Error>> + std::marker::Send + 'static)`

Since some of our futures have borrows across await points, they have self-referential data which means it no longer implements Unpin; it is not safe to move the data after polling. So we have to pin the Box. A Box actually implements Unpin since moving it around simply means moving the pointer, not the actual data it points to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. 👍

I did not realize this was something we needed for it to work cleanly with hyper. I assumed it was for our own reasons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of amazing we can rely on rust compile time checking for this and not need to rely on extensive testing to ensure we did not break anything at a really low level here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not realize this was something we needed for it to work cleanly with hyper. I assumed it was for our own reasons.

I'm not to clear on why exactly hyper requires it, but it seems that some combinators (like select) in the futures crate also require it.

Kind of amazing we can rely on rust compile time checking for this

Yes, one of my favorite things about Rust is the very strong type system

@jaspervdm jaspervdm added this to the 3.1.0 milestone Feb 15, 2020
Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Let's get this in for 3.1.0.

@jaspervdm jaspervdm merged commit 6bca34c into mimblewimble:master Feb 18, 2020
@antiochp antiochp mentioned this pull request Feb 24, 2020
@jaspervdm jaspervdm deleted the futures0.3 branch February 25, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants