upgrade to hyper v1.0 #1368

niklasad1 · 2024-05-10T16:42:08Z

Things that could be improved in this PR or in follow-up:

add helper function serve/serve_with_graceful_shutdown such that users of the low-level API doesn't have to implement their graceful shutdown stuff (I have repeated that a few times already now)
get rid of BoxBody if possible
add additional feature flags to support aws-lc-rs TLS crypto backend

client/http-client/Cargo.toml

niklasad1 · 2024-05-17T15:32:03Z

client/http-client/Cargo.toml

@@ -34,8 +37,8 @@ tokio = { version = "1.16", features = ["net", "rt-multi-thread", "macros"] }

 [features]
 default = ["native-tls"]
-native-tls = ["hyper-rustls/native-tokio", "__tls"]
-webpki-tls = ["hyper-rustls/webpki-tokio", "__tls"]
+native-tls = ["hyper-rustls/native-tokio", "hyper-rustls/ring", "__tls"]


let's discuss whether we should support both crypto backends.

Rustls now supports both ring and aws-lc-rs but migrating to aws-lc-rs requires more dependencies such as cmake, NASM etc.

I'm happy with not exposing it initially, and then as a non-breaking change we can always expose it later or whatever if there's demand :)

client/ws-client/src/tests.rs

client/http-client/src/transport.rs

server/src/middleware/http/proxy_get_request.rs

server/src/transport/ws.rs

jsdw · 2024-05-20T15:06:50Z

client/http-client/src/lib.rs

@@ -46,3 +46,10 @@ mod tests;
 pub use client::{HttpClient, HttpClientBuilder};
 pub use hyper::http::{HeaderMap, HeaderValue};
 pub use jsonrpsee_types as types;
+
+/// Default HTTP body for the client.
+pub type HttpBody = http_body_util::Full<hyper::body::Bytes>;


Could this also be jsonrpsee_core::http_helpers::ResponseBody, since the below two things reference that?

(maybe rename RepsonseBody to HttpBody though?)

jsdw · 2024-05-20T15:14:11Z

examples/examples/http_proxy_middleware.rs

@@ -63,17 +66,17 @@ async fn main() -> anyhow::Result<()> {
 	println!("[main]: response: {:?}", response);

 	// Use hyper client to manually submit a `GET /health` request.
-	let http_client = Client::new();
+	let http_client = Client::builder(TokioExecutor::new()).build_http();


Interesting way to be agnostic over runtime; I could imagine we might adopt a similar thing in subxt to hide the spawning stuff behind the scenes unless the user wants to override the executor or manually drive stuff

jsdw · 2024-05-20T15:53:56Z

server/src/transport/ws.rs

+			let upgraded = match hyper::upgrade::on(req).await {
+				Ok(u) => u,
+				Err(e) => {
+					tracing::debug!(target: LOG_TARGET, "WS upgrade handshake failed: {}", e);
+					return Err(HttpResponse::new(HttpResponseBody::from(format!("WS upgrade handshake failed {e}"))));
+				}
+			};


Is this the thing that moved into that async block below not super long ago for a reason I can't remember (maybe something shutdown related)?

Wondering what the impact of moving it up is!

Like, previously it would only be executed when the returned future is polled, but now it's executed in the connect(..).await and has to finish before the fut is returned

Yes, https://github.com/hyperium/hyper/blob/master/examples/upgrades.rs#L56-#L72

Thus, it will be stuck until the response is returned but I can add a comment in the code because it confuses me as well

jsdw · 2024-05-20T15:56:13Z

server/src/server.rs

-	/// fn run_server() -> ServerHandle {
-	///     let addr = SocketAddr::from(([127, 0, 0, 1], 0));
+	/// async fn run_server() -> ServerHandle {
+	///     let listener = tokio::net::TcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).await.unwrap();


I guess this could move into the tokio::spawn and then run_server could be sync again (iirc this is in an example or two as well)?

jsdw

LGTM, nice one! Just a couple of small comments!

niklasad1 · 2024-05-21T09:10:14Z

@CodiumAI-Agent /review

CodiumAI-Agent · 2024-05-21T09:11:21Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	5, because the PR involves multiple files with significant changes including feature updates, refactoring, and dependency updates. The complexity and breadth of the changes require a thorough review to ensure compatibility, performance, and adherence to best practices.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The PR introduces changes across multiple files which could lead to integration issues if not properly tested. For example, updates in dependency versions and major refactoring could affect existing functionalities.
⚡ Possible issues	Performance Concern: The extensive use of new features and libraries might impact the performance. It is crucial to benchmark the new changes against the current implementation to ensure there is no performance degradation.
🔒 Security concerns	No

Code feedback:

relevant file	server/src/server.rs
suggestion	Consider adding integration tests to cover the new changes introduced in the server configuration and connection handling. This is important to ensure that the new configurations and the refactored connection handling work as expected under different scenarios. [important]
relevant line	use hyper::body::Bytes;

relevant file	server/src/server.rs
suggestion	It's recommended to handle possible errors from the `tokio::net::TcpListener::bind` method to prevent the server from panicking at runtime if the address is already in use or if there are insufficient permissions to bind to the address. [important]
relevant line	let listener = tokio::net::TcpListener::bind(SocketAddr::from(([127, 0, 0, 1], 0))).await.unwrap();

relevant file	server/src/server.rs
suggestion	For better error handling and to avoid potential data loss, consider implementing a backpressure mechanism or a way to handle backpressure from clients in the server implementation. This could prevent the server from being overwhelmed by too many requests or data at once. [medium]
relevant line	use futures_util::future::{self, Either};

relevant file	server/src/server.rs
suggestion	To improve the modularity and readability of the server setup code, consider refactoring the large async block inside `tokio::spawn` into separate functions or methods. This would make the code easier to maintain and test. [medium]
relevant line	tokio::spawn(async move {

lexnv · 2024-05-21T12:20:32Z

server/src/server.rs

@@ -960,32 +995,28 @@ impl<RpcMiddleware, HttpMiddleware> TowerService<RpcMiddleware, HttpMiddleware>
 	}
 }

-impl<RpcMiddleware, HttpMiddleware> hyper::service::Service<hyper::Request<hyper::Body>>
-	for TowerService<RpcMiddleware, HttpMiddleware>
+impl<B, RpcMiddleware, HttpMiddleware> Service<HttpRequest<B>> for TowerService<RpcMiddleware, HttpMiddleware>


nit: Maybe instead of B we can call this Body? To keep consistency with RpcMiddleware and HttpMiddleware?

lexnv · 2024-05-21T12:25:21Z

client/ws-client/src/tests.rs

@@ -461,6 +461,8 @@ fn assert_error_response(err: Error, exp: ErrorObjectOwned) {

 #[tokio::test]
 async fn redirections() {
+	init_logger();


nit: is this indended?

lexnv

LGTM!

niklasad1 force-pushed the na-hyper-v1.0 branch from b07cd0c to cf2e210 Compare May 10, 2024 16:45

upgrade to hyper v1.0

93c7dc2

niklasad1 force-pushed the na-hyper-v1.0 branch from cf2e210 to 93c7dc2 Compare May 13, 2024 09:56

niklasad1 added 4 commits May 14, 2024 20:07

some progress

38bcfd4

fix nits

5ffd811

Merge remote-tracking branch 'origin/master' into na-hyper-v1.0

e764e33

more cleanup

4a61aed

niklasad1 added the breaking change Breaking change in the public APIs label May 16, 2024

niklasad1 commented May 16, 2024

View reviewed changes

client/http-client/Cargo.toml Outdated Show resolved Hide resolved

niklasad1 added 11 commits May 16, 2024 10:04

Update client/http-client/Cargo.toml

58b725c

make all examples compile

efc92be

remove nits

103add0

unify http request

0d9be00

introduce BoxError

3232128

more boxerror

642d964

fix tests

1af0511

remove sync requirement

8c6a5ea

fix nits

0fe58e2

use ring crypto backend

b8d1295

more ring

4ff7805

niklasad1 changed the title ~~[WIP]: upgrade to hyper v1.0~~ upgrade to hyper v1.0 May 17, 2024

niklasad1 commented May 17, 2024

View reviewed changes

client/ws-client/src/tests.rs Outdated Show resolved Hide resolved

Update client/ws-client/src/tests.rs

d357e1d

niklasad1 commented May 17, 2024

View reviewed changes

client/http-client/src/transport.rs Outdated Show resolved Hide resolved

Update client/http-client/src/transport.rs

abc94bd

niklasad1 commented May 17, 2024

View reviewed changes

server/src/middleware/http/proxy_get_request.rs Outdated Show resolved Hide resolved

Update server/src/middleware/http/proxy_get_request.rs

ec9f320

niklasad1 commented May 17, 2024

View reviewed changes

server/src/transport/ws.rs Outdated Show resolved Hide resolved

Update server/src/transport/ws.rs

1e7bc49

niklasad1 marked this pull request as ready for review May 17, 2024 15:54

niklasad1 requested a review from a team as a code owner May 17, 2024 15:54

simplify code

ae860bd

jsdw reviewed May 20, 2024

View reviewed changes

jsdw approved these changes May 20, 2024

View reviewed changes

niklasad1 mentioned this pull request May 21, 2024

refactor: use rustls-platform-verifier cert store and add custom cert store #1372

Closed

address grumbles

0e79167

lexnv reviewed May 21, 2024

View reviewed changes

lexnv approved these changes May 21, 2024

View reviewed changes

unify http body in client/server

bb3dbd5

niklasad1 merged commit 257513e into master May 21, 2024
11 checks passed

niklasad1 deleted the na-hyper-v1.0 branch May 21, 2024 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade to hyper v1.0 #1368

upgrade to hyper v1.0 #1368

niklasad1 commented May 10, 2024 •

edited

niklasad1 May 17, 2024

jsdw May 20, 2024

jsdw May 20, 2024

jsdw May 20, 2024

jsdw May 20, 2024

jsdw May 20, 2024

niklasad1 May 21, 2024 •

edited

niklasad1 May 21, 2024

jsdw May 20, 2024

jsdw left a comment •

edited

niklasad1 commented May 21, 2024

CodiumAI-Agent commented May 21, 2024

lexnv May 21, 2024

lexnv May 21, 2024

lexnv left a comment

upgrade to hyper v1.0 #1368

upgrade to hyper v1.0 #1368

Conversation

niklasad1 commented May 10, 2024 • edited

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

niklasad1 May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw left a comment • edited

Choose a reason for hiding this comment

niklasad1 commented May 21, 2024

CodiumAI-Agent commented May 21, 2024

PR Review 🔍

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

niklasad1 commented May 10, 2024 •

edited

niklasad1 May 21, 2024 •

edited

jsdw left a comment •

edited