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

Opt-In Panic Recovery #364

Merged
merged 4 commits into from
Dec 13, 2022
Merged

Conversation

JacobOaks
Copy link
Contributor

@JacobOaks JacobOaks commented Dec 8, 2022

Currently, when a panic occurs in provided/invoked/decorated functions, the message users see can be very unwieldy, especially with complicated dependency graphs.

This change allows us to construct our container with a new Option called RecoverFromPanics():

c := dig.New(dig.RecoverFromPanics())

When this is done, any panics that occur in provided/invoked/decorated functions will be recovered from, and wrapped in a new error Type, PanicError, containing the relevant function and the panic value. This PanicError will then be dealt with as a normal error when propagated back us, meaning panics would be just as readable as normal errors when dealing with long complicated call chains.

When opted in, we can distinguish panics from dig errors and non-dig errors when calling c.Invoke like so:

if err := c.Invoke(myFunc); err != nil {
    rootCause := dig.RootCause(err)
    var pe dig.PanicError
    var de dig.Error
    if errors.As(rootCause, &pe) {
        // This was originally a panic, look at pe.Panic
    } else if errors.As(rootCause, &de) {
        // This is a Dig error
    } else {
        // This is just an error from my functions
    }
}

Or, if we just want to distinguish panics from errors we can simply check errors.As(err, &pe).

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #364 (eca90af) into master (cbba855) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head eca90af differs from pull request most recent head 1aa3b92. Consider uploading reports for the commit 1aa3b92 to get more accurate results

@@            Coverage Diff             @@
##           master     #364      +/-   ##
==========================================
+ Coverage   98.30%   98.33%   +0.03%     
==========================================
  Files          21       21              
  Lines        1414     1441      +27     
==========================================
+ Hits         1390     1417      +27     
  Misses         15       15              
  Partials        9        9              
Impacted Files Coverage Δ
constructor.go 97.22% <100.00%> (+0.25%) ⬆️
container.go 100.00% <100.00%> (ø)
decorate.go 100.00% <100.00%> (ø)
error.go 100.00% <100.00%> (ø)
invoke.go 100.00% <100.00%> (ø)
scope.go 98.90% <100.00%> (+0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posting on behalf of @r-hang:

Perhaps add some more details on how to use this in the PR description.

@JacobOaks JacobOaks merged commit c29a62e into uber-go:master Dec 13, 2022
sywhang added a commit to uber-go/fx that referenced this pull request Dec 22, 2022
Since uber-go/dig#364 was merged into Dig, we
can now built upon it to add the ability to recover from panics in
user-provided code into Fx, by providing a `fx.RecoverFromPanics()`
option into their application. This is disallowed on the module level,
only on the application level.

```go
app := fx.New(
    fx.Provide(func() int {
        panic("terrible sorrow")
    }),
    fx.Invoke(func(a int) {}),
    fx.RecoverFromPanics(), // panic will be set to app.Err now
)

err := app.Err()
var pe dig.PanicError
if errors.As(err, &pe) { // panic happened when we invoke
    // handle panic
}
```

Co-authored-by: Sung Yoon Whang <sungyoon@uber.com>
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants