middleware refactoring #793

niklasad1 · 2022-06-13T11:55:37Z

It's a quite significant refactoring where the sink is only used for subscriptions as the rest of the calls just returns MethodResponse which is a type that has the final JSON-RPC response and status flag whether the calls was successful or not.
In order to intercept the messages to log the full responses in the middleware.

Further, it also modifies the SubscriptionMethod to be a future which returns when the subscription call has been answered which in turn forces plenty noise i.e, I had to change lots of code.

In addition I noticed that SubscriptionSink::send(&mut self, msg: M) is not needed so I changed it to SubscriptionSink::send(&self, msg: M) also then pipe_from_stream doesn't need &mut self anymore.
rustc may benefit from that to some optimizations....

It splits the middleware into two traits for WS and HTTP.

/// Defines a middleware specifically for HTTP requests with callbacks during the RPC request life-cycle.
/// The primary use case for this is to collect timings for a larger metrics collection solution.
///
/// See [`HttpServerBuilder::set_middleware`](../../jsonrpsee_http_server/struct.HttpServerBuilder.html#method.set_middleware) method
/// for examples.
pub trait HttpMiddleware: Send + Sync + Clone + 'static {
	/// Intended to carry timestamp of a request, for example `std::time::Instant`. How the middleware
	/// measures time, if at all, is entirely up to the implementation.
	type Instant: std::fmt::Debug + Send + Sync + Copy;

	/// Called when a new JSON-RPC request comes to the server.
	fn on_request(&self, remote_addr: SocketAddr, headers: &Headers) -> Self::Instant;

	/// Called on each JSON-RPC method call, batch requests will trigger `on_call` multiple times.
	fn on_call(&self, method_name: &str, params: Params);

	/// Called on each JSON-RPC method completion, batch requests will trigger `on_result` multiple times.
	fn on_result(&self, method_name: &str, success: bool, started_at: Self::Instant);

	/// Called once the JSON-RPC request is finished and response is sent to the output buffer.
	fn on_response(&self, result: &str, _started_at: Self::Instant);
}

/// Defines a middleware specifically for WebSocket connections with callbacks during the RPC request life-cycle.
/// The primary use case for this is to collect timings for a larger metrics collection solution.
///
/// See the [`WsServerBuilder::set_middleware`](../../jsonrpsee_ws_server/struct.WsServerBuilder.html#method.set_middleware)
/// for examples.
pub trait WsMiddleware: Send + Sync + Clone + 'static {
	/// Intended to carry timestamp of a request, for example `std::time::Instant`. How the middleware
	/// measures time, if at all, is entirely up to the implementation.
	type Instant: std::fmt::Debug + Send + Sync + Copy;

	/// Called when a new client connects
	fn on_connect(&self, remote_addr: SocketAddr, headers: &Headers);

	/// Called when a new JSON-RPC request comes to the server.
	fn on_request(&self) -> Self::Instant;

	/// Called on each JSON-RPC method call, batch requests will trigger `on_call` multiple times.
	fn on_call(&self, method_name: &str, params: Params);

	/// Called on each JSON-RPC method completion, batch requests will trigger `on_result` multiple times.
	fn on_result(&self, method_name: &str, success: bool, started_at: Self::Instant);

	/// Called once the JSON-RPC request is finished and response is sent to the output buffer.
	fn on_response(&self, result: &str, started_at: Self::Instant);

	/// Called when a client disconnects
	fn on_disconnect(&self, remote_addr: std::net::SocketAddr);
}

niklasad1 · 2022-06-21T05:59:54Z

core/src/middleware.rs

-	/// Called when a new JSON-RPC comes to the server.
-	fn on_request(&self) -> Self::Instant;
+	/// Called when a new JSON-RPC request comes to the server.
+	fn on_request(&self, remote_addr: std::net::SocketAddr, headers: &http::HeaderMap) -> Self::Instant;


This is a bit weird as the WS will have the same remote addr and headers for the entire connection

Then for HTTP it will be useful for each "request".

So what we could do here:_

on_connect(&self, remote_addr: std::net::SocketAddr, headers: &http::HeaderMap)

on_request(&self)

Then justify the documentation that on_connect for can be used to inspect headers and so on for stateless protocols, we could also move these to different traits as I could be useful to separate middleware impls because HTTP/WS is quite different.

I've been mulling this over; I'm personally leaning towards separate middleware for Http and Ws, since it feels like we have to hack it around to have functions which map nicely to both.

Pros of separate middlware:

precise traits that better express what's actually happening. (http middleware doesn't need on_connect, ws middleware doesn't need headers in on_request, that sort of thing)

(maybe this helps perf or something?)

Cons:

we need to have two similar traits and sets of blanket impls.

users/we can't write generic middleware that applies to both (though I doubt that's an issue, and one could always write some wrapper GenericMiddleware thing that implements both where there is overlap if needed)

yeah, I agree it's better to have two different traits.

You most likely want to handle them differently.

core/src/server/helpers.rs

core/src/server/rpc_module.rs

jsdw · 2022-06-21T14:13:23Z

ws-server/src/server.rs

@@ -427,252 +468,79 @@ async fn background_task(

 		tracing::debug!("recv {} bytes", data.len());

-		let request_start = middleware.on_request();
+		let request_start = middleware.on_request(remote_addr, &headers);


So, it looks like those headers will only ever contain Host and Origin from above; what about any custom headers?

paritytech/soketto#60,

it's missing from soketto but I don't want to make this PR any larger.

Gotcha; makes sense!

jsdw · 2022-06-21T14:14:11Z

ws-server/src/server.rs

@@ -304,8 +324,8 @@ where
 	}
 }

-async fn background_task(
-	server: SokettoServer<'_, BufReader<BufWriter<Compat<tokio::net::TcpStream>>>>,
+struct BackgroundTask<'a, M> {


Good idea to make this a struct!

jsdw · 2022-06-21T16:39:16Z

ws-server/src/server.rs

-where
-	M: Middleware,
-{
+async fn handshake<M: Middleware>(socket: tokio::net::TcpStream, mode: HandshakeResponse<'_, M>) -> Result<(), Error> {


Probably out of scope of this PR but it feels like having an accept_handshake and reject_handshake call (or similar) would make more sense, since this call basically just switches on an enum and has very little shared between them?

jsdw · 2022-06-21T16:55:42Z

core/src/middleware.rs

@@ -37,22 +39,22 @@
 pub trait Middleware: Send + Sync + Clone + 'static {
 	/// Intended to carry timestamp of a request, for example `std::time::Instant`. How the middleware
 	/// measures time, if at all, is entirely up to the implementation.
-	type Instant: Send + Copy;
+	type Instant: std::fmt::Debug + Send + Sync + Copy;


Another offtopic thought (just noting for future reference); before 1.0 is it worth making this more generic than Instant so that it can, if needed, be a way for users to pass context between middleware calls and such?

Can you elaborate a bit what you mean with an example?

It's possible to copy it between calls and stuff right now

core/src/server/helpers.rs

ws-server/Cargo.toml

niklasad1 · 2022-06-29T13:49:28Z

core/src/middleware.rs

+	fn on_response(&self, result: &str, started_at: Self::Instant);
+
+	/// Called when a client disconnects
+	fn on_disconnect(&self, remote_addr: std::net::SocketAddr);


this is silly but I reckon that it might be useful to log when peers disconnect

niklasad1 · 2022-07-05T10:28:34Z

core/src/middleware.rs

+	fn on_request(&self, remote_addr: SocketAddr, headers: &Headers) -> Self::Instant;
+
+	/// Called on each JSON-RPC method call, batch requests will trigger `on_call` multiple times.
+	fn on_call(&self, method_name: &str, params: Params);


do we want method kind here as well i.e, if the call is a "subscription", "method call" or "notification"?

Is it easily accessbile? I guess it wouldn't do any harm to have!

I don't do it because it requires to lookup the method name in the HashMap.

should be quite easy to do.

Mmm, if it's a performance hit to do so (even when no middleware) then I'm happy not at the mo!

we do this lookup when doing the call so I didn't want to do it because the timing might be off.

but that it is probably picking hairs and I can do on_call after looking up the method

niklasad1 · 2022-07-05T10:29:34Z

http-server/src/server.rs

+}
+
+#[derive(Debug, Clone)]
+struct CallData<'a, M: Middleware> {


this should be cheap to copy

jsdw · 2022-07-05T13:00:13Z

examples/examples/middleware_ws.rs

 use jsonrpsee::ws_client::WsClientBuilder;
 use jsonrpsee::ws_server::{RpcModule, WsServerBuilder};

 #[derive(Clone)]
 struct Timings;

-impl middleware::Middleware for Timings {
+impl middleware::WsMiddleware for Timings {


I love the middleware split!

I'm still wondering about type Instant. If I want to share some state between middleware calls, or capture the timings between say on_connect and on_disconnect or some random other thing, currently we can't do it.

But we could if we did something like:

trait WsMiddleware { type State: Default + Clone; fn on_connect(&Self, state: &mut State, ...otherParams); fn on_request(&self, state: &mut State, ...otherParams); //... and so on; mutable per-connection/request state given to each call }

Then we can set/read the state somewhat arbitrarily in any call that's interested, eg for timings:

struct ConnectionTimings; struct ConnectionTimingsState(Option<Instant>); impl WsMiddleware for ConnectionTimings { type State = ConnectionTimingsState; fn on_connect(&self, state: &mut State, ...) { *state = Some(Instant::now()); } fn on_disconnect(&self, state: &mut State, ...) { println!("Disconnected; connection open for {}", state.0.unwrap().elapsed()); } }

Or something that logs the time of each call and prints them all off at the end or whatever else.

A default "no middleware" impl can have State = () which should be free.

Just a thought, since we are expanding middleware to be more useful; what do you think?

(this could very well be a separate issue too if you liked the idea; no need to do it all here necessarily).

Sounds reasonable.

I guess it would be flexible as users can "measure" whatever they want as all methods on middleware traits are supposed to have &mut State such how long a connection lives, individual calls, batches and so on takes?

So why not.

That was my take on it yeah; it'd just give the flexibility to keep track of whatever was desired across the lifetime of a single connection :)

As we agreed, let's think about this in a followup issue/pr!

Edit: issue added: #816

lexnv

LGTM! 👍 As a follow-up we could extend the middleware with method_kind and custom user context/state to extend the type Instant

jsdw

Looks great to me!

niklasad1 added 3 commits June 13, 2022 12:52

WIP: refactoring

1dafb1e

refactor ws server

e9252ac

refactor http server

000492c

niklasad1 force-pushed the na-middleware branch from 498aef1 to 12dbf53 Compare June 18, 2022 11:48

fix tests

cbc189e

niklasad1 force-pushed the na-middleware branch from 12dbf53 to cbc189e Compare June 18, 2022 17:30

niklasad1 marked this pull request as ready for review June 18, 2022 20:28

niklasad1 requested a review from a team as a code owner June 18, 2022 20:28

niklasad1 added 6 commits June 18, 2022 22:47

Merge remote-tracking branch 'origin/master' into na-middleware

6083c82

Delete TODO.txt

babdd06

fix tests again

1b5ec0f

Merge remote-tracking branch 'origin/na-middleware' into na-middleware

31203b9

add benches/src/lib.rs

7bcec5f

remove bench changes; fast less deps

cb32799

niklasad1 self-assigned this Jun 20, 2022

niklasad1 added 3 commits June 20, 2022 09:43

no more env_logger

14b0937

update examples

11392d3

ws server; expose headers in middleware

baf0e6b

niklasad1 changed the title ~~WIP: middleware refactoring~~ middleware refactoring Jun 20, 2022

add back uncommented code

0e7d115

niklasad1 commented Jun 21, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into na-middleware

2784bee

jsdw reviewed Jun 21, 2022

View reviewed changes

core/src/server/helpers.rs Outdated Show resolved Hide resolved

jsdw reviewed Jun 21, 2022

View reviewed changes

core/src/server/rpc_module.rs Outdated Show resolved Hide resolved

jsdw reviewed Jun 21, 2022

View reviewed changes

niklasad1 added 2 commits June 22, 2022 10:23

fix nits

108794b

make the code more readable

5ad1192

niklasad1 added 2 commits June 28, 2022 12:39

fix tests

c5d46ee

revert benchmark change

0b0d987

niklasad1 commented Jun 28, 2022

View reviewed changes

core/src/server/helpers.rs Outdated Show resolved Hide resolved

Update core/src/server/helpers.rs

ca0be0b

niklasad1 commented Jun 28, 2022

View reviewed changes

core/src/server/helpers.rs Show resolved Hide resolved

niklasad1 commented Jun 28, 2022

View reviewed changes

ws-server/Cargo.toml Show resolved Hide resolved

niklasad1 added 3 commits June 28, 2022 13:07

Update ws-server/Cargo.toml

79138ed

Merge remote-tracking branch 'origin/na-middleware' into na-middleware

3e88504

add limit to batch responses as well

150824b

niklasad1 commented Jun 29, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into na-middleware

c2e686c

niklasad1 mentioned this pull request Jun 30, 2022

Inspecting incoming request #811

Closed

niklasad1 added 3 commits June 30, 2022 12:12

Merge remote-tracking branch 'origin/master' into na-middleware

dbd30e2

pre-allocate string for batches

a76d72f

small refactor

1d4fd8a

niklasad1 commented Jul 5, 2022

View reviewed changes

http-server/src/server.rs

}

#[derive(Debug, Clone)]

struct CallData<'a, M: Middleware> {

Copy link

Member Author

niklasad1 Jul 5, 2022

Choose a reason for hiding this comment

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

this should be cheap to copy

jsdw reviewed Jul 5, 2022

View reviewed changes

lexnv approved these changes Jul 6, 2022

View reviewed changes

jsdw mentioned this pull request Jul 6, 2022

Consider generalising middleware by passing state into every call #816

Closed

jsdw approved these changes Jul 6, 2022

View reviewed changes

niklasad1 merged commit 3ee635f into master Jul 6, 2022

niklasad1 deleted the na-middleware branch July 6, 2022 15:45

This was referenced Jul 23, 2022

RpcModule::register_blocking_method with panicking method #794

Closed

[middleware]: expose all details regarding a call. #758

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

middleware refactoring #793

middleware refactoring #793

niklasad1 commented Jun 13, 2022 •

edited

niklasad1 Jun 21, 2022 •

edited

jsdw Jun 21, 2022 •

edited

niklasad1 Jun 21, 2022

jsdw Jun 21, 2022

niklasad1 Jun 21, 2022

jsdw Jun 22, 2022

jsdw Jun 21, 2022

jsdw Jun 21, 2022

jsdw Jun 21, 2022

niklasad1 Jun 28, 2022

niklasad1 Jun 29, 2022

niklasad1 Jul 5, 2022

jsdw Jul 5, 2022

niklasad1 Jul 6, 2022

jsdw Jul 6, 2022

niklasad1 Jul 6, 2022 •

edited

niklasad1 Jul 5, 2022

jsdw Jul 5, 2022 •

edited

jsdw Jul 5, 2022

niklasad1 Jul 5, 2022 •

edited

jsdw Jul 5, 2022

jsdw Jul 6, 2022 •

edited

lexnv left a comment

jsdw left a comment

middleware refactoring #793

middleware refactoring #793

Conversation

niklasad1 commented Jun 13, 2022 • edited

niklasad1 Jun 21, 2022 • edited

Choose a reason for hiding this comment

jsdw Jun 21, 2022 • 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

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

niklasad1 Jul 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jul 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Jul 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jul 6, 2022 • edited

Choose a reason for hiding this comment

lexnv left a comment

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

niklasad1 commented Jun 13, 2022 •

edited

niklasad1 Jun 21, 2022 •

edited

jsdw Jun 21, 2022 •

edited

niklasad1 Jul 6, 2022 •

edited

jsdw Jul 5, 2022 •

edited

niklasad1 Jul 5, 2022 •

edited

jsdw Jul 6, 2022 •

edited