[JSON-RPC] - Upgrade jsonrpsee to v0.16.1 #6387
Conversation
patrickkuo
commented
Nov 26, 2022
•
patrickkuo
commented
- removed WsServerBuilder and all websocket server code, jsonrpsee now support http and ws on the same server.
patrickkuo
force-pushed
the
pat/jsonrpsee_upgrade
branch
from
b4da44d to
c97d723
Compare
patrickkuo
requested review from
bmwill,
longbowlu,
Jordan-Mysten and
666lcz
crates/sui-json-rpc/src/metrics.rs
Outdated
| fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
| let handle = Handle::current(); | ||
| let _ = handle.enter(); | ||
| let mut v = futures::executor::block_on(self.inner.lock()); |
There was a problem hiding this comment.
You cannot use another executor inside of a tokio runtime. This can lead to deadlocking. The way that tower services are supposed to work is that you have an owned service, not a wrapper around a shared one with Mutexes
There was a problem hiding this comment.
Generally services end up being clone-able and they're just cloned to handle concurrent requests.
There was a problem hiding this comment.
Sadly jsonrpsee's TowerService is not clone-able :( , I will fix this upstream
There was a problem hiding this comment.
PR created upstream, paritytech/jsonrpsee#950
I am using a forked version instead until next jsonrpsee release
crates/sui-json-rpc/src/metrics.rs
Outdated
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct JsonRpcMetricService<S> { | ||
| inner: Arc<Mutex<S>>, |
There was a problem hiding this comment.
This should just be
| inner: Arc<Mutex<S>>, | |
| inner: S, |
| 0.001, 0.005, 0.01, 0.05, 0.1, 0.25, 0.5, 1., 2.5, 5., 10., 20., 30., 60., 90., | ||
| ]; | ||
|
|
||
| impl<S> JsonRpcMetricService<S> { |
There was a problem hiding this comment.
I don't see this layer actually used anywhere, am i just missing where its used?
There was a problem hiding this comment.
This is the service used by the MetricsLayer, MetricsLayer is added to the middleware
removed WsServerBuilder and all websocket server code, jsonrpsee now support http and ws on the same server.
patrickkuo
force-pushed
the
pat/jsonrpsee_upgrade
branch
from
dc4e800 to
c532b64
Compare
| } | ||
| } | ||
|
|
||
| fn is_json(content_type: Option<&hyper::header::HeaderValue>) -> bool { |
There was a problem hiding this comment.
this looks fragile. Is it too expensive to convert to lower case and check contains?
| self.inner.poll_ready(cx).map_err(Into::into) | ||
| } | ||
|
|
||
| fn call(&mut self, req: Request<Body>) -> Self::Future { |
There was a problem hiding this comment.
is this layer called before or after request deserialization? If it's before, it'd be helpful to record deser errors separately
There was a problem hiding this comment.
the layer wraps the inner call, we can add logic before or after the actual jsonrpsee service call
sui/crates/sui-json-rpc/src/metrics.rs
Lines 131 to 132 in 4cf4b90
This PR only upgrade the existing code to use new version of jsonrpsee, I can add deser errors in next PR.
Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com>
* upgrade jsonrpsee removed WsServerBuilder and all websocket server code, jsonrpsee now support http and ws on the same server. * regen hakari * remove ws config from test and swarm * fixup after rebase * make service clonable instead of using mutex * Update crates/test-utils/src/network.rs Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com> Co-authored-by: Lu Zhang <8418040+longbowlu@users.noreply.github.com>
