Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

RpcClient no longer panics in a tokio multi-threaded runtime #16385

Merged
merged 1 commit into from
Apr 5, 2021

Conversation

mvines
Copy link
Contributor

@mvines mvines commented Apr 5, 2021

Post-tokio 1 upgrade (Solana 1.6.2), RpcClient would panic if run from within a tokio runtime. Fix it by shuttling the blocking requests off to a different tokio thread

One glorious day we'll have an async RpcClient (blocked on the jsonrpc crates turning into jsonrpsee crates for a tokio 1 update here), after which we can autogen the RpcClient bindings. For now RpcClient working in async is better than nothing though.

@mvines mvines added the v1.6 label Apr 5, 2021
let request_id = 1;

let request_json = request.build_request_json(request_id, params);
let request_id = self.request_id.fetch_add(1, Ordering::Relaxed);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Freebie!


#[tokio::test(flavor = "current_thread")]
#[should_panic(expected = "can call blocking only when running on the multi-threaded runtime")]
async fn http_sender_ontokio_current_thread_should_panic() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't great, but I don't see a way around. Fortunately [tokio::main] creates a multi_thread runtime, so most users won't see this (but #[tokio::test], sans flavour, is current_thread)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, obv not ideal but okay for now. I haven't thought of anything better.

@mvines mvines requested a review from CriesofCarrots April 5, 2021 20:28

#[tokio::test(flavor = "current_thread")]
#[should_panic(expected = "can call blocking only when running on the multi-threaded runtime")]
async fn http_sender_ontokio_current_thread_should_panic() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see, obv not ideal but okay for now. I haven't thought of anything better.

@codecov
Copy link

codecov bot commented Apr 5, 2021

Codecov Report

Merging #16385 (0be8ce6) into master (b521f50) will increase coverage by 0.0%.
The diff coverage is 75.3%.

@@           Coverage Diff           @@
##           master   #16385   +/-   ##
=======================================
  Coverage    80.1%    80.1%           
=======================================
  Files         411      411           
  Lines      110155   110194   +39     
=======================================
+ Hits        88268    88366   +98     
+ Misses      21887    21828   -59     

@mvines mvines merged commit a4f0d86 into solana-labs:master Apr 5, 2021
@mvines mvines deleted the tm branch April 5, 2021 23:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants