feat(server): JSON-RPC specific middleware #1215
Conversation
niklasad1
force-pushed
the
na-new-jsonrpc-middleware
branch
from
3205b22
to
525cb67
Compare
niklasad1
force-pushed
the
na-new-jsonrpc-middleware
branch
from
fa82771
to
0bdbfd9
Compare
niklasad1
changed the title
niklasad1
changed the title
server/src/middleware/mod.rs
Outdated
| /// Similar to `tower::Service` but specific for jsonrpsee and | ||
| /// doesn't requires `&mut self` for performance reasons. | ||
| #[async_trait::async_trait] | ||
| pub trait RpcServiceT<'a> { |
There was a problem hiding this comment.
There is also an alternative to do which doesn't requrire boxing the future as the async trait does:
pub trait Service {
/// The future response value.
type Future<'cx>: Future<Output = MethodResponse>>
where
Self: 'cx,
Request: 'cx;
/// Process the request and return the response asynchronously.
fn call(&self, req: Request<'_>) -> Self::Future<'_>;
}
There was a problem hiding this comment.
This would allow more a little better control/performance especially if there are a bunch of middlewares, and our logging one could also then be as light weight as possible, so IMO it's probably worth doing this even if it makes the trait more annoying (people can just set the type to Box<dyn Future<Output = Foo> + Send + Sync + 'static> if they want to keep it simple anyway I think).
There was a problem hiding this comment.
Our stuff requires still requires Send + 'static and not sure if it's worth all the complexity with such Service trait
service and low-level API for more fine-grained API to disconnect peers etc
#1224
| /// | ||
| /// This logs each request and response for every call. | ||
| /// | ||
| pub fn rpc_logger(self, max_log_len: u32) -> RpcServiceBuilder<Stack<RpcLoggerLayer, L>> { |
There was a problem hiding this comment.
dq: Is this a wrapper over RpcService.layer(RpcLogger::new(..)) for convenience since that's a very common use-case?
There was a problem hiding this comment.
Yeah, I removed our default logging/tracing in the server and this can be used for "backward-compatible logger" or folks to adapt their own layer for customized logging/tracing.
| rate: Rate, | ||
| } | ||
|
|
||
| impl<S> RateLimit<S> { |
There was a problem hiding this comment.
What's your opinion on supporting this in the server/rpc/layers similar to the RpcLogger? Its a pretty nice implementation :D
There was a problem hiding this comment.
My offhand thought is that rate limiting is quite complicated and everybody may want to have a slightly different approach to how limits are applied and how things are throttled etc, so while we could maintain a "simple" one, maybe it's best left as an exercise for the reader (but I might be wrong!)
There was a problem hiding this comment.
Yeah, let's start with this an example and let see whether anyone wants it or not in jsonrpsee :)
I would be happy not to maintain it :D
core/src/server/helpers.rs
Outdated
| /// Send a JSON-RPC response to the client. If the serialization of `result` exceeds `max_response_size`, | ||
| /// an error will be sent instead. | ||
| pub fn response<T>(id: Id, result: ResponsePayload<T>, max_response_size: usize) -> Self |
There was a problem hiding this comment.
Nit: I wonder whether it's worth renaming this to method_response to try and make it feel like "not the default one to use", and/or maybe just a tweak to docs like "If the response is from a subscription, use subscription_response", orr ditching subscription_response and instead adding another arg to response (ie is_subscription: bool) so that every call has to be explicit.
There was a problem hiding this comment.
Sure, it makes sense.
I just added subscription_response and never thought about it. I was just happy that it worked :P
There was a problem hiding this comment.
I'm not too fussed, having looked at the rest of the code (maybe just add the doc comment?), so whatever you prefer :)
There was a problem hiding this comment.
yeah, a subscription response is basically just a method response but in some scenarios in the server it may be useful to know and in middleware doing grafana metrics etc.
I think I want to keep response but lemme adjust the docs a little bit better.
core/src/server/rpc_module.rs
Outdated
| pub fn method_kind(&self, method_name: &str) -> MethodKind { | ||
| match self.method(method_name) { |
There was a problem hiding this comment.
Instead of MethodKind::Unknown, wouyld it make more sense to return Option<MethodKind> here? (or alternately, rename it to somehting clearer eg MethodKind::NotFound?)
There was a problem hiding this comment.
ah, this code is not used so I removed it but I guess your comment still applies.
Lemme fix it NotFound is better
| @@ -438,6 +438,9 @@ impl Subscription { | |||
| let raw = self.rx.recv().await?; | |||
|
|
|||
| tracing::debug!("[Subscription::next]: rx {}", raw); | |||
|
|
|||
| // clippy complains about this but it doesn't compile without the extra res binding. | |||
There was a problem hiding this comment.
Ooh weird; what's the error?!
There was a problem hiding this comment.
lifetime doesn't live long enough, it may be some issue with my own serde impl I did a long time ago.
There was a problem hiding this comment.
Strange though that nothing changes at all whether the variable is there or not! I wouldn't expect any difference since you haven't given any extra type info on the variable or whatever :)
| let rpc_middleware = RpcServiceBuilder::new() | ||
| .layer_fn(|service| Logger(service)) | ||
| // This state is created per connection. | ||
| .layer_fn(|service| CallsPerConn { service, count: AtomicUsize::new(0) }) | ||
| // This state is shared by all connections. | ||
| .layer_fn(move |service| GlobalCalls { service, count: global_cnt.clone() }); |
Co-authored-by: James Wilson <james@jsdw.me>
…na-new-jsonrpc-middleware
This PR replaces the
RpcLoggerwith a composable middleware similar to thetower::Service traitwhich runs on every RPC call and batch requests may call this middleware several times.In that way jsonrpsee can utilize the tower::ServiceBuilder and use the nice layer APIs but jsonrpsee has trait called
RpcServiceTwhich is very similar to the tower::Service trait but simplifies things with concrete types and doesn't require &mut self.The downside with having a different service trait than tower is that the built-in layers won't work on jsonrpsee.
Example of custom middleware implementation.
Resolves #1157, resolves #1128, resolves #989, resolves #1190
Benches