-
Notifications
You must be signed in to change notification settings - Fork 2.6k
use ThreadPool to execute spawn_worker(fn) #3836
Changes from 2 commits
bbd6e42
df4d06d
9c87d8b
b7f48f1
6394319
742d72f
d09f65d
48899be
e5a0bd1
8fedb51
1114fa5
07cb32d
a37fc79
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -16,6 +16,8 @@ futures-timer = "0.4.0" | |||||
| hyper = "0.12.35" | ||||||
| hyper-tls = "0.3.2" | ||||||
| log = "0.4.8" | ||||||
| threadpool = "1.0" | ||||||
| num_cpus = "1.6" | ||||||
|
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.
Suggested change
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. sure |
||||||
| offchain-primitives = { package = "substrate-offchain-primitives", path = "./primitives" } | ||||||
| codec = { package = "parity-scale-codec", version = "1.0.0", features = ["derive"] } | ||||||
| parking_lot = "0.9.0" | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -33,19 +33,20 @@ | |||
|
|
||||
| #![warn(missing_docs)] | ||||
|
|
||||
| use std::{ | ||||
| fmt, | ||||
| marker::PhantomData, | ||||
| sync::Arc, | ||||
| }; | ||||
| extern crate num_cpus; | ||||
|
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. We use
Suggested change
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. should not be needed in 2018 edition?
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. I'm sorry I don't understand what 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. The usage of
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. Yup, just remove that line, fix the tests and we are good to go.
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
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. There are build results below the comments section on github: Actually it's not a test failing, but you need to run
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, I see.
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. I have committed the change of Cargo.lock . But there is a trouble, when I run 'cargo check' in the path ' ~/substrate/ ' , it shows that futures-io-preview can't compile.
error[E0106]: missing lifetime specifier error[E0106]: missing lifetime specifier error[E0106]: missing lifetime specifier error[E0106]: missing lifetime specifier error[E0106]: missing lifetime specifier error: aborting due to 6 previous errors
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. But your code seem to be compiling well on the CI machine.. Make sure you have the latest version of rustc and stable/nightly toolchains installed.
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. Here is my rust version. rustc 1.38.0-nightly (69656fa4c 2019-07-13) |
||||
| use std::{fmt, marker::PhantomData, sync::Arc}; | ||||
|
|
||||
| use client::runtime_api::ApiExt; | ||||
| use futures::future::Future; | ||||
| use log::{debug, warn}; | ||||
| use network::NetworkStateInfo; | ||||
| use primitives::{offchain, ExecutionContext}; | ||||
| use sr_primitives::{generic::BlockId, traits::{self, ProvideRuntimeApi}}; | ||||
| use transaction_pool::txpool::{Pool, ChainApi}; | ||||
| use sr_primitives::{ | ||||
| generic::BlockId, | ||||
| traits::{self, ProvideRuntimeApi}, | ||||
| }; | ||||
| use threadpool::ThreadPool; | ||||
| use transaction_pool::txpool::{ChainApi, Pool}; | ||||
|
|
||||
| mod api; | ||||
|
|
||||
|
|
@@ -55,132 +56,132 @@ pub use offchain_primitives::OffchainWorkerApi; | |||
|
|
||||
| /// An offchain workers manager. | ||||
| pub struct OffchainWorkers<Client, Storage, Block: traits::Block> { | ||||
| client: Arc<Client>, | ||||
| db: Storage, | ||||
| _block: PhantomData<Block>, | ||||
| client: Arc<Client>, | ||||
| db: Storage, | ||||
| _block: PhantomData<Block>, | ||||
| thread_pool: ThreadPool, | ||||
| } | ||||
|
|
||||
| impl<Client, Storage, Block: traits::Block> OffchainWorkers<Client, Storage, Block> { | ||||
| /// Creates new `OffchainWorkers`. | ||||
| pub fn new(client: Arc<Client>, db: Storage) -> Self { | ||||
| Self { | ||||
| client, | ||||
| db, | ||||
| _block: PhantomData, | ||||
| } | ||||
| } | ||||
| /// Creates new `OffchainWorkers`. | ||||
| pub fn new(client: Arc<Client>, db: Storage) -> Self { | ||||
| Self { | ||||
| client, | ||||
| db, | ||||
| _block: PhantomData, | ||||
| thread_pool: ThreadPool::new(num_cpus::get()), | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| impl<Client, Storage, Block: traits::Block> fmt::Debug for OffchainWorkers< | ||||
| Client, | ||||
| Storage, | ||||
| Block, | ||||
| > { | ||||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||||
| f.debug_tuple("OffchainWorkers").finish() | ||||
| } | ||||
| impl<Client, Storage, Block: traits::Block> fmt::Debug for OffchainWorkers<Client, Storage, Block> { | ||||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||||
| f.debug_tuple("OffchainWorkers").finish() | ||||
| } | ||||
| } | ||||
|
|
||||
| impl<Client, Storage, Block> OffchainWorkers< | ||||
| Client, | ||||
| Storage, | ||||
| Block, | ||||
| > where | ||||
| Block: traits::Block, | ||||
| Client: ProvideRuntimeApi + Send + Sync + 'static, | ||||
| Client::Api: OffchainWorkerApi<Block>, | ||||
| Storage: client::backend::OffchainStorage + 'static, | ||||
| impl<Client, Storage, Block> OffchainWorkers<Client, Storage, Block> | ||||
| where | ||||
| Block: traits::Block, | ||||
| Client: ProvideRuntimeApi + Send + Sync + 'static, | ||||
| Client::Api: OffchainWorkerApi<Block>, | ||||
| Storage: client::backend::OffchainStorage + 'static, | ||||
| { | ||||
| /// Start the offchain workers after given block. | ||||
| #[must_use] | ||||
| pub fn on_block_imported<A>( | ||||
| &self, | ||||
| number: &<Block::Header as traits::Header>::Number, | ||||
| pool: &Arc<Pool<A>>, | ||||
| network_state: Arc<dyn NetworkStateInfo + Send + Sync>, | ||||
| is_validator: bool, | ||||
| ) -> impl Future<Output = ()> where A: ChainApi<Block=Block> + 'static { | ||||
| let runtime = self.client.runtime_api(); | ||||
| let at = BlockId::number(*number); | ||||
| let has_api = runtime.has_api::<dyn OffchainWorkerApi<Block>>(&at); | ||||
| debug!("Checking offchain workers at {:?}: {:?}", at, has_api); | ||||
|
|
||||
| if has_api.unwrap_or(false) { | ||||
| let (api, runner) = api::AsyncApi::new( | ||||
| pool.clone(), | ||||
| self.db.clone(), | ||||
| at.clone(), | ||||
| network_state.clone(), | ||||
| is_validator, | ||||
| ); | ||||
| debug!("Spawning offchain workers at {:?}", at); | ||||
| let number = *number; | ||||
| let client = self.client.clone(); | ||||
| spawn_worker(move || { | ||||
| let runtime = client.runtime_api(); | ||||
| let api = Box::new(api); | ||||
| debug!("Running offchain workers at {:?}", at); | ||||
| let run = runtime.offchain_worker_with_context( | ||||
| &at, | ||||
| ExecutionContext::OffchainCall(Some((api, offchain::Capabilities::all()))), | ||||
| number, | ||||
| ); | ||||
| if let Err(e) = run { | ||||
| log::error!("Error running offchain workers at {:?}: {:?}", at, e); | ||||
| } | ||||
| }); | ||||
| futures::future::Either::Left(runner.process()) | ||||
| } else { | ||||
| futures::future::Either::Right(futures::future::ready(())) | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| /// Spawns a new offchain worker. | ||||
| /// | ||||
| /// We spawn offchain workers for each block in a separate thread, | ||||
| /// since they can run for a significant amount of time | ||||
| /// in a blocking fashion and we don't want to block the runtime. | ||||
| /// | ||||
| /// Note that we should avoid that if we switch to future-based runtime in the future, | ||||
| /// alternatively: | ||||
| /// TODO [ToDr] (#1458) we can consider using a thread pool instead. | ||||
| fn spawn_worker(f: impl FnOnce() -> () + Send + 'static) { | ||||
| std::thread::spawn(f); | ||||
| /// Start the offchain workers after given block. | ||||
| #[must_use] | ||||
| pub fn on_block_imported<A>( | ||||
| &self, | ||||
| number: &<Block::Header as traits::Header>::Number, | ||||
| pool: &Arc<Pool<A>>, | ||||
| network_state: Arc<dyn NetworkStateInfo + Send + Sync>, | ||||
| is_validator: bool, | ||||
| ) -> impl Future<Output = ()> | ||||
| where | ||||
| A: ChainApi<Block = Block> + 'static, | ||||
| { | ||||
| let runtime = self.client.runtime_api(); | ||||
| let at = BlockId::number(*number); | ||||
| let has_api = runtime.has_api::<dyn OffchainWorkerApi<Block>>(&at); | ||||
| debug!("Checking offchain workers at {:?}: {:?}", at, has_api); | ||||
|
|
||||
| if has_api.unwrap_or(false) { | ||||
| let (api, runner) = api::AsyncApi::new( | ||||
| pool.clone(), | ||||
| self.db.clone(), | ||||
| at.clone(), | ||||
| network_state.clone(), | ||||
| is_validator, | ||||
| ); | ||||
| debug!("Spawning offchain workers at {:?}", at); | ||||
| let number = *number; | ||||
| let client = self.client.clone(); | ||||
| self.spawn_worker(move || { | ||||
| let runtime = client.runtime_api(); | ||||
| let api = Box::new(api); | ||||
| debug!("Running offchain workers at {:?}", at); | ||||
| let run = runtime.offchain_worker_with_context( | ||||
| &at, | ||||
| ExecutionContext::OffchainCall(Some((api, offchain::Capabilities::all()))), | ||||
| number, | ||||
| ); | ||||
| if let Err(e) = run { | ||||
| log::error!("Error running offchain workers at {:?}: {:?}", at, e); | ||||
| } | ||||
| }); | ||||
| futures::future::Either::Left(runner.process()) | ||||
| } else { | ||||
| futures::future::Either::Right(futures::future::ready(())) | ||||
| } | ||||
| } | ||||
|
|
||||
| /// Spawns a new offchain worker. | ||||
| /// | ||||
| /// We spawn offchain workers for each block in a separate thread, | ||||
| /// since they can run for a significant amount of time | ||||
| /// in a blocking fashion and we don't want to block the runtime. | ||||
| /// | ||||
| /// Note that we should avoid that if we switch to future-based runtime in the future, | ||||
| /// alternatively: | ||||
| fn spawn_worker(&self, f: impl FnOnce() -> () + Send + 'static) { | ||||
| self.thread_pool.execute(f); | ||||
| } | ||||
| } | ||||
|
|
||||
| #[cfg(test)] | ||||
| mod tests { | ||||
| use super::*; | ||||
| use network::{Multiaddr, PeerId}; | ||||
|
|
||||
| struct MockNetworkStateInfo(); | ||||
|
|
||||
| impl NetworkStateInfo for MockNetworkStateInfo { | ||||
| fn external_addresses(&self) -> Vec<Multiaddr> { | ||||
| Vec::new() | ||||
| } | ||||
|
|
||||
| fn peer_id(&self) -> PeerId { | ||||
| PeerId::random() | ||||
| } | ||||
| } | ||||
|
|
||||
| #[test] | ||||
| fn should_call_into_runtime_and_produce_extrinsic() { | ||||
| // given | ||||
| let _ = env_logger::try_init(); | ||||
| let client = Arc::new(test_client::new()); | ||||
| let pool = Arc::new(Pool::new(Default::default(), transaction_pool::FullChainApi::new(client.clone()))); | ||||
| let db = client_db::offchain::LocalStorage::new_test(); | ||||
| let network_state = Arc::new(MockNetworkStateInfo()); | ||||
|
|
||||
| // when | ||||
| let offchain = OffchainWorkers::new(client, db); | ||||
| futures::executor::block_on(offchain.on_block_imported(&0u64, &pool, network_state, false)); | ||||
|
|
||||
| // then | ||||
| assert_eq!(pool.status().ready, 1); | ||||
| assert_eq!(pool.ready().next().unwrap().is_propagateable(), false); | ||||
| } | ||||
| use super::*; | ||||
|
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. We use tabs in the codebase, please avoid reformattings. |
||||
| use network::{Multiaddr, PeerId}; | ||||
|
|
||||
| struct MockNetworkStateInfo(); | ||||
|
|
||||
| impl NetworkStateInfo for MockNetworkStateInfo { | ||||
| fn external_addresses(&self) -> Vec<Multiaddr> { | ||||
| Vec::new() | ||||
| } | ||||
|
|
||||
| fn peer_id(&self) -> PeerId { | ||||
| PeerId::random() | ||||
| } | ||||
| } | ||||
|
|
||||
| #[test] | ||||
| fn should_call_into_runtime_and_produce_extrinsic() { | ||||
| // given | ||||
| let _ = env_logger::try_init(); | ||||
| let client = Arc::new(test_client::new()); | ||||
| let pool = Arc::new(Pool::new( | ||||
| Default::default(), | ||||
| transaction_pool::FullChainApi::new(client.clone()), | ||||
| )); | ||||
| let db = client_db::offchain::LocalStorage::new_test(); | ||||
| let network_state = Arc::new(MockNetworkStateInfo()); | ||||
|
|
||||
| // when | ||||
| let offchain = OffchainWorkers::new(client, db); | ||||
| futures::executor::block_on(offchain.on_block_imported(&0u64, &pool, network_state, false)); | ||||
|
|
||||
| // then | ||||
| assert_eq!(pool.status().ready, 1); | ||||
| assert_eq!(pool.ready().next().unwrap().is_propagateable(), false); | ||||
| } | ||||
| } | ||||
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.
Latest version is
1.7, can you update the numbers here?