From cb7383b6b69e6e5890b2cabc9519d9cf7f42c214 Mon Sep 17 00:00:00 2001
From: Pierre Krieger <pierre.krieger1708@gmail.com>
Date: Thu, 1 Aug 2019 09:53:53 +0200
Subject: [PATCH] Documentation and small cleanup in panic-handler (#3249)
* Documentation and small cleanup in panic-handler
* Apply suggestions from code review
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
---
.../core/executor/src/native_executor.rs | 2 +-
substrate/core/panic-handler/src/lib.rs | 71 +++++++++++++++----
substrate/core/state-machine/src/ext.rs | 28 ++++----
3 files changed, 72 insertions(+), 29 deletions(-)
diff --git a/substrate/core/executor/src/native_executor.rs b/substrate/core/executor/src/native_executor.rs
index 10010ef1663..f57c3316683 100644
--- a/substrate/core/executor/src/native_executor.rs
+++ b/substrate/core/executor/src/native_executor.rs
@@ -34,7 +34,7 @@ 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 termination for the native call.
- let _guard = panic_handler::AbortGuard::new(false);
+ let _guard = panic_handler::AbortGuard::force_unwind();
::std::panic::catch_unwind(f).map_err(|_| Error::Runtime)
}
diff --git a/substrate/core/panic-handler/src/lib.rs b/substrate/core/panic-handler/src/lib.rs
index b2fd7238e0d..287ff72a6aa 100644
--- a/substrate/core/panic-handler/src/lib.rs
+++ b/substrate/core/panic-handler/src/lib.rs
@@ -15,18 +15,31 @@
// along with Substrate. If not, see <http://www.gnu.org/licenses/>.
//! Custom panic hook with bug report link
+//!
+//! This crate provides the [`set`] function, which wraps around [`std::panic::set_hook`] and
+//! sets up a panic hook that prints a backtrace and invites the user to open an issue to the
+//! given URL.
+//!
+//! By default, the panic handler aborts the process by calling [`std::process::exit`]. This can
+//! temporarily be disabled by using an [`AbortGuard`].
use backtrace::Backtrace;
use std::io::{self, Write};
+use std::marker::PhantomData;
use std::panic::{self, PanicInfo};
use std::cell::Cell;
use std::thread;
thread_local! {
- pub static ABORT: Cell<bool> = Cell::new(true);
+ static ABORT: Cell<bool> = Cell::new(true);
}
-/// Set the panic hook
+/// Set the panic hook.
+///
+/// Calls [`std::panic::set_hook`] to set up the panic hook.
+///
+/// The `bug_url` parameter is an invitation for users to visit that URL to submit a bug report
+/// in the case where a panic happens.
pub fn set(bug_url: &'static str) {
panic::set_hook(Box::new(move |c| panic_hook(c, bug_url)));
}
@@ -39,7 +52,7 @@ This is a bug. Please report it at:
")}
/// Set aborting flag. Returns previous value of the flag.
-pub fn set_abort(enabled: bool) -> bool {
+fn set_abort(enabled: bool) -> bool {
ABORT.with(|flag| {
let prev = flag.get();
flag.set(enabled);
@@ -47,22 +60,47 @@ pub fn set_abort(enabled: bool) -> bool {
})
}
-/// Abort flag guard. Sets abort on construction and reverts to previous setting when dropped.
-pub struct AbortGuard(bool);
+/// RAII guard for whether panics in the current thread should unwind or abort.
+///
+/// Sets a thread-local abort flag on construction and reverts to the previous setting when dropped.
+/// Does not implement `Send` on purpose.
+///
+/// > **Note**: Because we restore the previous value when dropped, you are encouraged to leave
+/// > the `AbortGuard` on the stack and let it destroy itself naturally.
+pub struct AbortGuard {
+ /// Value that was in `ABORT` before we created this guard.
+ previous_val: bool,
+ /// Marker so that `AbortGuard` doesn't implement `Send`.
+ _not_send: PhantomData<std::rc::Rc<()>>
+}
impl AbortGuard {
- /// Create a new guard and set abort flag to specified value.
- pub fn new(enable: bool) -> AbortGuard {
- AbortGuard(set_abort(enable))
+ /// Create a new guard. While the guard is alive, panics that happen in the current thread will
+ /// unwind the stack (unless another guard is created afterwards).
+ pub fn force_unwind() -> AbortGuard {
+ AbortGuard {
+ previous_val: set_abort(false),
+ _not_send: PhantomData
+ }
+ }
+
+ /// Create a new guard. While the guard is alive, panics that happen in the current thread will
+ /// abort the process (unless another guard is created afterwards).
+ pub fn force_abort() -> AbortGuard {
+ AbortGuard {
+ previous_val: set_abort(true),
+ _not_send: PhantomData
+ }
}
}
impl Drop for AbortGuard {
fn drop(&mut self) {
- set_abort(self.0);
+ set_abort(self.previous_val);
}
}
+/// Function being called when a panic happens.
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>");
@@ -102,9 +140,14 @@ fn panic_hook(info: &PanicInfo, report_url: &'static str) {
})
}
-#[test]
-fn does_not_abort() {
- set("test");
- let _guard = AbortGuard::new(false);
- ::std::panic::catch_unwind(|| panic!()).ok();
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn does_not_abort() {
+ set("test");
+ let _guard = AbortGuard::force_unwind();
+ ::std::panic::catch_unwind(|| panic!()).ok();
+ }
}
diff --git a/substrate/core/state-machine/src/ext.rs b/substrate/core/state-machine/src/ext.rs
index f6369159ba4..b910ddcaf6b 100644
--- a/substrate/core/state-machine/src/ext.rs
+++ b/substrate/core/state-machine/src/ext.rs
@@ -173,35 +173,35 @@ where
N: crate::changes_trie::BlockNumber,
{
fn storage(&self, key: &[u8]) -> Option<Vec<u8>> {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
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);
+ let _guard = panic_handler::AbortGuard::force_abort();
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 original_storage(&self, key: &[u8]) -> Option<Vec<u8>> {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
self.backend.storage(key).expect(EXT_NOT_ALLOWED_TO_FAIL)
}
fn original_storage_hash(&self, key: &[u8]) -> Option<H::Out> {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
self.backend.storage_hash(key).expect(EXT_NOT_ALLOWED_TO_FAIL)
}
fn child_storage(&self, storage_key: ChildStorageKey<H>, key: &[u8]) -> Option<Vec<u8>> {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
self.overlay.child_storage(storage_key.as_ref(), key).map(|x| x.map(|x| x.to_vec())).unwrap_or_else(||
self.backend.child_storage(storage_key.as_ref(), key).expect(EXT_NOT_ALLOWED_TO_FAIL))
}
fn exists_storage(&self, key: &[u8]) -> bool {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
match self.overlay.storage(key) {
Some(x) => x.is_some(),
_ => self.backend.exists_storage(key).expect(EXT_NOT_ALLOWED_TO_FAIL),
@@ -209,7 +209,7 @@ where
}
fn exists_child_storage(&self, storage_key: ChildStorageKey<H>, key: &[u8]) -> bool {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
match self.overlay.child_storage(storage_key.as_ref(), key) {
Some(x) => x.is_some(),
@@ -218,7 +218,7 @@ where
}
fn place_storage(&mut self, key: Vec<u8>, value: Option<Vec<u8>>) {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
if is_child_storage_key(&key) {
warn!(target: "trie", "Refuse to directly set child storage key");
return;
@@ -229,14 +229,14 @@ where
}
fn place_child_storage(&mut self, storage_key: ChildStorageKey<H>, key: Vec<u8>, value: Option<Vec<u8>>) {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
self.mark_dirty();
self.overlay.set_child_storage(storage_key.into_owned(), key, value);
}
fn kill_child_storage(&mut self, storage_key: ChildStorageKey<H>) {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
self.mark_dirty();
self.overlay.clear_child_storage(storage_key.as_ref());
@@ -246,7 +246,7 @@ where
}
fn clear_prefix(&mut self, prefix: &[u8]) {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
if is_child_storage_key(prefix) {
warn!(target: "trie", "Refuse to directly clear prefix that is part of child storage key");
return;
@@ -264,7 +264,7 @@ where
}
fn storage_root(&mut self) -> H::Out {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
if let Some((_, ref root)) = self.storage_transaction {
return root.clone();
}
@@ -292,7 +292,7 @@ where
}
fn child_storage_root(&mut self, storage_key: ChildStorageKey<H>) -> Vec<u8> {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
if self.storage_transaction.is_some() {
self
.storage(storage_key.as_ref())
@@ -319,7 +319,7 @@ where
}
fn storage_changes_root(&mut self, parent_hash: H::Out) -> Result<Option<H::Out>, ()> {
- let _guard = panic_handler::AbortGuard::new(true);
+ let _guard = panic_handler::AbortGuard::force_abort();
self.changes_trie_transaction = build_changes_trie::<_, T, H, N>(
self.backend,
self.changes_trie_storage.clone(),
--
GitLab