You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This change replaces blocking on the async runtime with a proper await when fetching network state for peer_getBasicInfo.
Using futures::executor::block_on inside jsonrpsee RPC handlers blocks the async runtime and can stall other RPC under load; this matches the async pattern already used in txwatch.rs.
Overall: The change is correct in intent — replacing futures::executor::block_on with async/.await is the right thing to do. Blocking the async runtime inside an RPC handler can indeed stall the entire jsonrpsee executor under load. Thumbs up on the motivation.
One issue to flag:
The PR adds async-trait as a new crate dependency in node/Cargo.toml, but this is unnecessary. The codebase already uses jsonrpsee's re-export of async_trait — you can see it in txwatch.rs:
use jsonrpsee::{
core::{async_trait, SubscriptionResult},
proc_macros::rpc,
The PR should use jsonrpsee::core::async_trait instead of pulling in a separate async-trait dependency. This:
Avoids adding a redundant dependency to node/Cargo.toml
Actually matches the pattern in txwatch.rs (which the PR description claims to follow)
Guarantees the same async_trait version that jsonrpsee expects
The fix would be:
Remove the async-trait.workspace = true line from node/Cargo.toml
Changeuse async_trait::async_trait; to import from jsonrpsee instead:
use jsonrpsee::{core::RpcResult, core::async_trait, proc_macros::rpc,RpcModule};
(or group it however you prefer)
Everything else looks good — the async fn signature on the trait, the #[async_trait] on the impl block, and the .await on network.network_state() are all correct. The error handling is unchanged, which is fine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change replaces blocking on the async runtime with a proper await when fetching network state for peer_getBasicInfo.
Using futures::executor::block_on inside jsonrpsee RPC handlers blocks the async runtime and can stall other RPC under load; this matches the async pattern already used in txwatch.rs.
made by mooncitydev