Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

RpcHandlers Refactorings #6846

Merged
17 commits merged into from
Aug 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ pub fn new_full(config: Configuration)
}

pub fn new_light_base(config: Configuration) -> Result<(
TaskManager, Arc<RpcHandlers>, Arc<LightClient>,
TaskManager, RpcHandlers, Arc<LightClient>,
Arc<NetworkService<Block, <Block as BlockT>::Hash>>,
Arc<sc_transaction_pool::LightPool<Block, LightClient, sc_network::config::OnDemand<Block>>>
), ServiceError> {
Expand Down
3 changes: 2 additions & 1 deletion client/service/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ test-helpers = []
derive_more = "0.99.2"
futures01 = { package = "futures", version = "0.1.29" }
futures = { version = "0.3.4", features = ["compat"] }
jsonrpc-pubsub = "14.2.0"
jsonrpc-pubsub = "14.2"
jsonrpc-core = "14.2"
rand = "0.7.3"
parking_lot = "0.10.0"
lazy_static = "1.4.0"
Expand Down
4 changes: 2 additions & 2 deletions client/service/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ pub fn build_offchain_workers<TBl, TBackend, TCl>(
/// Spawn the tasks that are required to run a node.
pub fn spawn_tasks<TBl, TBackend, TExPool, TRpc, TCl>(
params: SpawnTasksParams<TBl, TCl, TExPool, TRpc, TBackend>,
) -> Result<Arc<RpcHandlers>, Error>
) -> Result<RpcHandlers, Error>
where
TCl: ProvideRuntimeApi<TBl> + HeaderMetadata<TBl, Error=sp_blockchain::Error> + Chain<TBl> +
BlockBackend<TBl> + BlockIdTo<TBl, Error=sp_blockchain::Error> + ProofProvider<TBl> +
Expand Down Expand Up @@ -540,7 +540,7 @@ pub fn spawn_tasks<TBl, TBackend, TExPool, TRpc, TCl>(
);
let rpc = start_rpc_servers(&config, gen_handler)?;
// This is used internally, so don't restrict access to unsafe RPC
let rpc_handlers = Arc::new(RpcHandlers(gen_handler(sc_rpc::DenyUnsafe::No)));
let rpc_handlers = RpcHandlers(Arc::new(gen_handler(sc_rpc::DenyUnsafe::No).into()));

// Telemetry
let telemetry = config.telemetry_endpoints.clone().and_then(|endpoints| {
Expand Down
8 changes: 7 additions & 1 deletion client/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ impl<T: MallocSizeOf> MallocSizeOfWasm for T {}
impl<T> MallocSizeOfWasm for T {}

/// RPC handlers that can perform RPC queries.
pub struct RpcHandlers(sc_rpc_server::RpcHandler<sc_rpc::Metadata>);
#[derive(Clone)]
pub struct RpcHandlers(Arc<jsonrpc_core::MetaIoHandler<sc_rpc::Metadata>>);

impl RpcHandlers {
/// Starts an RPC query.
Expand All @@ -115,6 +116,11 @@ impl RpcHandlers {
.map(|res| res.expect("this should never fail"))
.boxed()
}

/// Provides access to the underlying `MetaIoHandler`
pub fn io_handler(&self) -> Arc<jsonrpc_core::MetaIoHandler<sc_rpc::Metadata>> {
self.0.clone()
}
Comment on lines +121 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you absolutely need this function, or can you add the MetaIoHandler methods directly to RpcHandlers? If so, that would be cleaner and more abstract IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so the use case for this is to allow end users (and me 😇 ) use the jsonrpc-core-client local transport, which takes a Deref<Target=MetaIoHandler> + 'static. Hence the need to have the MetaIoHandler behind an Arc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, fair enough : )

}

/// Sinks to propagate network status updates.
Expand Down
9 changes: 6 additions & 3 deletions client/service/src/task_manager/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,12 @@ impl TaskManager {
}
}

/// Set what the task manager should keep alive.
pub(super) fn keep_alive<T: 'static + Send + Sync>(&mut self, to_keep_alive: T) {
self.keep_alive = Box::new(to_keep_alive);
Copy link
Contributor

Choose a reason for hiding this comment

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

@expenses I was sure that you did make a tuple magic thingy here that would make this function callable multiple times 🤔 why was is it gone? I'm missing something...

Copy link
Member

Choose a reason for hiding this comment

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

You mean keep_alive((SOmehing, else, yolo))

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally mean S0mething, else, yolo.

I think she refactored her code before pushing and since keep_alive here is privat-ish she didn't foresee it could be misused.

Clearly something to fix, it's a pitfall.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah SOmething :P You should change your font to distinguish O and 0 :P

Copy link
Contributor

Choose a reason for hiding this comment

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

The mix between the monospace font and sans font tricked my eyes

/// Set what the task manager should keep alive, can be called multiple times.
pub fn keep_alive<T: 'static + Send + Sync>(&mut self, to_keep_alive: T) {
// allows this fn to safely called multiple times.
use std::mem;
let old = mem::replace(&mut self.keep_alive, Box::new(()));
self.keep_alive = Box::new((to_keep_alive, old));
Comment on lines -339 to +344
Copy link
Contributor

Choose a reason for hiding this comment

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

If you absolutely need to call this function more than once, then this is fine..

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just to tempting to call it more than once imo. If there is no downside we should do it

}

/// Register another TaskManager to terminate and gracefully shutdown when the parent
Expand Down
4 changes: 2 additions & 2 deletions utils/browser/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use wasm_bindgen::prelude::*;
use futures::{
prelude::*, channel::{oneshot, mpsc}, compat::*, future::{ready, ok, select}
};
use std::{sync::Arc, pin::Pin};
use std::pin::Pin;
use sc_chain_spec::Extension;
use libp2p_wasm_ext::{ExtTransport, ffi};

Expand Down Expand Up @@ -124,7 +124,7 @@ struct RpcMessage {
}

/// Create a Client object that connects to a service.
pub fn start_client(mut task_manager: TaskManager, rpc_handlers: Arc<RpcHandlers>) -> Client {
pub fn start_client(mut task_manager: TaskManager, rpc_handlers: RpcHandlers) -> Client {
// We dispatch a background task responsible for processing the service.
//
// The main action performed by the code below consists in polling the service with
Expand Down