Skip to content

Commit

Permalink
Fix a race in lifecycle execution (#1062)
Browse files Browse the repository at this point in the history
In #1061, a fix was made to ensure all Stop hooks whose corresponding
Start hooks ran successfully were run.

This does not fix a case where it's possible for the Start hooks to race
with the Stop hooks. In particular, it's possible for a Start hook to be
in the middle of execution when Fx App bails out of the execution loop
due to Start lifecycle context expiration. If the Stop hook happens to
be running when the Start hook finishes running, the same hook will be
run twice because the l.hooks and l.numStarted can change in the middle
of the Stop hook execution loop.

The fix is quite simple - we need to snapshot this state before going
into the Stop hook execution loop so that the changed state due to the
Start hook that updated the state does not affect the Stop hook
execution loop.

Verified that the attached test fails without the fix made in this PR
with the following error:
```
--- FAIL: TestAppStart (0.00s)
    --- FAIL: TestAppStart/race_test (0.30s)
        app_test.go:1252:
            	Error Trace:	/Users/sungyoon/go/src/github.com/uber-go/fx/app_test.go:1252
            	            				/Users/sungyoon/go/src/github.com/uber-go/fx/lifecycle.go:338
            	            				/Users/sungyoon/go/src/github.com/uber-go/fx/lifecycle.go:300
            	            				/Users/sungyoon/go/src/github.com/uber-go/fx/app.go:700
            	            				/Users/sungyoon/go/src/github.com/uber-go/fx/app.go:781
            	            				/Users/sungyoon/go/src/github.com/uber-go/fx/asm_arm64.s:1172
            	Error:      	expected each hook to run exactly once only
            	Test:       	TestAppStart/race_test
FAIL

```
  • Loading branch information
sywhang committed Mar 28, 2023
1 parent 8b301d4 commit c9134b8
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 2 deletions.
54 changes: 54 additions & 0 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1229,6 +1229,60 @@ func TestAppStart(t *testing.T) {
assert.NotContains(t, err.Error(), "timed out while executing hook OnStart")
})

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

var (
firstStart bool
secondStart bool

firstStop bool
secondStop bool
)
app := New(
Invoke(func(lc Lifecycle) {
lc.Append(Hook{
OnStart: func(context.Context) error {
firstStart = true
time.Sleep(10 * time.Millisecond)
return nil
},
OnStop: func(context.Context) error {
if firstStop {
assert.Fail(t, "expected each hook to run exactly once only")
}
firstStop = true
time.Sleep(100 * time.Millisecond)
return nil
},
})
lc.Append(Hook{
OnStart: func(context.Context) error {
secondStart = true
time.Sleep(100 * time.Millisecond)
return nil
},
OnStop: func(context.Context) error {
secondStop = true
time.Sleep(10 * time.Millisecond)
return nil
},
})
}),
)
startCtx, cancelStart := context.WithTimeout(context.Background(), 100*time.Millisecond)
defer cancelStart()
err := app.Start(startCtx)
require.Error(t, err)
assert.ErrorContains(t, err, "context deadline exceeded")
require.NoError(t, app.Stop(context.Background()))

assert.True(t, firstStart)
assert.True(t, secondStart) // this should eventually run.
assert.True(t, firstStop) // this should eventually run.
assert.False(t, secondStop) // this shouldn't be run since context timed out before second start hook finished running.
})

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

Expand Down
7 changes: 5 additions & 2 deletions internal/lifecycle/lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -280,15 +280,18 @@ func (l *Lifecycle) Stop(ctx context.Context) error {

l.mu.Lock()
l.stopRecords = make(HookRecords, 0, l.numStarted)
// Take a snapshot of hook state to avoid races.
allHooks := l.hooks[:]
numStarted := l.numStarted
l.mu.Unlock()

// Run backward from last successful OnStart.
var errs []error
for ; l.numStarted > 0; l.numStarted-- {
for ; numStarted > 0; numStarted-- {
if err := ctx.Err(); err != nil {
return err
}
hook := l.hooks[l.numStarted-1]
hook := allHooks[numStarted-1]
if hook.OnStop == nil {
continue
}
Expand Down

0 comments on commit c9134b8

Please sign in to comment.