Skip to content

Commit

Permalink
fix: wg.Done() must be outside RecoverRepanic (#247)
Browse files Browse the repository at this point in the history
This is a small change to the example for correctness.
The concurrency issue is subtle, but here is an attempt to explain it.

Keep in mind that:
- deferred calls happen in reverse order.
- to recover from panics in f, RecoverRepanic must defer a func call
before calling f.

If defer wg.Done() is part of f, the argument passed to RecoverRepanic,
it means it will be called before any deferred functions setup by
RecoverRepanic itself. If all goroutines started in the loop would call
wg.Done() before an event is reported to Sentry, the main goroutine
blocked on wg.Wait() would unblock and reach the end of func main,
terminating the execution of the program, consequently dropping any
other pending work in other goroutines, reporting to Sentry included.

Additionally, reinforce that RecoverRepanic may report only one panic.
Even though already documented in the body of the function, promoting a
note right at the documentation of RecoverRepanic will hopefully raise
awareness for anyone trying to use the example.
  • Loading branch information
rhcarvalho committed Jun 15, 2020
1 parent 1d0b5e6 commit 050fc94
Showing 1 changed file with 21 additions and 9 deletions.
30 changes: 21 additions & 9 deletions example/recover-repanic/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,21 +50,32 @@ func main() {
// Run some worker goroutines to simulate work.
for i := 0; i < nWorkers; i++ {
wg.Add(1)
go RecoverRepanic(func() {
go func() {
// Note that wg.Done() must be outside of RecoverRepanic, otherwise
// it would unblock wg.Wait() before RecoverRepanic is done doing
// its job (reporting panic to Sentry).
defer wg.Done()
// Sleep to simulate some work.
time.Sleep(time.Duration(rand.Intn(1000)) * time.Millisecond)
// Intentionally access an index out of bounds to trigger a runtime
// panic.
fmt.Println(make([]int, 3)[3])
})
RecoverRepanic(func() {
// Sleep to simulate some work.
time.Sleep(time.Duration(rand.Intn(1000)) * time.Millisecond)
// Intentionally access an index out of bounds to trigger a runtime
// panic.
fmt.Println(make([]int, 3)[3])
})
}()
}

wg.Wait()
}

// RecoverRepanic calls f and, in case of a runtime panic, reports the panic to
// Sentry and repanics.
//
// Note that if RecoverRepanic is called from multiple goroutines and they panic
// concurrently, then the repanic initiated from RecoverRepanic, unless handled
// further down the call stack, will cause the program to crash without waiting
// for other goroutines to finish their work. That means that most likely only
// the first panic will be successfully reported to Sentry.
func RecoverRepanic(f func()) {
// Clone the current hub so that modifications of the scope are visible only
// within this function.
Expand Down Expand Up @@ -104,12 +115,13 @@ func RecoverRepanic(f func()) {
// reporting to Sentry.
defer func() {
if x := recover(); x != nil {
// Create an event and enqueue it for reporting.
hub.Recover(x)
// Because the goroutine running this code is going to crash the
// program, call Flush to send the event to Sentry before it is too
// late. Set the timeout to an appropriate value depending on your
// program, what is the maximum time to wait before giving up and
// dropping the event.
// program. The value is the maximum time to wait before giving up
// and dropping the event.
hub.Flush(2 * time.Second)
// Note that if multiple goroutines panic, possibly only the first
// one to call Flush will succeed in sending the event. If you want
Expand Down

0 comments on commit 050fc94

Please sign in to comment.