Skip to content

Commit

Permalink
Do not run lifecycle hooks with expired context (#875)
Browse files Browse the repository at this point in the history
Currently, the lifecycle hooks are still run even when the context
passed as parameter to Start/Stop calls has expired/DeadlineExceeded.

This changes the fx lifecycle hook execution loop to check for expired
context before executing each hook, and bailing out of the exec loop
upon encountering context expiration.

Refs GO-1026.
  • Loading branch information
sywhang committed Apr 20, 2022
1 parent 7e72104 commit 81f070e
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 0 deletions.
8 changes: 8 additions & 0 deletions internal/lifecycle/lifecycle.go
Expand Up @@ -77,6 +77,11 @@ func (l *Lifecycle) Start(ctx context.Context) error {
l.mu.Unlock()

for _, hook := range l.hooks {
// if ctx has cancelled, bail out of the loop.
if err := ctx.Err(); err != nil {
return err
}

if hook.OnStart != nil {
l.mu.Lock()
l.runningHook = hook
Expand Down Expand Up @@ -131,6 +136,9 @@ func (l *Lifecycle) Stop(ctx context.Context) error {
// Run backward from last successful OnStart.
var errs []error
for ; l.numStarted > 0; l.numStarted-- {
if err := ctx.Err(); err != nil {
return err
}
hook := l.hooks[l.numStarted-1]
if hook.OnStop == nil {
continue
Expand Down
44 changes: 44 additions & 0 deletions internal/lifecycle/lifecycle_test.go
Expand Up @@ -119,6 +119,30 @@ func TestLifecycleStart(t *testing.T) {
assert.Equal(t, 2, starterCount, "expected the first and second starter to execute")
assert.Equal(t, 1, stopperCount, "expected the first stopper to execute since the second starter failed")
})

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

l := New(testLogger(t), fxclock.System)
l.Append(Hook{
OnStart: func(context.Context) error {
assert.Fail(t, "this hook should not run")
return nil
},
OnStop: func(context.Context) error {
assert.Fail(t, "this hook should not run")
return nil
},
})
ctx, cancel := context.WithCancel(context.Background())
cancel()
err := l.Start(ctx)
require.Error(t, err)
// Note: Stop does not return an error here because no hooks
// have been started, so we don't end up any of the corresponding
// stop hooks.
require.NoError(t, l.Stop(ctx))
})
}

func TestLifecycleStop(t *testing.T) {
Expand Down Expand Up @@ -247,6 +271,26 @@ func TestLifecycleStop(t *testing.T) {
assert.Equal(t, err, l.Start(context.Background()))
l.Stop(context.Background())
})

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

l := New(testLogger(t), fxclock.System)
l.Append(Hook{
OnStart: func(context.Context) error {
return nil
},
OnStop: func(context.Context) error {
assert.Fail(t, "this hook should not run")
return nil
},
})
ctx, cancel := context.WithCancel(context.Background())
err := l.Start(ctx)
require.NoError(t, err)
cancel()
require.Error(t, l.Stop(ctx))
})
}

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

0 comments on commit 81f070e

Please sign in to comment.