Skip to content

Commit

Permalink
ensure withTimeout doesn't have if runtime.Goexit() is called from wi…
Browse files Browse the repository at this point in the history
…thin the callback. (#890)
  • Loading branch information
rubensayshi committed Jun 10, 2022
1 parent e4e7928 commit b74068d
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 2 deletions.
21 changes: 19 additions & 2 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -765,9 +765,26 @@ type withTimeoutParams struct {
lifecycle *lifecycleWrapper
}

// errHookCallbackExited is returned when a hook callback does not finish executing
var errHookCallbackExited = fmt.Errorf("goroutine exited without returning")

func withTimeout(ctx context.Context, param *withTimeoutParams) error {
c := make(chan error, 1)
go func() { c <- param.callback(ctx) }()
go func() {
// If runtime.Goexit() is called from within the callback
// then nothing is written to the chan.
// However the defer will still be called, so we can write to the chan,
// to avoid hanging until the timeout is reached.
callbackExited := false
defer func() {
if !callbackExited {
c <- errHookCallbackExited
}
}()

c <- param.callback(ctx)
callbackExited = true
}()

var err error

Expand All @@ -782,7 +799,7 @@ func withTimeout(ctx context.Context, param *withTimeoutParams) error {
err = ctx.Err()
}
}
if err != context.DeadlineExceeded {
if err != context.DeadlineExceeded && err != errHookCallbackExited {
return err
}
// On timeout, report running hook's caller and recorded
Expand Down
20 changes: 20 additions & 0 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"log"
"os"
"reflect"
"runtime"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -381,6 +382,7 @@ func TestNewApp(t *testing.T) {
[]string{"Provided", "Provided", "Provided", "Provided", "Decorated", "Decorated", "LoggerInitialized", "Started"},
spy.EventTypes())
})

}

func TestWithLoggerErrorUseDefault(t *testing.T) {
Expand Down Expand Up @@ -1262,6 +1264,24 @@ func TestAppStart(t *testing.T) {
assert.Contains(t, err.Error(), "go.uber.org/fx_test.TestAppStart")
assert.Contains(t, err.Error(), "/app_test.go")
})

t.Run("HookGoroutineExitsErrorMsg", func(t *testing.T) {
t.Parallel()

addHook := func(lc Lifecycle) {
lc.Append(Hook{
OnStart: func(ctx context.Context) error {
runtime.Goexit()
return nil
},
})
}
app := fxtest.New(t,
Invoke(addHook),
)
err := app.Start(context.Background()).Error()
assert.Contains(t, err, "OnStart hook added by go.uber.org/fx_test.TestAppStart.func10.1 failed: goroutine exited without returning")
})
}

func TestAppStop(t *testing.T) {
Expand Down

0 comments on commit b74068d

Please sign in to comment.