Fixed panic handling (#1667)

substrate/Cargo.lock

@@ -3433,7 +3433,6 @@ dependencies = [
  "ansi_term 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "app_dirs 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "atty 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)",
- "backtrace 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)",
  "clap 2.32.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "env_logger 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "error-chain 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3449,6 +3448,7 @@ dependencies = [
  "structopt 0.2.14 (registry+https://github.com/rust-lang/crates.io-index)",
  "substrate-client 0.1.0",
  "substrate-network 0.1.0",
+ "substrate-panic-handler 0.1.0",
  "substrate-primitives 0.1.0",
  "substrate-service 0.3.0",
  "substrate-telemetry 0.3.0",
@@ -3618,6 +3618,7 @@ dependencies = [
  "serde_derive 1.0.85 (registry+https://github.com/rust-lang/crates.io-index)",
  "sr-io 0.1.0",
  "sr-version 0.1.0",
+ "substrate-panic-handler 0.1.0",
  "substrate-primitives 0.1.0",
  "substrate-serializer 0.1.0",
  "substrate-state-machine 0.1.0",
@@ -3749,6 +3750,14 @@ dependencies = [
  "void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
+[[package]]
+name = "substrate-panic-handler"
+version = "0.1.0"
+dependencies = [
+ "backtrace 0.3.13 (registry+https://github.com/rust-lang/crates.io-index)",
+ "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
 [[package]]
 name = "substrate-primitives"
 version = "0.1.0"
@@ -3898,6 +3907,7 @@ dependencies = [
  "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "parity-codec 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "parking_lot 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "substrate-panic-handler 0.1.0",
  "substrate-primitives 0.1.0",
  "substrate-trie 0.4.0",
  "trie-db 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)",

substrate/Cargo.toml

@@ -30,6 +30,7 @@ members = [
 	"core/finality-grandpa/primitives",
 	"core/keyring",
 	"core/network",
+	"core/panic-handler",
 	"core/primitives",
 	"core/rpc",
 	"core/rpc-servers",

substrate/core/cli/Cargo.toml

@@ -7,7 +7,6 @@ edition = "2018"
 
 [dependencies]
 clap = "~2.32"
-backtrace = "0.3"
 env_logger = "0.6"
 error-chain = "0.12"
 log = "0.4"
@@ -23,6 +22,7 @@ futures = "0.1.17"
 fdlimit = "0.1"
 exit-future = "0.1"
 sysinfo = "0.7"
+panic-handler = { package = "substrate-panic-handler", path = "../../core/panic-handler" }
 client = { package = "substrate-client", path = "../../core/client" }
 network = { package = "substrate-network", path = "../../core/network" }
 runtime_primitives = { package = "sr-primitives", path = "../../core/sr-primitives" }

substrate/core/cli/src/lib.rs

@@ -24,7 +24,6 @@ mod traits;
 mod params;
 pub mod error;
 pub mod informant;
-mod panic_hook;
 
 use runtime_primitives::traits::As;
 use service::{
@@ -76,6 +75,8 @@ pub struct VersionInfo {
 	pub description: &'static str,
 	/// Executable file author.
 	pub author: &'static str,
+	/// Support URL.
+	pub support_url: &'static str,
 }
 
 /// Something that can be converted into an exit signal.
@@ -189,7 +190,7 @@ where
 	I: IntoIterator<Item = T>,
 	T: Into<std::ffi::OsString> + Clone,
 {
-	panic_hook::set();
+	panic_handler::set(version.support_url);
 
 	let full_version = service::config::full_version_from_strs(
 		version.version,

substrate/core/executor/Cargo.toml

@@ -13,6 +13,7 @@ trie = { package = "substrate-trie", path = "../trie" }
 serializer = { package = "substrate-serializer", path = "../serializer" }
 state_machine = { package = "substrate-state-machine", path = "../state-machine"  }
 runtime_version = { package = "sr-version", path = "../sr-version" }
+panic-handler = { package = "substrate-panic-handler", path = "../panic-handler" }
 serde = "1.0"
 serde_derive = "1.0"
 wasmi = { version = "0.4.3" }

substrate/core/executor/src/native_executor.rs

@@ -97,11 +97,9 @@ fn fetch_cached_runtime_version<'a, E: Externalities<Blake2Hasher>>(
 fn safe_call<F, U>(f: F) -> Result<U>
 	where F: UnwindSafe + FnOnce() -> U
 {
-	// Substrate uses custom panic hook that terminates process on panic. Disable it for the native call.
+	// Substrate uses custom panic hook that terminates process on panic. Disable termination for the native call.
-	let hook = ::std::panic::take_hook();
+	let _guard = panic_handler::AbortGuard::new(false);
-	let result = ::std::panic::catch_unwind(f).map_err(|_| ErrorKind::Runtime.into());
+	::std::panic::catch_unwind(f).map_err(|_| ErrorKind::Runtime.into())
-	::std::panic::set_hook(hook);
-	result
 }
 
 /// Set up the externalities and safe calling environment to execute calls to a native runtime.

substrate/core/panic-handler/Cargo.toml

@@ -0,0 +1,10 @@
+[package]
+name = "substrate-panic-handler"
+version = "0.1.0"
+authors = ["Parity Technologies <admin@parity.io>"]
+description = "Substrate panic handler."
+edition = "2018"
+
+[dependencies]
+backtrace = "0.3"
+log = "0.4"

substrate/core/panic-handler/src/lib.rs

@@ -19,20 +19,51 @@
 use backtrace::Backtrace;
 use std::io::{self, Write};
 use std::panic::{self, PanicInfo};
+use std::cell::Cell;
 use std::thread;
 
-/// Set the panic hook
+thread_local! {
-pub fn set() {
+	pub static ABORT: Cell<bool> = Cell::new(true);
-	panic::set_hook(Box::new(panic_hook));
 }
 
-static ABOUT_PANIC: &str = "
+/// Set the panic hook
+pub fn set(bug_url: &'static str) {
+	panic::set_hook(Box::new(move |c| panic_hook(c, bug_url)));
+}
+
+macro_rules! ABOUT_PANIC {
+	() => ("
 This is a bug. Please report it at:
 
-    https://github.com/paritytech/polkadot/issues/new
+	{}
-";
+")}
 
-fn panic_hook(info: &PanicInfo) {
+/// Set aborting flag. Returns previous value of the flag.
+pub fn set_abort(enabled: bool) -> bool {
+	ABORT.with(|flag| {
+		let prev = flag.get();
+		flag.set(enabled);
+		prev
+	})
+}
+
+/// Abort flag guard. Sets abort on construction and reverts to previous setting when dropped.
+pub struct AbortGuard(bool);
+
+impl AbortGuard {
+	/// Create a new guard and set abort flag to specified value.
+	pub fn new(enable: bool) -> AbortGuard {
+		AbortGuard(set_abort(enable))
+	}
+}
+
+impl Drop for AbortGuard {
+	fn drop(&mut self) {
+		set_abort(self.0);
+	}
+}
+
+fn panic_hook(info: &PanicInfo, report_url: &'static str) {
 	let location = info.location();
 	let file = location.as_ref().map(|l| l.file()).unwrap_or("<unknown>");
 	let line = location.as_ref().map(|l| l.line()).unwrap_or(0);
@@ -63,7 +94,17 @@ fn panic_hook(info: &PanicInfo) {
 		name, msg, file, line
 	);
 
-	let _ = writeln!(stderr, "{}", ABOUT_PANIC);
+	let _ = writeln!(stderr, ABOUT_PANIC!(), report_url);
-	::std::process::exit(1);
+	ABORT.with(|flag| {
+		if flag.get() {
+			::std::process::exit(1);
+		}
+	})
 }
 
+#[test]
+fn does_not_abort() {
+	set("test");
+	let _guard = AbortGuard::new(false);
+	::std::panic::catch_unwind(|| panic!()).ok();
+}

substrate/core/state-machine/Cargo.toml

@@ -15,4 +15,5 @@ trie-db = "0.9"
 trie-root = "0.9"
 trie = { package = "substrate-trie", path = "../trie" }
 primitives = { package = "substrate-primitives", path = "../primitives" }
+panic-handler = { package = "substrate-panic-handler", path = "../panic-handler" }
 parity-codec = "3.0"

substrate/core/state-machine/src/ext.rs

@@ -181,21 +181,25 @@ where
 	H::Out: Ord + HeapSizeOf,
 {
 	fn storage(&self, key: &[u8]) -> Option<Vec<u8>> {
+		let _guard = panic_handler::AbortGuard::new(true);
 		self.overlay.storage(key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(||
 			self.backend.storage(key).expect(EXT_NOT_ALLOWED_TO_FAIL))
 	}
 
 	fn storage_hash(&self, key: &[u8]) -> Option<H::Out> {
+		let _guard = panic_handler::AbortGuard::new(true);
 		self.overlay.storage(key).map(|x| x.map(|x| H::hash(x))).unwrap_or_else(||
 			self.backend.storage_hash(key).expect(EXT_NOT_ALLOWED_TO_FAIL))
 	}
 
 	fn child_storage(&self, storage_key: &[u8], key: &[u8]) -> Option<Vec<u8>> {
+		let _guard = panic_handler::AbortGuard::new(true);
 		self.overlay.child_storage(storage_key, key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(||
 			self.backend.child_storage(storage_key, key).expect(EXT_NOT_ALLOWED_TO_FAIL))
 	}
 
 	fn exists_storage(&self, key: &[u8]) -> bool {
+		let _guard = panic_handler::AbortGuard::new(true);
 		match self.overlay.storage(key) {
 			Some(x) => x.is_some(),
 			_ => self.backend.exists_storage(key).expect(EXT_NOT_ALLOWED_TO_FAIL),
@@ -203,6 +207,7 @@ where
 	}
 
 	fn exists_child_storage(&self, storage_key: &[u8], key: &[u8]) -> bool {
+		let _guard = panic_handler::AbortGuard::new(true);
 		match self.overlay.child_storage(storage_key, key) {
 			Some(x) => x.is_some(),
 			_ => self.backend.exists_child_storage(storage_key, key).expect(EXT_NOT_ALLOWED_TO_FAIL),
@@ -210,6 +215,7 @@ where
 	}
 
 	fn place_storage(&mut self, key: Vec<u8>, value: Option<Vec<u8>>) {
+		let _guard = panic_handler::AbortGuard::new(true);
 		if is_child_storage_key(&key) {
 			warn!(target: "trie", "Refuse to directly set child storage key");
 			return;
@@ -220,6 +226,7 @@ where
 	}
 
 	fn place_child_storage(&mut self, storage_key: Vec<u8>, key: Vec<u8>, value: Option<Vec<u8>>) -> bool {
+		let _guard = panic_handler::AbortGuard::new(true);
 		if !is_child_storage_key(&storage_key) || !is_child_trie_key_valid::<H>(&storage_key) {
 			return false;
 		}
@@ -231,6 +238,7 @@ where
 	}
 
 	fn kill_child_storage(&mut self, storage_key: &[u8]) {
+		let _guard = panic_handler::AbortGuard::new(true);
 		if !is_child_storage_key(storage_key) || !is_child_trie_key_valid::<H>(storage_key) {
 			return;
 		}
@@ -243,6 +251,7 @@ where
 	}
 
 	fn clear_prefix(&mut self, prefix: &[u8]) {
+		let _guard = panic_handler::AbortGuard::new(true);
 		if is_child_storage_key(prefix) {
 			warn!(target: "trie", "Refuse to directly clear prefix that is part of child storage key");
 			return;
@@ -260,6 +269,7 @@ where
 	}
 
 	fn storage_root(&mut self) -> H::Out {
+		let _guard = panic_handler::AbortGuard::new(true);
 		if let Some((_, ref root)) = self.storage_transaction {
 			return root.clone();
 		}
@@ -283,6 +293,7 @@ where
 	}
 
 	fn child_storage_root(&mut self, storage_key: &[u8]) -> Option<Vec<u8>> {
+		let _guard = panic_handler::AbortGuard::new(true);
 		if !is_child_storage_key(storage_key) || !is_child_trie_key_valid::<H>(storage_key) {
 			return None;
 		}
@@ -295,6 +306,7 @@ where
 	}
 
 	fn storage_changes_root(&mut self, parent: H::Out, parent_num: u64) -> Option<H::Out> {
+		let _guard = panic_handler::AbortGuard::new(true);
 		let root_and_tx = compute_changes_trie_root::<_, T, H>(
 			self.backend,
 			self.changes_trie_storage.clone(),

substrate/node/src/main.rs

@@ -60,6 +60,7 @@ fn run() -> cli::error::Result<()> {
 		executable_name: "substrate",
 		author: "Parity Technologies <admin@parity.io>",
 		description: "Generic substrate node",
+		support_url: "https://github.com/paritytech/substrate/issues/new",
 	};
 	cli::run(::std::env::args(), Exit, version)
 }