Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recover() does not behave as expected #1656

Closed
deelawn opened this issue Feb 13, 2024 · 0 comments · Fixed by #1672
Closed

recover() does not behave as expected #1656

deelawn opened this issue Feb 13, 2024 · 0 comments · Fixed by #1672
Assignees
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@deelawn
Copy link
Contributor

deelawn commented Feb 13, 2024

In go, calling recover() only works inside a defer to handle panics that originated within the current call frame. From the definition here:

If recover is called outside the deferred function it will not stop a panicking sequence

This is not the case in gno; calling recover() in gno will recover any panic that has happened inside or outside of the current call frame. Take this code example:

package main

func doSomething() {
	defer func() {
		doSomethingElse()
		if r := recover(); r != nil {
			panic("do something panic")
		}
	}()
	panic("")
}

func doSomethingElse() {
	if r := recover(); r != nil {
		panic("do something else panic")
	}
}

func main() {
	doSomething()
}

The output should be a panic with the message do something panic. Instead we get the message do something else panic.

I think perhaps the path to solving this is to save panics in the last call frame's struct rather than in the machine's struct.

Note that fixing this will render the testing standard library's Recover method useless.

@deelawn deelawn added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related labels Feb 13, 2024
@deelawn deelawn added this to the 🚀 main.gno.land (required) milestone Feb 13, 2024
@deelawn deelawn self-assigned this Feb 13, 2024
@deelawn deelawn linked a pull request Feb 21, 2024 that will close this issue
7 tasks
deelawn added a commit that referenced this issue Mar 30, 2024
<!-- please provide a detailed description of the changes made in this
pull request. -->
## High level overview
Addresses #1656. This is meant to bring achieve parity between gno and
go's `recover` functionality. The new tests help to ensure this.

BREAKING CHANGE: this will break any realm code that has relies on the
incorrect `recover` implementation

### Changes
For the convenience of the reviewer, here are the categories of changes
that have been made:
- the `recover` function has been modified to determine whether a panic
can be recovered; this is based on both the "level" of nested defer
statements and the frame from which the panic was initiated
- `Frame`s are now stored as pointers to avoid copying frame values as
they are tracked by the `Exception`s that have been thrown
- a new `Exception` type has been defined to encapsulate both the panic
string and the frame in which it was initiated
- the machine has two new "scope" member variables for tracking levels
of `panic` and `defer`
- added safety mechanisms to retrieving frames because we may not always
want it to panic when trying to retrieve a call frame at an index that
exceeds the stack depth
- the gno standard library's `testing` package's `Recover` function no
longer works as intended; the tests have been modified to achieve the
intended behavior
- various new file tests have been added to ensure `recover` behaves as
expected in certain edge cases

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Development

Successfully merging a pull request may close this issue.

2 participants