scs/substrate-api-client

Remove future `block_on` to update to jsonrpsee v17+

Closed this issue · 4 comments

While trying to update the jsonrpsee client to a newer version (#671), the client (most likely) deadlocked itself after 59 requests (see https://github.com/scs/substrate-api-client/actions/runs/7126321622/job/19404332411). This is because we are using future::executor::block_on within tokio runtime to make async code synchronous. More info: paritytech/jsonrpsee#1259

To ensure this deadlock does not happen, I currently see two options:

  1. Replace futures with a tokio block.
    Example replacement:
    Old code:
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)))
	}
}

New code (didn't find a better way yet, should be improved):

impl Request for JsonrpseeClient {
	fn request<R: DeserializeOwned>(&self, method: &str, params: RpcParams) -> Result<R> {
                // Catch the current runtime.
		let handle = match Handle::try_current() {
			Ok(handle) => handle,
			Err(_) => {
				// We are not inside a tokio runtime, so lets start one.
				let rt =
					tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap();
				rt.handle().clone()
			},
		};

		let client = self.inner.clone();
		let method_string = method.to_string();

		// The inner jsonrpsee client must not deserialize to the `R` value, because the return value must
		// implement `Send`. But we do not want to enforce the `R` value to implement this solely because we need
		// to de-async something. Therefore, the deserialization must happen outside of the newly spawned thread.
		// We need to spawn a new thread because tokio does not allow the blocking of the main thread:
		// ERROR: Cannot block the current thread from within a runtime.
		// This happens because a function attempted to block the current thread while the thread is being used to drive asynchronous tasks.
		let string_answer: Value = std::thread::spawn(move || {
			handle.block_on(client.request(&method_string, RpcParamsWrapper(params)))
		})
		.join()
		.unwrap()
		.map_err(|e| Error::Client(Box::new(e)))?;

		let deserialized_value: R = serde_json::from_value(string_answer)?;
		Ok(deserialized_value)
	}
}
  1. Open PR on jsonrpsee providing sync api

Marking this as a bug, because I'm not sure if this may cause problems we simply haven't detected yet. It only revealed itself because jsonrpsee switched to oneshot channels from tokio in v17. There may be problems however with other block_ons, like in the subscription function.

I've experimented a little with tokio and block_on and reached the following conclusion: It needs quite some workaround to make it work. Request is one thing, with subscription it gets even more complex. So I'm currently wondering if it makes sense to offer a sync representation of jsonrpsee if jsonrpsee does not offer one themselfes. Reasons against offering a sync interface:

  1. jsonrpsee enforces tokio (or wasm) runtime anyway - I don't see the benefit of using tokio in a sync environment.
  2. It will cost us quite some effort (and maybe even adaption of the code that affects the whole interface, including the other clients), to offer a sync interface.

Problems I encountered during implementing a sync interface:

  • Tokio multi thread runtime does not allow blocking of the driving async thread. A solution would be to spawn a single thread inside the sync function (request and subscribe) and use a block_on function inside this thread. However, this poses a problem that we need the return value to implement Send, which would need an extra restriction in our interface trait. Or some other workaround, such leaving the thread spawning to the user. But if the thread spawning is forgotten, it will result in a runtime error, not compilation.
  • Tokio single threaded runtime does allow the usage of blocking. But using this runtime we a have a problem with spawning the jsonrpsee client (which enforces multi-threaded runtime, to my understanding). So another workaround for spawning the jsonrpsee client would be necessary

All in all, it will be quite a fragile setup, that either needs us to implement a very robust interface, catching all possible user variations (using multi-threaded tokio, single thread and so on) or a solution the needs a very robust user code, e.g. never use tokio multi runtime or spawn a thread whenever a node request is issued.... The bad thing: Such tokio errors result in runtime errors, not compilation.

My conclusion: The cleanest solution would be to implement a sync interface of jsonrspee itself, or remove the sync jsonrpsee interface completely for now.

@masapr, @Niederb what do you think?

Hmmm, that's annoying that the block_on is causing so much trouble. It would be nice if there was a runtime independent implementation, but probably it cannot really be done.
My opinion: If jsonrpsee had a sync interface we could use that, but I don't see that happening anytime soon. So I would opt for removing sync support for jsonrpsee

I would also say, let's remove the support for sync jsonrpsee. As long as we have clear examples, how the api-client can be used for a sync-usecase, I think it should be fine. So, generally I would rather make sure we have clear examples for different usecases, and not provide solutions, that are unstable / only work with specific setups.