-
Notifications
You must be signed in to change notification settings - Fork 2.7k
RpcHandlers Refactorings #6846
RpcHandlers Refactorings #6846
Changes from 13 commits
529dac2
2c19950
3da0cef
23e5640
901754e
94c525c
641d9b9
c43f762
1439bba
5f61b4f
172ccdd
70325ee
4b54972
3f83364
bce71e9
8bd60aa
b8f42a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,7 +105,7 @@ impl<Hash> ManualSeal<Hash> { | |
| } | ||
| } | ||
|
|
||
| impl<Hash: Send + 'static> ManualSealApi<Hash> for ManualSeal<Hash> { | ||
| impl<Hash: std::fmt::Debug + Send + 'static> ManualSealApi<Hash> for ManualSeal<Hash> { | ||
| fn create_block( | ||
| &self, | ||
| create_empty: bool, | ||
|
|
@@ -122,7 +122,8 @@ impl<Hash: Send + 'static> ManualSealApi<Hash> for ManualSeal<Hash> { | |
| sender: Some(sender), | ||
| }; | ||
| sink.send(command).await?; | ||
| receiver.await? | ||
| let res = receiver.await?; | ||
| res | ||
|
||
| }.boxed(); | ||
|
|
||
| Box::new(future.map_err(Error::from).compat()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
|
@@ -115,8 +116,14 @@ 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you absolutely need this function, or can you add the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, fair enough : ) |
||
| } | ||
|
|
||
|
|
||
seunlanlege marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// Sinks to propagate network status updates. | ||
| /// For each element, every time the `Interval` fires we push an element on the sender. | ||
| #[derive(Clone)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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..
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you moving that out again? It wasn't nice inside SubstrateCli?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, having the spec_factory in the cli complicates the API for paritytech/substrate-test-runner, where users would also have to provide
impl SubstrateClisimply because we want the spec_factory. Splitting it out keeps our API lean and simple.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it's for this? paritytech/substrate-test-runner#2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it related in any way with this? #6651
Are you trying to run nodes inside tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seunlanlege I don't see the full picture yet so bare with me I might be completely wrong. If you can provide me documentation and more information on the project it would help me understand better the big picture.
Wild guess: I don't think you need sc-cli at all. sc-cli is for building clis, here you are building a library, you should be using sc-service and use the sc_service::Configuration object directly. It's sc-service that is the glue code and the center of everything, not sc-cli. You can't just pick the elements you want from sc-cli without applying to the full logic of sc-cli's usage.
Example: if you pass the arguments of the cli by code, I'm confident that this is wrong. sc-cli is really only meant for building a binary and the arguments are passed by a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(In other words: sc-cli must be a dependency of a bin crate, not a lib crate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cecton Please show them your code you have done for the polkadot test service. Which solves this problem AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/paritytech/polkadot/blob/master/node/test-service/src/lib.rs
@seunlanlege This is a testing crate for polkadot. It uses sc-service directly. I just pass the chain-spec I want, tweak the sc_service::Configuration object the way I want, and that's how I can run a node. The nodes can communicate with each other using memory network so it doesn't open a port on the machine and I don't need to know the port number.
The PR #6651 is meant to improve testing nodes by providing an async runtime for the test and a timeout.
The PR #6555 is also meant to improve testing nodes by providing a "wait_for_blocks" function (to check that blocks are being created) and "send_transaction" to arbitrarily send a transaction to the node.