Name all the tasks! (#6726)

* Remove any implementation of `Spawn` or `Executor` from our task executors

* Fix compilation

* Rename `SpawnBlockingExecutor`

* Update primitives/core/src/traits.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Fix tests

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

substrate/client/service/src/builder.rs

@@ -24,8 +24,7 @@ use crate::{
 	config::{Configuration, KeystoreConfig, PrometheusConfig, OffchainWorkerConfig},
 };
 use sc_client_api::{
-	light::RemoteBlockchain, ForkBlocks, BadBlocks, CloneableSpawn, UsageProvider,
-	ExecutorProvider,
+	light::RemoteBlockchain, ForkBlocks, BadBlocks, UsageProvider, ExecutorProvider,
 };
 use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnboundedReceiver};
 use sc_chain_spec::get_extension;
@@ -55,7 +54,7 @@ use sc_telemetry::{telemetry, SUBSTRATE_INFO};
 use sp_transaction_pool::MaintainedTransactionPool;
 use prometheus_endpoint::Registry;
 use sc_client_db::{Backend, DatabaseSettings};
-use sp_core::traits::CodeExecutor;
+use sp_core::traits::{CodeExecutor, SpawnNamed};
 use sp_runtime::BuildStorage;
 use sc_client_api::{
 	BlockBackend, BlockchainEvents,
@@ -334,7 +333,7 @@ pub fn new_client<E, Block, RA>(
 	fork_blocks: ForkBlocks<Block>,
 	bad_blocks: BadBlocks<Block>,
 	execution_extensions: ExecutionExtensions<Block>,
-	spawn_handle: Box<dyn CloneableSpawn>,
+	spawn_handle: Box<dyn SpawnNamed>,
 	prometheus_registry: Option<Registry>,
 	config: ClientConfig,
 ) -> Result<(
@@ -750,7 +749,8 @@ fn gen_handler<TBl, TBackend, TExPool, TRpc, TCl>(
 		chain_type: config.chain_spec.chain_type(),
 	};
 
-	let subscriptions = SubscriptionManager::new(Arc::new(spawn_handle));
+	let task_executor = sc_rpc::SubscriptionTaskExecutor::new(spawn_handle);
+	let subscriptions = SubscriptionManager::new(Arc::new(task_executor));
 
 	let (chain, state, child_state) = if let (Some(remote_blockchain), Some(on_demand)) =
 		(remote_blockchain, on_demand) {

substrate/client/service/src/client/call_executor.rs

@@ -27,9 +27,12 @@ use sp_state_machine::{
 };
 use sc_executor::{RuntimeVersion, RuntimeInfo, NativeVersion};
 use sp_externalities::Extensions;
-use sp_core::{NativeOrEncoded, NeverNativeValue, traits::CodeExecutor, offchain::storage::OffchainOverlayedChanges};
+use sp_core::{
+	NativeOrEncoded, NeverNativeValue, traits::{CodeExecutor, SpawnNamed},
+	offchain::storage::OffchainOverlayedChanges,
+};
 use sp_api::{ProofRecorder, InitializeBlock, StorageTransactionCache};
-use sc_client_api::{backend, call_executor::CallExecutor, CloneableSpawn};
+use sc_client_api::{backend, call_executor::CallExecutor};
 use super::client::ClientConfig;
 
 /// Call executor that executes methods locally, querying all required
@@ -37,7 +40,7 @@ use super::client::ClientConfig;
 pub struct LocalCallExecutor<B, E> {
 	backend: Arc<B>,
 	executor: E,
-	spawn_handle: Box<dyn CloneableSpawn>,
+	spawn_handle: Box<dyn SpawnNamed>,
 	client_config: ClientConfig,
 }
 
@@ -46,7 +49,7 @@ impl<B, E> LocalCallExecutor<B, E> {
 	pub fn new(
 		backend: Arc<B>,
 		executor: E,
-		spawn_handle: Box<dyn CloneableSpawn>,
+		spawn_handle: Box<dyn SpawnNamed>,
 		client_config: ClientConfig,
 	) -> Self {
 		LocalCallExecutor {
@@ -242,7 +245,7 @@ where
 		method: &str,
 		call_data: &[u8]
 	) -> Result<(Vec<u8>, StorageProof), sp_blockchain::Error> {
-		sp_state_machine::prove_execution_on_trie_backend::<_, _, NumberFor<Block>, _>(
+		sp_state_machine::prove_execution_on_trie_backend::<_, _, NumberFor<Block>, _, _>(
 			trie_state,
 			overlay,
 			&self.executor,

substrate/client/service/src/client/client.rs

@@ -92,8 +92,8 @@ use rand::Rng;
 
 #[cfg(feature="test-helpers")]
 use {
-	sp_core::traits::CodeExecutor,
-	sc_client_api::{CloneableSpawn, in_mem},
+	sp_core::traits::{CodeExecutor, SpawnNamed},
+	sc_client_api::in_mem,
 	sc_executor::RuntimeInfo,
 	super::call_executor::LocalCallExecutor,
 };
@@ -149,7 +149,7 @@ pub fn new_in_mem<E, Block, S, RA>(
 	genesis_storage: &S,
 	keystore: Option<sp_core::traits::BareCryptoStorePtr>,
 	prometheus_registry: Option<Registry>,
-	spawn_handle: Box<dyn CloneableSpawn>,
+	spawn_handle: Box<dyn SpawnNamed>,
 	config: ClientConfig,
 ) -> sp_blockchain::Result<Client<
 	in_mem::Backend<Block>,
@@ -189,7 +189,7 @@ pub fn new_with_backend<B, E, Block, S, RA>(
 	executor: E,
 	build_genesis_storage: &S,
 	keystore: Option<sp_core::traits::BareCryptoStorePtr>,
-	spawn_handle: Box<dyn CloneableSpawn>,
+	spawn_handle: Box<dyn SpawnNamed>,
 	prometheus_registry: Option<Registry>,
 	config: ClientConfig,
 ) -> sp_blockchain::Result<Client<B, LocalCallExecutor<B, E>, Block, RA>>

substrate/client/service/src/client/light.rs

@@ -21,17 +21,14 @@
 use std::sync::Arc;
 
 use sc_executor::RuntimeInfo;
-use sp_core::traits::CodeExecutor;
+use sp_core::traits::{CodeExecutor, SpawnNamed};
 use sp_runtime::BuildStorage;
 use sp_runtime::traits::{Block as BlockT, HashFor};
 use sp_blockchain::Result as ClientResult;
 use prometheus_endpoint::Registry;
 
-use super::call_executor::LocalCallExecutor;
-use super::client::{Client,ClientConfig};
-use sc_client_api::{
-	light::Storage as BlockchainStorage, CloneableSpawn,
-};
+use super::{call_executor::LocalCallExecutor, client::{Client, ClientConfig}};
+use sc_client_api::light::Storage as BlockchainStorage;
 use sc_light::{Backend, GenesisCallExecutor};
 
 
@@ -40,7 +37,7 @@ pub fn new_light<B, S, RA, E>(
 	backend: Arc<Backend<S, HashFor<B>>>,
 	genesis_storage: &dyn BuildStorage,
 	code_executor: E,
-	spawn_handle: Box<dyn CloneableSpawn>,
+	spawn_handle: Box<dyn SpawnNamed>,
 	prometheus_registry: Option<Registry>,
 ) -> ClientResult<
 		Client<

substrate/client/service/src/lib.rs

@@ -556,7 +556,7 @@ mod tests {
 		// given
 		let (client, longest_chain) = TestClientBuilder::new().build_with_longest_chain();
 		let client = Arc::new(client);
-		let spawner = sp_core::testing::SpawnBlockingExecutor::new();
+		let spawner = sp_core::testing::TaskExecutor::new();
 		let pool = BasicPool::new_full(
 			Default::default(),
 			Arc::new(FullChainApi::new(client.clone(), None)),

substrate/client/service/src/task_manager/mod.rs

@@ -19,8 +19,6 @@ use log::{debug, error};
 use futures::{
 	Future, FutureExt, StreamExt,
 	future::{select, Either, BoxFuture},
-	compat::*,
-	task::{Spawn, FutureObj, SpawnError},
 	sink::SinkExt,
 };
 use prometheus_endpoint::{
@@ -28,7 +26,6 @@ use prometheus_endpoint::{
 	PrometheusError,
 	CounterVec, HistogramOpts, HistogramVec, Opts, Registry, U64
 };
-use sc_client_api::CloneableSpawn;
 use sp_utils::mpsc::{TracingUnboundedSender, TracingUnboundedReceiver, tracing_unbounded};
 use crate::{config::{TaskExecutor, TaskType, JoinFuture}, Error};
 
@@ -132,14 +129,6 @@ impl SpawnTaskHandle {
 	}
 }
 
-impl Spawn for SpawnTaskHandle {
-	fn spawn_obj(&self, future: FutureObj<'static, ()>)
-	-> Result<(), SpawnError> {
-		self.spawn("unnamed", future);
-		Ok(())
-	}
-}
-
 impl sp_core::traits::SpawnNamed for SpawnTaskHandle {
 	fn spawn_blocking(&self, name: &'static str, future: BoxFuture<'static, ()>) {
 		self.spawn_blocking(name, future);
@@ -150,21 +139,6 @@ impl sp_core::traits::SpawnNamed for SpawnTaskHandle {
 	}
 }
 
-impl sc_client_api::CloneableSpawn for SpawnTaskHandle {
-	fn clone(&self) -> Box<dyn CloneableSpawn> {
-		Box::new(Clone::clone(self))
-	}
-}
-
-type Boxed01Future01 = Box<dyn futures01::Future<Item = (), Error = ()> + Send + 'static>;
-
-impl futures01::future::Executor<Boxed01Future01> for SpawnTaskHandle {
-	fn execute(&self, future: Boxed01Future01) -> Result<(), futures01::future::ExecuteError<Boxed01Future01>>{
-		self.spawn("unnamed", future.compat().map(drop));
-		Ok(())
-	}
-}
-
 /// A wrapper over `SpawnTaskHandle` that will notify a receiver whenever any
 /// task spawned through it fails. The service should be on the receiver side
 /// and will shut itself down whenever it receives any message, i.e. an

substrate/client/service/test/src/client/light.rs

@@ -37,9 +37,9 @@ use substrate_test_runtime_client::{
 	runtime::{Hash, Block, Header}, TestClient, ClientBlockImportExt,
 };
 use sp_api::{InitializeBlock, StorageTransactionCache, ProofRecorder, OffchainOverlayedChanges};
-use sp_consensus::{BlockOrigin};
+use sp_consensus::BlockOrigin;
 use sc_executor::{NativeExecutor, WasmExecutionMethod, RuntimeVersion, NativeVersion};
-use sp_core::{H256, tasks::executor as tasks_executor, NativeOrEncoded};
+use sp_core::{H256, NativeOrEncoded, testing::TaskExecutor};
 use sc_client_api::{
 	blockchain::Info, backend::NewBlockState, Backend as ClientBackend, ProofProvider,
 	in_mem::{Backend as InMemBackend, Blockchain as InMemoryBlockchain},
@@ -317,7 +317,7 @@ fn execution_proof_is_generated_and_checked() {
 		// check remote execution proof locally
 		let local_result = check_execution_proof::<_, _, BlakeTwo256>(
 			&local_executor(),
-			tasks_executor(),
+			Box::new(TaskExecutor::new()),
 			&RemoteCallRequest {
 				block: substrate_test_runtime_client::runtime::Hash::default(),
 				header: remote_header,
@@ -345,7 +345,7 @@ fn execution_proof_is_generated_and_checked() {
 		// check remote execution proof locally
 		let execution_result = check_execution_proof_with_make_header::<_, _, BlakeTwo256, _>(
 			&local_executor(),
-			tasks_executor(),
+			Box::new(TaskExecutor::new()),
 			&RemoteCallRequest {
 				block: substrate_test_runtime_client::runtime::Hash::default(),
 				header: remote_header,
@@ -479,7 +479,7 @@ fn prepare_for_read_proof_check() -> (TestChecker, Header, StorageProof, u32) {
 	let local_checker = LightDataChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	(local_checker, remote_block_header, remote_read_proof, heap_pages)
 }
@@ -527,7 +527,7 @@ fn prepare_for_read_child_proof_check() -> (TestChecker, Header, StorageProof, V
 	let local_checker = LightDataChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	(local_checker, remote_block_header, remote_read_proof, child_value)
 }
@@ -558,7 +558,7 @@ fn prepare_for_header_proof_check(insert_cht: bool) -> (TestChecker, Hash, Heade
 	let local_checker = LightDataChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	(local_checker, local_cht_root, remote_block_header, remote_header_proof)
 }
@@ -642,7 +642,7 @@ fn changes_proof_is_generated_and_checked_when_headers_are_not_pruned() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	let local_checker = &local_checker as &dyn FetchChecker<Block>;
 	let max = remote_client.chain_info().best_number;
@@ -717,7 +717,7 @@ fn changes_proof_is_generated_and_checked_when_headers_are_pruned() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(local_storage)),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 
 	// check proof on local client
@@ -752,7 +752,7 @@ fn check_changes_proof_fails_if_proof_is_wrong() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	let local_checker = &local_checker as &dyn FetchChecker<Block>;
 	let max = remote_client.chain_info().best_number;
@@ -840,7 +840,7 @@ fn check_changes_tries_proof_fails_if_proof_is_wrong() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	assert!(local_checker.check_changes_tries_proof(4, &remote_proof.roots,
 	                                                remote_proof.roots_proof.clone()).is_err());
@@ -851,7 +851,7 @@ fn check_changes_tries_proof_fails_if_proof_is_wrong() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(local_storage)),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 	let result = local_checker.check_changes_tries_proof(
 		4, &remote_proof.roots, StorageProof::empty()
@@ -869,7 +869,7 @@ fn check_body_proof_faulty() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 
 	let body_request = RemoteBodyRequest {
@@ -893,7 +893,7 @@ fn check_body_proof_of_same_data_should_succeed() {
 	let local_checker = TestChecker::new(
 		Arc::new(DummyBlockchain::new(DummyStorage::new())),
 		local_executor(),
-		tasks_executor(),
+		Box::new(TaskExecutor::new()),
 	);
 
 	let body_request = RemoteBodyRequest {

substrate/client/service/test/src/client/mod.rs

@@ -40,8 +40,7 @@ use sp_runtime::traits::{
 use substrate_test_runtime::TestAPI;
 use sp_state_machine::backend::Backend as _;
 use sp_api::{ProvideRuntimeApi, OffchainOverlayedChanges};
-use sp_core::tasks::executor as tasks_executor;
-use sp_core::{H256, ChangesTrieConfiguration, blake2_256};
+use sp_core::{H256, ChangesTrieConfiguration, blake2_256, testing::TaskExecutor};
 use std::collections::{HashMap, HashSet};
 use std::sync::Arc;
 use sp_consensus::{
@@ -165,6 +164,7 @@ fn construct_block(
 	let mut offchain_overlay = OffchainOverlayedChanges::default();
 	let backend_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&backend);
 	let runtime_code = backend_runtime_code.runtime_code().expect("Code is part of the backend");
+	let task_executor = Box::new(TaskExecutor::new());
 
 	StateMachine::new(
 		backend,
@@ -176,7 +176,7 @@ fn construct_block(
 		&header.encode(),
 		Default::default(),
 		&runtime_code,
-		tasks_executor(),
+		task_executor.clone() as Box<_>,
 	).execute(
 		ExecutionStrategy::NativeElseWasm,
 	).unwrap();
@@ -192,7 +192,7 @@ fn construct_block(
 			&tx.encode(),
 			Default::default(),
 			&runtime_code,
-			tasks_executor(),
+			task_executor.clone() as Box<_>,
 		).execute(
 			ExecutionStrategy::NativeElseWasm,
 		).unwrap();
@@ -208,7 +208,7 @@ fn construct_block(
 		&[],
 		Default::default(),
 		&runtime_code,
-		tasks_executor(),
+		task_executor.clone() as Box<_>,
 	).execute(
 		ExecutionStrategy::NativeElseWasm,
 	).unwrap();
@@ -262,7 +262,7 @@ fn construct_genesis_should_work_with_native() {
 		&b1data,
 		Default::default(),
 		&runtime_code,
-		tasks_executor(),
+		TaskExecutor::new(),
 	).execute(
 		ExecutionStrategy::NativeElseWasm,
 	).unwrap();
@@ -298,7 +298,7 @@ fn construct_genesis_should_work_with_wasm() {
 		&b1data,
 		Default::default(),
 		&runtime_code,
-		tasks_executor(),
+		TaskExecutor::new(),
 	).execute(
 		ExecutionStrategy::AlwaysWasm,
 	).unwrap();
@@ -334,7 +334,7 @@ fn construct_genesis_with_bad_transaction_should_panic() {
 		&b1data,
 		Default::default(),
 		&runtime_code,
-		tasks_executor(),
+		TaskExecutor::new(),
 	).execute(
 		ExecutionStrategy::NativeElseWasm,
 	);
@@ -1743,7 +1743,7 @@ fn cleans_up_closed_notification_sinks_on_block_import() {
 			&substrate_test_runtime_client::GenesisParameters::default().genesis_storage(),
 			None,
 			None,
-			sp_core::tasks::executor(),
+			Box::new(TaskExecutor::new()),
 			Default::default(),
 		)
 			.unwrap();