Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Use hyper 0.11 in ethcore-miner and improvements in parity-reactor #8335

Merged
merged 6 commits into from
Apr 10, 2018

Conversation

sorpaas
Copy link
Collaborator

@sorpaas sorpaas commented Apr 9, 2018

This aligns ethcore-miner's hyper dependency with the rest of the crates. However, we still have secret-store depending on hyper 0.10 and I'm planning to fix that in a separate PR.

  • parity-reactor's Remote::spawn_fn now exposes the raw Remote's handle. This allows the EventLoop to be used in many tokio contexts.
  • parity-miner is updated to use async hyper request. This, probably until we get tokio runtime, would need to pass the Remote handle around.

@sorpaas sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies. labels Apr 9, 2018
@sorpaas sorpaas added this to the 1.11 milestone Apr 9, 2018
Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

looks good to me. @0x7CFE can you also look into it?

@5chdn 5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Apr 9, 2018
Copy link
Contributor

@0x7CFE 0x7CFE left a comment

Choose a reason for hiding this comment

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

LGTM


let mut future: Box<Future<Item=(), Error=()>> = Box::new(future::ok(()));
for url in urls {
let mut req = Request::new(Method::Post, url.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be better to use fetch instead of raw hyper?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure whether we can use fetch to send POST request. The fetch crates looks like to use a background thread and is hard coded to only send GET request? https://github.com/paritytech/parity/blob/master/util/fetch/src/client.rs#L216

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it. I'll try to get a version based on fetch once that PR is merged.

@@ -35,6 +35,7 @@ use kvdb_memorydb;
use miner::external::ExternalMiner;
use miner::transaction_queue::PrioritizationStrategy;
use parking_lot::Mutex;
use parity_reactor::Remote;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if parity_reactor abstraction is something we want/should keep.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I totally agree that some day we should remove the reactor wrapper and just use the new plain Tokio Runtime (which also gets a better ThreadPool executer, and allows us not to pass Handle/Remote everywhere!). However, it looks like currently libraries haven't matured enough to do that. For example, hyper still doesn't support solely using Tokio Runtime as of now. So I think currently we still need to keep using reactor abstraction and fix stuff based on that, before we can remove it?

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 9, 2018
@5chdn 5chdn removed the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Apr 9, 2018
@sorpaas sorpaas changed the title Use hyper 0.11 in ethcore-miner and improvements in parity-reactor [WIP] Use hyper 0.11 in ethcore-miner and improvements in parity-reactor Apr 9, 2018
@sorpaas
Copy link
Collaborator Author

sorpaas commented Apr 10, 2018

Okay I have done a new version using fetch. There're several things to note.

Improvements/fixes done on fetch crate:

  • The redirection following logic is fixed to align with HTTP standard. Our current implementation will follow everything using the original METHOD and URL. However, this should only be done for 307 and 308 (Temporary Redirect and Permanent Redirect). For 301, 302, and 303, the request should instead always be GET.
  • fetch is extended to allow fetching arbitrary requests. This fixes the current problem that we cannot POST message body. With this change, I added a new struct Request. This is a wrapper around hyper::Request with a) Changed from using Uri to use Url, as this seems to be what's being used in the rest of the crate, b) Support Clone operation, this is need for redirection following logic, and c) with_ helper functions to allow easier building.
  • A type problem raises because now we're not parsing Url in Fetch::fetch, but in Fetch::get and Fetch::post. The issue is that Rust compiler cannot figure out the return type if those functions are predefined trait functions. So I have to move them to trait implementations. We only have one Client implementation for Fetch with all others being test structs. So this is probably not a problem, but if you have a fix for this please let me know.

Regarding miner crate:

  • @tomusdrw I still need to keep parity-reactor dependency in miner. The issue is that Fetch returns a Future and I would need to spawn it somewhere.
  • Remote and FetchClient parameter passing is moved to CLI, so we don't need to change too many function calls. Miner::push_notifier automatically handles seal_work.enable.

@sorpaas sorpaas changed the title [WIP] Use hyper 0.11 in ethcore-miner and improvements in parity-reactor Use hyper 0.11 in ethcore-miner and improvements in parity-reactor Apr 10, 2018
@5chdn 5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Apr 10, 2018
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 10, 2018
@debris debris merged commit 692cd10 into openethereum:master Apr 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants