This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Adds host function for handle panicking #7954
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This pr adds an extra host function that should be called by the wasm instance when its panicking. Currently we only log such a panic with an error severity. In the future we would call this host function and have the advantage that the panic message would be returned as error, instead of just some generic "panicked" message. This pr only adds the host function and the client side implementation. Some future pr would switch over the runtime to use this functionality.
bkchr
added
A0-please_review
dvdplm
reviewed
Jan 22, 2021
| @@ -46,6 +46,7 @@ struct FunctionExecutor<'a> { | |||
| host_functions: &'a [&'static dyn Function], | |||
| allow_missing_func_imports: bool, | |||
| missing_functions: &'a [String], | |||
| instance_panicked: Option<Error>, | |||
There was a problem hiding this comment.
Could be just panicked maybe?
| /// Should be called when the wasm instance is panicking. | ||
| /// | ||
| /// The given `message` should correspond to the reason the instance panicked. | ||
| fn panicking(&mut self, message: &str) { |
There was a problem hiding this comment.
Maybe just panicked for consistency?
pepyakin
reviewed
Jan 22, 2021
| trait PanicHandler { | ||
| /// Should be called when the wasm instance is panicking. | ||
| /// | ||
| /// The given `message` should correspond to the reason the instance panicked. |
There was a problem hiding this comment.
Some points that should be IMO addressed in the docs:
- It's not mandatory to call this function when "panic" happens, but if the user does it, then they will get a proper message and not a message some general trap message.
- panicking is a Rust term. I would suggest we use the pharsing: "when the wasm instance entered an unrecoverable state and needs to terminate" or something along these lines.
- there is nothing said what are the semantics if called more than once. The last call wins?
UPD: I also see in the code that if this function was called at least once, then it will return an error, even if there were no traps within the call. That should be either documented as well.
Alternatively, we could just give trapping semantics to this call. I.e. when it's called it stashes the message and then traps.
UPD2: I see that wasmi doesn't have the same behavior. If panicked was called but no trap happened we do return with success.
gnunicorn
added
A3-needsresolving
A5-stale
|
Anyone working on this? |
bkchr
removed
A3-needsresolving
A5-stale
This was referenced Jan 24, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pr adds an extra host function that should be called by the wasm
instance when its panicking. Currently we only log such a panic with an
error severity. In the future we would call this host function and
have the advantage that the panic message would be returned as error,
instead of just some generic "panicked" message.
This pr only adds the host function and the client side implementation.
Some future pr would switch over the runtime to use this functionality.