-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
2b139a4
to
de42caf
Compare
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.
Looks good, but requires an pr to the node-template
.
")} | ||
|
||
/// Set aborting flag. Returns previous value of the flag. | ||
pub fn set_abort(enabled: bool) -> bool { |
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 think this function should not be public?
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.
panics in the runtime should not abort, but panics in client code, including exernalities, should.
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.
@bkchr I'd leave it public in case someone needs to set and forget. The crate API aims to be universal.
@@ -181,35 +181,41 @@ where | |||
H::Out: Ord + HeapSizeOf, | |||
{ | |||
fn storage(&self, key: &[u8]) -> Option<Vec<u8>> { | |||
let _guard = panic_handler::AbortGuard::new(true); |
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.
Why do we need to abort here?
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.
As @rphmeier answered in the wrong comment, client code panic should not lead to runtime error and potential consensus issues. Also panic behaviour between native and wasm runtimes needs to be consistent.
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.
Okay. :)
* Link substrate issue tracker in panic * Replace allocator with freeing-bump allocator * Revert me: Panic on double allocate/free * Revert me: Add shallow benchmark for a first impression * Revert "Revert me: Add shallow benchmark for a first impression" This reverts commit 5f0d4df. * Revert "Revert me: Panic on double allocate/free" This reverts commit a114df7. * Rename heap to FreeingBumpHeapAllocator * Rename heap.rs to allocator.rs * Use sandbox heap * Move functions * Move variables into constructor * Revert "Move variables into constructor" This reverts commit f46fa0d. * Remove unnecessary casts * Add comment for new parameter * Improve typing * Move variables into constructor * Avoid dynamic allocation * Remove unused variables * Revert "Link substrate issue tracker in panic" This reverts commit 32dfa1d. In the meantime this was fixed in #1667. * Improve naming * Only assert in debug mode * Remove dynamic allocation
* Link substrate issue tracker in panic * Replace allocator with freeing-bump allocator * Revert me: Panic on double allocate/free * Revert me: Add shallow benchmark for a first impression * Revert "Revert me: Add shallow benchmark for a first impression" This reverts commit 5f0d4df. * Revert "Revert me: Panic on double allocate/free" This reverts commit a114df7. * Rename heap to FreeingBumpHeapAllocator * Rename heap.rs to allocator.rs * Use sandbox heap * Move functions * Move variables into constructor * Revert "Move variables into constructor" This reverts commit f46fa0d. * Remove unnecessary casts * Add comment for new parameter * Improve typing * Move variables into constructor * Avoid dynamic allocation * Remove unused variables * Revert "Link substrate issue tracker in panic" This reverts commit 32dfa1d. In the meantime this was fixed in paritytech#1667. * Improve naming * Only assert in debug mode * Remove dynamic allocation
Closes #1470