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

Test jsonrpsee v0.20 #691

Closed
wants to merge 12 commits into from
Closed

Test jsonrpsee v0.20 #691

wants to merge 12 commits into from

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Dec 7, 2023

Test PR for #692

@haerdib haerdib self-assigned this Dec 7, 2023
@haerdib haerdib changed the title Test jsonrpsee v017 Test jsonrpsee v0.17 Dec 7, 2023
@haerdib haerdib force-pushed the bh/test-jsonrpsee-v017 branch from 4e107d6 to ccbabcb Compare December 8, 2023 16:26
@haerdib haerdib changed the title Test jsonrpsee v0.17 Test jsonrpsee v0.20 Dec 8, 2023
// 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)))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use block_on on tokio multi-thread it will panic.

The best would would be to change this to:

#[maybe_async::sync_impl]
impl Request for JsonrpseeClient {
	fn request<serde_json::Value>(&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);
                });

                let rx = rx.blocking_recv().unwrap();
		let deserialized_value: R = serde_json::from_value(rx)?;
		Ok(deserialized_value)
	}
}

Otherwise, you need to create a single threaded tokio-runtime https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.new_current_thread

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with the solution you're providing is, that it enforces the creation of a tokio task / thread outside of the request function, therefore breaking our current sync-api. With the current examples, it runs into the following runtime error at the blocking_recv() part:

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.

I'm not entirely sure what we should do here - either provide an all-inclusive function with a handle try_current matching to whatever the user does outside, or enforce either tokio or non-tokio threading.

In the long run, probably the best thing would be to either remove sync-api altogether on our side or introduce a sync-api for jsonrspee without tokio enforcement.

Copy link

@niklasad1 niklasad1 Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I see sorry for giving a bad suggestion. That was news to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bad suggestion at all - it really helped me along the way. Thank you very much!

@haerdib
Copy link
Contributor Author

haerdib commented Feb 5, 2024

closing this as obsolete - we removed the sync api as of #699

@haerdib haerdib closed this Feb 5, 2024
@haerdib haerdib deleted the bh/test-jsonrpsee-v017 branch February 19, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants