Closed haerdib closed 11 months ago
niklasad1
commented
11 months ago Hmm, thanks for raising the issue.
Are you sure that v0.17 suffers from this issue? I suspect that https://github.com/paritytech/jsonrpsee/pull/1145 could be a regression but it was introduced first in v0.20.
Can you please add instructions how to reproduce this?
haerdib
commented
11 months ago Yes, it is happening with v0.17.1.
Steps to reproduce:
Run a Substate node (i'm doing it with docker):
docker run -p 9944:9944 -p 9933:9933 -p 30333:30333 parity/substrate:latest --dev --rpc-external
Run the api-client benchmark example. I have created two branches, one with v 0.16 (works), one with v0.17 (does not work).
Build and run the example with the cargo command: cargo run -p ac-examples --example benchmark_bulk_xt.
The code in the example: https://github.com/scs/substrate-api-client/blob/bh/test-jsonrpsee/examples/examples/benchmark_bulk_xt.rs
I've tried to remove the clutter around the jsonrpsee client as much as possible. The relevant part (I believe) is now here:
while nonce < first_nonce + 60 {
// Create the extrinsic.
let xt = api.compose_extrinsic_offline(call.clone(), nonce);
let xt_bytes: Bytes = xt.encode().into();
let hex_encoded_xt = rpc_params![xt_bytes];
println!("Sending extrinsic with nonce {}", nonce);
// Send the extrinsic with jsonrpsee
let _xt_hash: H256 =
api.client().request("author_submitExtrinsic", hex_encoded_xt).unwrap();
nonce += 1;
}Where the api.client().request() is a block_on wrapper to make it a synchronous call. Relevant code part:
https://github.com/scs/substrate-api-client/blob/bh/test-jsonrpsee/src/rpc/jsonrpsee_client/mod.rs#L68-L73
impl Request for JsonrpseeClient {
fn request<R: DeserializeOwned>(&self, method: &str, params: RpcParams) -> Result<R> {
block_on(self.inner.request(method, RpcParamsWrapper(params)))
.map_err(|e| Error::Client(Box::new(e)))
}
}It's a bad practice to mix tokio with another async runtime such as futures::executor::block_on. Most likely it's really just that causes this to get stuck, see https://github.com/tokio-rs/tokio/issues/2603 and the reason is that jsonrpsee migrated from futures-channels to tokio-channels which Client::request and Client::subscribe awaits on..
EDIT: I tried your example and I was able to verify by printlns that the message was received by client and sent out on the oneshot channel but awaiting on the future doesn't return.
Thus, I'm quite certain that this is because jsonrpsee moved away from futures channels to tokio channels.
If you could refactor your sync API to something like then I think it should work as v0.16:
#[maybe_async::sync_impl]
impl Request for JsonrpseeClient {
fn request<R: DeserializeOwned>(&self, method: &str, params: RpcParams) -> Result<R> {
let (tx, rx) = tokio::sync::oneshot();
tokio::spawn(async move {
let res = self.inner.request(method, RpcParamsWrapper(params))).await.map_err(|e| Error::Client(Box::new(e)));
tx.send(res);
});
rx.blocking_recv().unwrap()
}
}Yeah, ok I tested by switching out tokio::sync::oneshot to futures_channel::oneshot and then this issue goes away... So indeed, futures::executor::block_on messes up tokio somehow.
//cc @jsdw @lexnv I don't regard this as jsonrpsee issue as the library itself is tightly-coupled to tokio but do think we should provide an sync API for making clients requests?
haerdib
commented
11 months ago Thank you very much for the investigation! I indeed did not know this could be a problem. Time to switch out futures completely.. As this is clearly an issue on our side, I'm closing this issue. And ofc, we would welcome a sync API ;)
jsdw
commented
11 months ago Re a sync APIs; I woudln't mind adding them in theory if:
a) It wasn't too complex to do. b) Users of the sync APIs didn't explicitly need to know about tokio at all.
The above code is a bit of an exception though in that it runs in a tokio runtime (#[tokio::main]), but then blocks on this call instead of using tokio, which I can only imagine leads to some task being scheduled on a tokio worker thread that already has this blocking task running on that doesn't yield to let the it progress and ends up deadlocking itself, or something like that.
That all said, we don't have any need for sync APIs internally, and so it's not the highest priority for us really.
I'm currently trying to upgrade our jsonrpsee client from v0.16 to v0.20. However, I'm hitting a problem I could not solve yet.. maybe you have an idea what's the problem here.
To test our wrapper client around jsonrpsee, we spam multiple Substrate node request in a synchronous manner. E.g. the second request is only sent after the first request was fired. There is no subscription involved.
With v0.16 that works without a problem : 60 and above requests are sent rapidly within less than a second. v0.16 logs:
However, after upgrading (both v0.17 and v0.20 have the same problem); The 58th message gets a time-out after one minute:
There was nothing changed on node (server) side, and nothing in the client code as well, apart from the necessary renaming. The logs do not show anything obvious. I did not find anything related to this problem in the changelogs either. Am I missing something?