-
Notifications
You must be signed in to change notification settings - Fork 290
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
Support app shutdown with exit codes #808
Changes from all commits
f0230d9
e5a682a
c482a9a
85ed3ac
b3f67f4
afe8f0e
4b32dc6
ade54ab
978250b
5d714d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -382,6 +382,7 @@ type App struct { | |
donesMu sync.Mutex // guards dones and shutdownSig | ||
dones []chan os.Signal | ||
shutdownSig os.Signal | ||
exitCode int | ||
|
||
osExit func(code int) // os.Exit override; used for testing only | ||
} | ||
|
@@ -674,8 +675,13 @@ func (app *App) run(done <-chan os.Signal) (exitCode int) { | |
startCtx, cancel := context.WithTimeout(context.Background(), app.StartTimeout()) | ||
defer cancel() | ||
|
||
exitCode = 1 | ||
if app.exitCode != 0 { | ||
exitCode = app.exitCode | ||
} | ||
|
||
if err := app.Start(startCtx); err != nil { | ||
return 1 | ||
return exitCode | ||
} | ||
|
||
sig := <-done | ||
|
@@ -685,10 +691,10 @@ func (app *App) run(done <-chan os.Signal) (exitCode int) { | |
defer cancel() | ||
|
||
if err := app.Stop(stopCtx); err != nil { | ||
return 1 | ||
return exitCode | ||
} | ||
|
||
return 0 | ||
return app.exitCode | ||
} | ||
|
||
// Err returns any error encountered during New's initialization. See the | ||
|
@@ -804,6 +810,25 @@ func (app *App) Done() <-chan os.Signal { | |
return c | ||
} | ||
|
||
// Shutdown captures a signal and exit code. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
type Shutdown struct { | ||
Signal os.Signal | ||
ExitCode int | ||
Comment on lines
+815
to
+816
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please document these fields.
(Figure out the value.)
|
||
} | ||
|
||
// DoneWithCode returns a channel of the Shutdown struct to block on after starting | ||
// the application. This behaves exactly as Done() does with the addition of | ||
// capturing the application's exit code, if one is set. | ||
func (app *App) DoneWithCode() <-chan Shutdown { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we find an alternative name for this? DoneWithCode isn't enough because we're returning a whole Shutdown struct that can conceivably contain more information in the future. I can't think of anything obvious yet. Maybe "Wait" or similar? We can discuss this. |
||
c := make(chan Shutdown, 1) | ||
// farm out a goroutine to wait for | ||
// app.Done() channel to be populated | ||
go func() { | ||
c <- Shutdown{Signal: <-app.Done(), ExitCode: app.exitCode} | ||
}() | ||
return c | ||
} | ||
|
||
// StartTimeout returns the configured startup timeout. Apps default to using | ||
// DefaultTimeout, but users can configure this behavior using the | ||
// StartTimeout option. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,11 +34,27 @@ type Shutdowner interface { | |
} | ||
|
||
// ShutdownOption provides a way to configure properties of the shutdown | ||
// process. Currently, no options have been implemented. | ||
// process. | ||
type ShutdownOption interface { | ||
apply(*shutdowner) | ||
} | ||
|
||
type exitCodeOption int | ||
|
||
func (e exitCodeOption) apply(s *shutdowner) { | ||
sywhang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
s.app.exitCode = int(e) | ||
} | ||
Comment on lines
+44
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I missed this before: The standard functional options pattern we have in our style guide uses an internal options struct for the options. For example, type shutdownOptions struct {
ExitCode int
}
type ShutdownOption interface{ apply(*shutdownOptions) }
func WithExitCode(int) ShutdownOption I feel like the functional options should resolve the exit code in the s.Shutdown call, and send the appropriate value into the channel from there. Then in Run, we'll use DoneWithCode and the returned exit code and all the state management for app.exitCode can be dropped. |
||
|
||
func (e exitCodeOption) String() string { | ||
return fmt.Sprintf("s.Shutdown(%v)", int(e)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be fx.WithExitCode. |
||
} | ||
|
||
// WithExitCode allows the user to configure the exitCode upon application | ||
// shutdown. | ||
Comment on lines
+52
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about,
|
||
func WithExitCode(exitCode int) ShutdownOption { | ||
return exitCodeOption(exitCode) | ||
} | ||
|
||
type shutdowner struct { | ||
app *App | ||
} | ||
|
@@ -49,6 +65,9 @@ type shutdowner struct { | |
// In practice this means Shutdowner.Shutdown should not be called from an | ||
// fx.Invoke, but from a fx.Lifecycle.OnStart hook. | ||
func (s *shutdowner) Shutdown(opts ...ShutdownOption) error { | ||
for _, opt := range opts { | ||
opt.apply(s) | ||
} | ||
return s.app.broadcastSignal(_sigTERM) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,59 @@ func TestShutdown(t *testing.T) { | |
assert.NotNil(t, <-done1, "done channel 1 did not receive signal") | ||
assert.NotNil(t, <-done2, "done channel 2 did not receive signal") | ||
}) | ||
|
||
t.Run("shutdown app with non-zero, user-specified exit code with app.Run()", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
var ( | ||
exited bool | ||
exitCode int | ||
) | ||
exit := func(code int) { | ||
exited = true | ||
exitCode = code | ||
} | ||
|
||
var s fx.Shutdowner | ||
expectedExitCode := 2 | ||
shutdown := func(s fx.Shutdowner) error { | ||
return s.Shutdown(fx.WithExitCode(expectedExitCode)) | ||
} | ||
|
||
app := fxtest.New( | ||
t, | ||
fx.Populate(&s), | ||
fx.WithExit(exit), | ||
fx.Invoke(shutdown), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is incorrect usage per our documentation of Shutdowner:
Previously, this just wouldn't work. |
||
) | ||
|
||
app.Run() | ||
|
||
assert.Equal(t, expectedExitCode, exitCode) | ||
assert.True(t, exited) | ||
}) | ||
|
||
t.Run("shutdown app with non-zero, user-specified exit code without app.Run()", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
var s fx.Shutdowner | ||
expectedExitCode := 2 | ||
app := fxtest.New( | ||
t, | ||
fx.Populate(&s), | ||
) | ||
|
||
require.NoError(t, app.Start(context.Background()), "error starting app") | ||
assert.NoError(t, s.Shutdown(fx.WithExitCode(expectedExitCode)), "error in app shutdown") | ||
done1, done2 := app.DoneWithCode(), app.DoneWithCode() | ||
done := app.Done() | ||
defer app.Stop(context.Background()) | ||
result1, result2 := <-done1, <-done2 | ||
|
||
assert.NotNil(t, <-done, "done channel did not receive signal") | ||
assert.Equal(t, expectedExitCode, result1.ExitCode, "mismatched exit code") | ||
assert.Equal(t, expectedExitCode, result2.ExitCode, "mismatched exit code") | ||
}) | ||
} | ||
|
||
func TestDataRace(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't give us what we want for normal use cases.
Per our documentation, Shutdowner should be invoked from a lifecycle hook.
That's run when we call App.Start.
By that point, we'll have already captured the exit code.
We need to use the new DoneWithCode API below after the Start, and use the exit code from that.