Replace reqwest with hyper #8099

twittner · 2018-03-13T11:37:13Z

The approach taken is to spawn a background thread running a tokio Core and hyper Client, processing requests sent over a mpsc channel and send back responses over oneshot channels. Hyper client itself does not implement Send so this way we can keep the Send bound of the Fetch trait itself and make this a local change.

To accommodate existing synchronous uses of fetch responses an adapter BodyReader is provided which waits for response body Chunks to arrive.

Resolves #7829.

parity-cla-bot · 2018-03-13T11:37:15Z

It looks like @twittner hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, plesae reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

twittner · 2018-03-13T11:40:02Z

[clabot:check]

parity-cla-bot · 2018-03-13T11:40:03Z

It looks like @twittner signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

On `clone` the counter is incremented, on `drop` decremented. Once 0 we send `None` over the channel, expecting the background thread to end.

folsen

LGTM other than the nitpicks. Although I really only browsed through the code for reasonable-ness, didn't dig too deep into all the logic around the read implementation and such.

folsen · 2018-03-13T15:59:46Z

util/fetch/src/client.rs

+					break
+				}
+			} else {
+				let body = self.body.take().expect("within this loop `self.body` is always defined");


Tiny nitpick but we have a standardized format for these texts: https://wiki.parity.io/Coding-guide

Thanks, I updated the message.

folsen · 2018-03-13T16:02:09Z

util/fetch/src/client.rs

+	fn it_should_fetch() {
+		let client = Client::new().unwrap();
+		let abort = Abort::default().with_max_duration(Duration::from_secs(10));
+		let future = client.fetch("https://httpbin.org/drip?numbytes=3&duration=3&delay=1&code=200", abort);


Is there no library to mock responses like this locally so we don't have to have an internet connection to run tests? (And potentially fail tests because httpbin.org is down or blocking us or something)

I know of mockito which can replace some of these tests. However it requires RUST_TEST_THREADS=1 and certain test cases like timeouts are curently not supported AFAICT.

Perhaps just spawn an http server in tests? Or at least bind a TcpSocket to a random port and respond with some predefined data.

I updated the tests to run a local test server instead.

tomusdrw

Looks good, couple of nitpicks as well.

tomusdrw · 2018-03-13T17:10:32Z

dapps/src/tests/helpers/fetch.rs

-
-	fn fetch_with_abort(&self, url: &str, _abort: fetch::Abort) -> Self::Result {
+	fn fetch(&self, url: &str, abort: fetch::Abort) -> Self::Result {
+		let u = Url::parse(url).unwrap();


Perhaps Url crate should be re-exported from fetch if it's required to instantiate some other public types (Response)?

Yes, this may be convenient.

tomusdrw · 2018-03-13T17:13:32Z

dapps/src/handlers/fetch.rs

@@ -303,7 +307,7 @@ impl ContentFetcherHandler {
 					Ok(ValidatorResponse::Streaming(stream)) => {
 						trace!(target: "dapps", "Validation OK. Streaming response.");
 						let (reading, response) = stream.into_response();
-						fetch2.process_and_forget(reading);
+						pool.spawn(reading).forget();


That could probably be pass-through (Since we use BodyParser to convert async->sync and later we use into_response() to convert from sync->async). To be investigated in subsequent PRs.

tomusdrw · 2018-03-13T17:22:53Z

util/fetch/src/client.rs

+	/// Create a new fetch client.
+	pub fn new() -> Result<Self, Error> {
+		let startup_done = Arc::new((Mutex::new(Ok(())), Condvar::new()));
+		let (tx_proto, rx_proto) = mpsc::channel(64);


Might be a bit too big for our use case, since every sender get's an additional slot anyway. Probably 16 would do.

tomusdrw · 2018-03-13T17:25:48Z

util/fetch/src/client.rs

+impl Client {
+	/// Create a new fetch client.
+	pub fn new() -> Result<Self, Error> {
+		let startup_done = Arc::new((Mutex::new(Ok(())), Condvar::new()));


I suppose std::sync::mpsc::channel/sync_channel() would be a bit simpler here, and also supports timeouts.

tomusdrw · 2018-03-13T17:27:19Z

util/fetch/src/client.rs

+								return Err(Error::Aborted)
+							}
+							if let Some(next_url) = redirect_location(url, &resp) {
+								if redirects >= abort.max_redirects() {


Is there a way to flatten the indentation here?

tomusdrw · 2018-03-13T17:32:03Z

util/fetch/src/client.rs

 		}
+		match try_ready!(self.body.poll()) {
+			None    => Ok(Async::Ready(None)),


Mixed tabs and spaces

tomusdrw · 2018-03-13T17:32:54Z

util/fetch/src/client.rs

+		while self.body.is_some() {
+			// Can we still read from the current chunk?
+			if self.offset < self.chunk.len() {
+				let k = cmp::min(self.chunk.len() - self.offset, buf.len() - m);


Might be good to use a bit more descriptive names for m and k

tomusdrw · 2018-03-13T17:34:14Z

util/fetch/src/client.rs

+			} else {
+				let body = self.body.take().expect("within this loop `self.body` is always defined");
+				match body.into_future().wait() { // wait for next chunk
+					Err((e, _))   => {


A bunch of more tabs and space mixture for identation.

tomusdrw · 2018-03-13T17:35:50Z

util/fetch/src/client.rs

+				self.offset += k;
+				m += k;
+				if m == buf.len() {
+					break


I would put return Ok(m) here and instead of the other break - thanks to this we don't need the Ok(m) at the bottom

I think Ok(m) is still needed there, as the while loop might theoretically not be entered at all.

Oh, that's true. We could use loop and if let Some(body) = self.body.take() inside to avoid expect?

tomusdrw · 2018-03-13T17:38:55Z

util/fetch/src/client.rs

+	fn it_should_fetch() {
+		let client = Client::new().unwrap();
+		let abort = Abort::default().with_max_duration(Duration::from_secs(10));
+		let future = client.fetch("https://httpbin.org/drip?numbytes=3&duration=3&delay=1&code=200", abort);


Perhaps just spawn an http server in tests? Or at least bind a TcpSocket to a random port and respond with some predefined data.

tomusdrw · 2018-03-14T11:10:29Z

util/fetch/src/client.rs

+				self.offset += k;
+				m += k;
+				if m == buf.len() {
+					break


Oh, that's true. We could use loop and if let Some(body) = self.body.take() inside to avoid expect?

kirushik · 2018-03-14T12:49:06Z

🎉

twittner requested a review from tomusdrw March 13, 2018 11:37

Kirill Pimenov and others added 7 commits March 13, 2018 16:03

Very primitive test of the Client API

deba819

[WIP] getting rid of request

17f45c8

Add support for redirects.

b47f4c6

Remove CpuPool from fetch::Client.

7fec3ae

Adapt code to API changes and fix tests.

ec9d156

Use reference counter to stop background thread.

da1794e

On `clone` the counter is incremented, on `drop` decremented. Once 0 we send `None` over the channel, expecting the background thread to end.

Fix tests.

dc8ed22

twittner force-pushed the replace_reqwest_with_hyper branch from fef7c11 to dc8ed22 Compare March 13, 2018 15:17

Comment.

a3ef61c

folsen reviewed Mar 13, 2018

View reviewed changes

Change expect messages.

6e49a09

tomusdrw approved these changes Mar 13, 2018

View reviewed changes

tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. labels Mar 13, 2018

twittner added 5 commits March 14, 2018 10:36

Use local test server for testing fetch client.

4e4519b

Ensure max_size also in BodyReader.

8bd9bd9

Replace Condvar with sync_channel.

88cd685

Re-export url::Url from fetch crate.

43539ac

Remove spaces.

065780a

twittner added the A0-pleasereview 🤓 Pull request needs code review. label Mar 14, 2018

tomusdrw approved these changes Mar 14, 2018

View reviewed changes

tomusdrw removed the A0-pleasereview 🤓 Pull request needs code review. label Mar 14, 2018

twittner added 2 commits March 14, 2018 12:41

Use random ports in local test server.

208047a

Merge branch 'master' into replace_reqwest_with_hyper

6acb1fc

folsen merged commit 322dfbc into master Mar 14, 2018

folsen deleted the replace_reqwest_with_hyper branch March 14, 2018 12:41

tomaka mentioned this pull request Mar 14, 2018

Tweaks and add a Dockerfile to compile for Android #8036

Merged

debris added a commit that referenced this pull request Mar 16, 2018

Backport #8099

a6dc80b

5chdn added this to the 1.11 milestone Mar 20, 2018

debris added a commit that referenced this pull request Apr 4, 2018

Backport #8099 (#8132)

fdd03cc

5chdn mentioned this pull request May 15, 2018

Release next-beta 2.0 #8619

Closed

62 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace reqwest with hyper #8099

Replace reqwest with hyper #8099

twittner commented Mar 13, 2018

parity-cla-bot commented Mar 13, 2018

twittner commented Mar 13, 2018

parity-cla-bot commented Mar 13, 2018

folsen left a comment

folsen Mar 13, 2018

twittner Mar 14, 2018

folsen Mar 13, 2018

twittner Mar 13, 2018

tomusdrw Mar 13, 2018

twittner Mar 14, 2018

tomusdrw left a comment

tomusdrw Mar 13, 2018

twittner Mar 14, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 13, 2018

twittner Mar 14, 2018

tomusdrw Mar 14, 2018

tomusdrw Mar 13, 2018

tomusdrw Mar 14, 2018

kirushik commented Mar 14, 2018

Replace reqwest with hyper #8099

Replace reqwest with hyper #8099

Conversation

twittner commented Mar 13, 2018

parity-cla-bot commented Mar 13, 2018

twittner commented Mar 13, 2018

parity-cla-bot commented Mar 13, 2018

folsen 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

tomusdrw 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

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

kirushik commented Mar 14, 2018