-
Notifications
You must be signed in to change notification settings - Fork 176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: jsonrpsee as service
and low-level API for more fine-grained API to disconnect peers etc
#1224
Conversation
@@ -0,0 +1,264 @@ | |||
// Copyright 2019-2021 Parity Technologies (UK) Ltd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//cc @xlc
This is the suggested API to disconnect peers etc, it's still just a PoC and I need to polish it.
Is it sufficient for your proxy/load balancer implementation?
This example just "disconnects peers" that don't comply with the "dummy rate limit middleware" and blacklists the IP addr.
methods: impl Into<Methods>, | ||
stop_handle: StopHandle, | ||
) -> TowerService<RpcMiddleware, HttpMiddleware> { | ||
let conn_id = self.conn_id.fetch_add(1, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could have been a u32 but it's so annoying with mutable stuff in FnMut closure's such as hyper::service_fn
} | ||
|
||
/// See [`Builder::max_request_body_size`](method@Builder::max_request_body_size) for documentation. | ||
pub fn max_request_body_size(mut self, size: u32) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is duplicated :(
we could add a trait for but it doesn't seems worth it for now
/// | ||
/// # Note | ||
/// This is similar to [`hyper::service::service_fn`]. | ||
#[derive(Debug, Clone)] | ||
pub struct TowerService<L> { | ||
pub struct TowerServiceNoHttp<L> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed on naming,
I think this shouldn't be used by external user in the "normal case" because the http middleware from jsonrpsee::ServerBuilder isn't used then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it may be possible that someone wants to wrap this on-top of their own tower http middleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed this to jsonrpsee internal thingy now
/// if you want only to enable `rpc_middleware` then [`TowerServiceNoHttp`] | ||
/// can be used. | ||
#[derive(Debug)] | ||
pub struct TowerService<RpcMiddleware, HttpMiddleware> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced this to enable both rpc_middleware and http_middleware
this is useful for instance when using the hyper_service_fn
see the jsonrpsee_as_service
example which is using this
content_type.and_then(|val| val.to_str().ok()).map_or(false, |content| { | ||
content.eq_ignore_ascii_case("application/json") | ||
|| content.eq_ignore_ascii_case("application/json; charset=utf-8") | ||
|| content.eq_ignore_ascii_case("application/json;charset=utf-8") | ||
}) | ||
} | ||
|
||
pub(crate) async fn reject_connection(socket: tokio::net::TcpStream) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to the jsonrpsee::TowerService
service
and low-level API for more fine-grained API to disconnect peers etc
service
and low-level API for more fine-grained API to disconnect peers etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did read the code and give it a tick, but I do not feel able to give any feedback :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I believe the low-level API enables users to implement all use-cases, and handles most future requests for other features. Being low-level API seems worth it to me, especially when most users will not bother implementing it! 👍
Co-authored-by: Alexandru Vasile <[email protected]>
This PR adds two new APIs (in order for users to manage the state themselves):
Limitations
The API to manage the websocket connection is quite high-level, such that this design doesn't allow to disconnect peers because of lower-level WebSocket details such as if the ping wasn't answered in time, send took longer than some hard limit, message buffer was exceeded x times, and similar.
Annoyance
As demonstrated in the added examples, it's a bit annoying for folks to bother with "server handle and stop handles" but I don't want to make such big refactor for that.
To implement "grafana metrics or something equivalent to the old RpcLogger" then the jsonrpsee as service must be used which is a bit harder to implement I reckon.
Examples
Two examples of how to use the
hyper::service fn
to utilize the jsonrpsee TowerServiceBasically, we want people to be able to disconnect misbehaving peers, and this is probably the cleanest way to keep jsonrpsee
as simple as possible.
Resolves #1016, resolves #1014, resolves #1000