-
Notifications
You must be signed in to change notification settings - Fork 2.7k
impl Debug for sc_service::Configuration #6400
Changes from all commits
2dd4f9a
fb31420
563da0a
7b0590e
d9e6275
d53a1c2
5cb14a0
fc69a32
1a67120
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ use prometheus_endpoint::Registry; | |
use tempfile::TempDir; | ||
|
||
/// Service configuration. | ||
#[derive(Debug)] | ||
pub struct Configuration { | ||
/// Implementation name | ||
pub impl_name: &'static str, | ||
|
@@ -42,7 +43,7 @@ pub struct Configuration { | |
/// Node role. | ||
pub role: Role, | ||
/// How to spawn background tasks. Mandatory, otherwise creating a `Service` will error. | ||
pub task_executor: Arc<dyn Fn(Pin<Box<dyn Future<Output = ()> + Send>>, TaskType) + Send + Sync>, | ||
pub task_executor: TaskExecutor, | ||
/// Extrinsic pool configuration. | ||
pub transaction_pool: TransactionPoolOptions, | ||
/// Network configuration. | ||
|
@@ -120,7 +121,7 @@ pub enum TaskType { | |
} | ||
|
||
/// Configuration of the client keystore. | ||
#[derive(Clone)] | ||
#[derive(Debug, Clone)] | ||
pub enum KeystoreConfig { | ||
/// Keystore at a path on-disk. Recommended for native nodes. | ||
Path { | ||
|
@@ -143,7 +144,7 @@ impl KeystoreConfig { | |
} | ||
} | ||
/// Configuration of the database of the client. | ||
#[derive(Clone, Default)] | ||
#[derive(Debug, Clone, Default)] | ||
pub struct OffchainWorkerConfig { | ||
/// If this is allowed. | ||
pub enabled: bool, | ||
|
@@ -152,7 +153,7 @@ pub struct OffchainWorkerConfig { | |
} | ||
|
||
/// Configuration of the Prometheus endpoint. | ||
#[derive(Clone)] | ||
#[derive(Debug, Clone)] | ||
pub struct PrometheusConfig { | ||
/// Port to use. | ||
pub port: SocketAddr, | ||
|
@@ -199,6 +200,7 @@ impl Default for RpcMethods { | |
} | ||
|
||
/// The base path that is used for everything that needs to be write on disk to run a node. | ||
#[derive(Debug)] | ||
pub enum BasePath { | ||
/// A temporary directory is used as base path and will be deleted when dropped. | ||
#[cfg(not(target_os = "unknown"))] | ||
|
@@ -253,3 +255,69 @@ impl std::convert::From<PathBuf> for BasePath { | |
BasePath::new(path) | ||
} | ||
} | ||
|
||
type TaskExecutorInner = Arc<dyn Fn(Pin<Box<dyn Future<Output = ()> + Send>>, TaskType) + Send + Sync>; | ||
|
||
/// Callable object that execute tasks. | ||
/// | ||
/// This struct can be created easily using `Into`. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ## Using tokio | ||
/// | ||
/// ``` | ||
/// # use sc_service::TaskExecutor; | ||
/// # mod tokio { pub mod runtime { | ||
/// # #[derive(Clone)] | ||
/// # pub struct Runtime; | ||
/// # impl Runtime { | ||
/// # pub fn new() -> Result<Self, ()> { Ok(Runtime) } | ||
/// # pub fn handle(&self) -> &Self { &self } | ||
/// # pub fn spawn(&self, _: std::pin::Pin<Box<dyn futures::future::Future<Output = ()> + Send>>) {} | ||
/// # } | ||
/// # } } | ||
/// use tokio::runtime::Runtime; | ||
/// | ||
/// let runtime = Runtime::new().unwrap(); | ||
/// let handle = runtime.handle().clone(); | ||
/// let task_executor: TaskExecutor = (move |future, _task_type| { | ||
/// handle.spawn(future); | ||
/// }).into(); | ||
/// ``` | ||
/// | ||
/// ## Using async-std | ||
/// | ||
/// ``` | ||
/// # use sc_service::TaskExecutor; | ||
/// # mod async_std { pub mod task { | ||
/// # pub fn spawn(_: std::pin::Pin<Box<dyn futures::future::Future<Output = ()> + Send>>) {} | ||
/// # } } | ||
/// let task_executor: TaskExecutor = (|future, _task_type| { | ||
/// async_std::task::spawn(future); | ||
/// }).into(); | ||
/// ``` | ||
#[derive(Clone)] | ||
pub struct TaskExecutor(TaskExecutorInner); | ||
|
||
impl std::fmt::Debug for TaskExecutor { | ||
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { | ||
write!(f, "TaskExecutor") | ||
} | ||
} | ||
|
||
impl<F> std::convert::From<F> for TaskExecutor | ||
where | ||
F: Fn(Pin<Box<dyn Future<Output = ()> + Send>>, TaskType) + Send + Sync + 'static, | ||
{ | ||
fn from(x: F) -> Self { | ||
Self(Arc::new(x)) | ||
} | ||
} | ||
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. This basically makes the struct callable. I'm not sure if this is the best solution.
Any advice on what would be the cleanest implementation? 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. As all the 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. While I know it's not much, but having so many typedefs and wrapping structs is in my opinion a major pain point in the Substrate code base as whole. 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. Oh well. My goal was not to wrap it, I just needed to implement Debug and there are only 3 possible ways:
I think the trade-off is fine here especially with the 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 other good point is that you don't need to import Arc and Pin, not even TaskExecutor, you just pass the closure and use 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 think adding some documentation would indeed be a good idea :) 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. done 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 done (I just noticed your comment now) |
||
|
||
impl TaskExecutor { | ||
/// Spawns a new asynchronous task. | ||
pub fn spawn(&self, future: Pin<Box<dyn Future<Output = ()> + Send>>, task_type: TaskType) { | ||
self.0(future, task_type) | ||
} | ||
} |
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.
Whoa, didn't know you could impl for
dyn
things! What's the difference between doing this and impling forT where T: ChainSpec
?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.
🤔 I have no idea. Maybe someone else know
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.
The compiler will refuse to compile
impl<T: ChainSpec> Debug for T {}
That being said, I don't think it's a good idea to implement
Debug
on trait objects. Not because it's wrong per se, but because it is a very obscure thing to do.