Skip to content
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

Remove future block_on to update to jsonrpsee v17+ #692

Closed
haerdib opened this issue Dec 8, 2023 · 4 comments · Fixed by #699
Closed

Remove future block_on to update to jsonrpsee v17+ #692

haerdib opened this issue Dec 8, 2023 · 4 comments · Fixed by #699
Assignees
Labels
F2-bug Something isn't working F9-dependencies Pull requests that update a dependency file Q3-substantial

Comments

@haerdib
Copy link
Contributor

haerdib commented Dec 8, 2023

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
@haerdib haerdib added F2-bug Something isn't working Q3-substantial labels Dec 8, 2023
@haerdib haerdib changed the title Remove future block_on Remove future block_on to update to jsonrpsee v17+ Dec 8, 2023
@haerdib haerdib added the F9-dependencies Pull requests that update a dependency file label Dec 8, 2023
@haerdib
Copy link
Contributor Author

haerdib commented Dec 8, 2023

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.

@haerdib haerdib self-assigned this Dec 14, 2023
@haerdib
Copy link
Contributor Author

haerdib commented Dec 18, 2023

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?

@Niederb
Copy link
Contributor

Niederb commented Dec 18, 2023

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

@masapr
Copy link
Collaborator

masapr commented Dec 19, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F2-bug Something isn't working F9-dependencies Pull requests that update a dependency file Q3-substantial
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants