Update hyper to version 0.12. #303

c0gent · 2018-08-27T18:43:18Z

Replace tokio_core with tokio.
server_utils::reactor::Remote has been renamed to Executor.
The Shared variant now contains a tokio::runtime::TaskExecutor.
This may need to be changed to a trait object (of
tokio::executor::Executor) or be otherwise abstracted to conceal the
type in the public API.
Bump crate versions to 0.9

Eventually, I'll be updating Parity to use these new changes and to remove tokio_core etc. You may want to consider holding off on merging until I'm done, to ensure everything works properly. I don't have an estimate on when that will be.

Closes #298

tomusdrw

Looks good! Couple of fixes required before the merge though.

tomusdrw · 2018-08-28T10:27:49Z

http/src/handler.rs

-				header::qitem(mime::APPLICATION_JSON)
-			]));
+			headers.append(header::ALLOW, Method::OPTIONS.as_str().parse().unwrap());
+			headers.append(header::ALLOW, Method::POST.as_str().parse().unwrap());


Please use HeaderValue::from_static here or turn unwraps into expects

tomusdrw · 2018-08-28T10:28:11Z

http/src/handler.rs

-				Ascii::new("content-type".to_owned()),
-				Ascii::new("accept".to_owned()),
-			]));
+			headers.append(header::ACCESS_CONTROL_ALLOW_METHODS, Method::OPTIONS.as_str().parse().unwrap());


same as above

tomusdrw · 2018-08-28T10:35:23Z

http/src/handler.rs

 			if let Some(cors_max_age) = cors_max_age {
-				headers.set(header::AccessControlMaxAge(cors_max_age));
+				headers.append(header::ACCESS_CONTROL_MAX_AGE, HeaderValue::from_str(&cors_max_age.to_string()).unwrap());


unwrap -> expect

http/src/lib.rs

tomusdrw · 2018-08-28T10:45:40Z

server-utils/src/stream_codec.rs

@@ -1,5 +1,6 @@
 use std::{io, str};
-use tokio_io::codec::{Decoder, Encoder};
+// use tokio_io::codec::{Decoder, Encoder};


stray comment

tcp/src/server.rs

http/src/response.rs

tomusdrw · 2018-08-28T14:13:36Z

ipc/Cargo.toml

-parity-tokio-ipc = { git = "https://github.com/nikvolf/parity-tokio-ipc", branch = "stable" }
+jsonrpc-core = { version = "9.0", path = "../core" }
+jsonrpc-server-utils = { version = "9.0", path = "../server-utils" }
+parity-tokio-ipc = { git = "https://github.com/poanetwork/parity-tokio-ipc" }


We need to switch to mainstream libs before merging.

server-utils/src/reactor.rs

c0gent · 2018-08-28T22:19:45Z

Everything except deciding what to do about the Response conversion is done.

* Replace `tokio_core` with `tokio`. * `server_utils::reactor::Remote` has been renamed to `Executor`. The `Shared` variant now contains a `tokio::runtime::TaskExecutor`. This may need to be changed to a trait object (of `tokio::executor::Executor`) or be otherwise abstracted to conceal the type in the public API. * Bump crate versions to 0.9

…tion

tomusdrw

Grumbles self addressed: c0gent#1

tomusdrw · 2018-09-25T08:16:36Z

http/src/handler.rs

@@ -317,34 +316,34 @@ impl<M: Metadata, S: Middleware<M>> RpcHandler<M, S> {
 		let metadata = self.jsonrpc_handler.extractor.read_metadata(&request);

 		// Proceed
-		match *request.method() {
+		match request.method().clone() {


Do we really need to clone here? *request.method() should work fine.

tomusdrw · 2018-09-25T08:17:50Z

http/examples/http_meta.rs

-			let auth = auth.map(|h| h.token.clone());
+		.meta_extractor(|req: &hyper::Request<hyper::Body>| {
+			let auth = req.headers().get(hyper::header::AUTHORIZATION)
+				.map(|h| h.to_str().expect("Invalid authorization value").to_owned());


IMHO better to use some default value in case of error here, I know it's only an example, but this produces a server that panics on invalid payload which is not really something that we should recommend.

tomusdrw · 2018-09-25T08:18:16Z

http/src/handler.rs

+			headers.append(header::ACCESS_CONTROL_ALLOW_METHODS, Method::POST.as_str().parse()
+				.expect("`Method` will always parse; qed"));
+
+			headers.append(header::ACCESS_CONTROL_ALLOW_HEADERS, HeaderValue::from_static("origin"));


Will require a merge with #305

tomusdrw · 2018-09-25T08:20:43Z

http/src/handler.rs

-
+	/// Returns true if the `content_type` header indicates a valid JSON
+	/// message.
+	fn is_json(content_type: Option<&header::HeaderValue>) -> bool {
 		match content_type {


Could be simplfiied:

match content_type.and_then(|val| val.to_str().ok()) { Some("application/json") => true, Some("application/json; charset=utf-8") => true, None => false }

tomusdrw · 2018-09-25T08:21:07Z

core/src/io.rs

@@ -11,7 +11,7 @@ use types::{Error, ErrorCode, Version};
 use types::{Request, Response, Call, Output};

 /// A type representing middleware or RPC response before serialization.
-pub type FutureResponse = Box<Future<Item=Option<Response>, Error=()> + Send>;
+pub type FutureResponse = Box<Future<Item=Option<Response>, Error=()> + Send + 'static>;


'static is implicit from Box we don't need it here.

tomusdrw · 2018-09-25T08:21:36Z

core/src/calls.rs

@@ -5,7 +5,7 @@ use futures::{Future, IntoFuture};
 use BoxFuture;

 /// Metadata trait
-pub trait Metadata: Clone + Send + 'static {}
+pub trait Metadata: Clone + Send + Sync + 'static {}


Why does Metadata have to be Sync? We should never really access it inside the server, so Send should be enough

tomusdrw · 2018-09-25T08:23:35Z

http/src/tests.rs

@@ -159,7 +159,8 @@ fn should_handle_health_endpoint_failure() {

 	// then
 	assert_eq!(response.status, "HTTP/1.1 503 Service Unavailable".to_owned());
-	assert_eq!(response.body, "25\n{\"code\":-34,\"message\":\"Server error\"}\n0\n");
+	// assert_eq!(response.body, "25\n{\"code\":-34,\"message\":\"Server error\"}\n0\n");


remove please

tomusdrw · 2018-09-25T08:23:46Z

http/src/tests.rs

@@ -458,7 +460,8 @@ fn should_add_cors_header_for_null_origin_when_all() {
 	// then
 	assert_eq!(response.status, "HTTP/1.1 200 OK".to_owned());
 	assert_eq!(response.body, method_not_found());
-	assert!(response.headers.contains("Access-Control-Allow-Origin: null"), "Headers missing in {}", response.headers);
+	// println!("HEADERS: {:?}", response.headers);


remove please

tomusdrw · 2018-09-25T08:25:16Z

ipc/src/server.rs

-					)
-				).split();
+				let (writer, reader) = Framed::new(
+	                io_stream,


Weird indentation

Address grumbles and refactor cors headers

c0gent · 2018-09-27T16:11:33Z

Just completely out of curiosity, I'm sure there's a good reason, why handle headers as ascii instead of HeaderName/HeaderValue in server-utils/src/cors.rs?

tomusdrw · 2018-09-27T19:24:20Z

@c0gent I'd like to keep server-utils agnostic to the framework we are using in http server (some stuff is shared with ws and minihttp servers) - so I don't really want to pull hyper into that dep. I missed that hyper got included in #305 so piggybacking this change on your PR :)

* Update hyper to version 0.12. * Replace `tokio_core` with `tokio`. * `server_utils::reactor::Remote` has been renamed to `Executor`. The `Shared` variant now contains a `tokio::runtime::TaskExecutor`. This may need to be changed to a trait object (of `tokio::executor::Executor`) or be otherwise abstracted to conceal the type in the public API. * Bump crate versions to 0.9 * Update hyper to version 0.12. * Replace `tokio_core` with `tokio`. * `server_utils::reactor::Remote` has been renamed to `Executor`. The `Shared` variant now contains a `tokio::runtime::TaskExecutor`. This may need to be changed to a trait object (of `tokio::executor::Executor`) or be otherwise abstracted to conceal the type in the public API. * Bump crate versions to 0.9 * Fix compilation of hyper-0.2 * Address grumbles. * Re-export `AllowCors` from `http`.

c0gent mentioned this pull request Aug 27, 2018

hbbft Integration Meta-Tracking Thread poanetwork/parity-ethereum#8

Open

43 tasks

c0gent force-pushed the c0gent-hyper branch 2 times, most recently from afd2f8d to 68c1faa Compare August 28, 2018 13:13

tomusdrw reviewed Aug 28, 2018

View reviewed changes

c0gent force-pushed the c0gent-hyper branch 2 times, most recently from e933137 to 9f50f79 Compare August 28, 2018 22:17

tomusdrw mentioned this pull request Sep 8, 2018

Update hyper requirement to 0.12 #274

Closed

c0gent force-pushed the c0gent-hyper branch 4 times, most recently from 25ca8c2 to 615e04a Compare September 14, 2018 23:04

c0gent force-pushed the c0gent-hyper branch from 615e04a to 82b3686 Compare September 15, 2018 13:30

c0gent mentioned this pull request Sep 24, 2018

Update hyper to 0.12 #298

Closed

Merge branch 'master' into c0gent-hyper

ae7fcce

c0gent force-pushed the c0gent-hyper branch from 82b3686 to 5341632 Compare September 26, 2018 04:01

c0gent force-pushed the c0gent-hyper branch from 5341632 to 29a194e Compare September 26, 2018 16:43

tomusdrw added 3 commits September 27, 2018 12:35

Fix compilation of hyper-0.2

f93c731

Merge remote-tracking branch 'c0gent/c0gent-hyper' into td-fixcompila…

7cff0ab

…tion

Address grumbles.

6b486b5

tomusdrw reviewed Sep 27, 2018

View reviewed changes

c0gent and others added 2 commits September 27, 2018 08:18

Merge pull request #1 from tomusdrw/td-fixcompilation

f8c30a8

Address grumbles and refactor cors headers

Re-export AllowCors from http.

60ea199

tomusdrw approved these changes Sep 27, 2018

View reviewed changes

tomusdrw merged commit 9e06214 into paritytech:master Sep 27, 2018

c0gent mentioned this pull request Oct 12, 2018

Replace tokio_core with tokio (ring -> 0.13) openethereum/parity-ethereum#9657

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update hyper to version 0.12. #303

Update hyper to version 0.12. #303

c0gent commented Aug 27, 2018 •

edited

Loading

tomusdrw left a comment

tomusdrw Aug 28, 2018

tomusdrw Aug 28, 2018

tomusdrw Aug 28, 2018

tomusdrw Aug 28, 2018

tomusdrw Aug 28, 2018

c0gent commented Aug 28, 2018 •

edited

Loading

tomusdrw left a comment

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

tomusdrw Sep 25, 2018

c0gent commented Sep 27, 2018

tomusdrw commented Sep 27, 2018

Update hyper to version 0.12. #303

Update hyper to version 0.12. #303

Conversation

c0gent commented Aug 27, 2018 • edited Loading

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

c0gent commented Aug 28, 2018 • edited Loading

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

c0gent commented Sep 27, 2018

tomusdrw commented Sep 27, 2018

c0gent commented Aug 27, 2018 •

edited

Loading

c0gent commented Aug 28, 2018 •

edited

Loading