Skip to content
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

Closed
wants to merge 10 commits into from
26 changes: 25 additions & 1 deletion app.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -665,7 +666,11 @@ func (app *App) Run() {
// Historically, we do not os.Exit(0) even though most applications
// cede control to Fx with they call app.Run. To avoid a breaking
// change, never os.Exit for success.
if code := app.run(app.Done()); code != 0 {
code := app.run(app.Done())
if app.exitCode != 0 {
code = app.exitCode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kinda ugly. Rather than doing this, can you change exitCode's default value to be 1 and change run to return app.exitCode on error cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, will that not rewrite the exitCode set upon app shutdown?

}
if code != 0 {
app.exit(code)
}
}
Expand Down Expand Up @@ -804,6 +809,25 @@ func (app *App) Done() <-chan os.Signal {
return c
}

// Shutdown captures a signal and exit code.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shutdown is a request to shut down the application.

type Shutdown struct {
Signal os.Signal
ExitCode int
Comment on lines +815 to +816
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document these fields.

Signal is the signal that caused the shutdown.
For manual shutdowns with Shutdowner, it is TODO.

(Figure out the value.)

ExitCode specifies the code the program should exit with.
For programs terminated with SIGINT, this is zero.
For programs terminated with Shutdowner, callers may specify the exit code using the WithExitCode function.

}

// 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 an application exit code, if one exists.
manjari25 marked this conversation as resolved.
Show resolved Hide resolved
func (app *App) DoneWithCode() <-chan Shutdown {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.
Expand Down
22 changes: 21 additions & 1 deletion shutdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I missed this before:
ShutdownOption is an option for a single Shutdown invocation. It doesn't need to store state on the application-scoped App object. It should store state either on the shutdowner, or better yet, on a shutdownOptions struct that the functional options turn into.

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))
Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about,

WithExitCode specifies the exit code for an application shutdown. Applications using App.Run will exit the program with this exit code. Applications using App.DoneWithCode will return this exit code in the Shutdown struct.

Defaults to 0.

func WithExitCode(exitCode int) ShutdownOption {
return exitCodeOption(exitCode)
}

type shutdowner struct {
app *App
}
Expand All @@ -49,6 +65,10 @@ 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 {
// Apply the options to shutdowner
manjari25 marked this conversation as resolved.
Show resolved Hide resolved
for _, opt := range opts {
opt.apply(s)
}
return s.app.broadcastSignal(_sigTERM)
}

Expand Down
53 changes: 53 additions & 0 deletions shutdown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect usage per our documentation of Shutdowner:

// In practice this means Shutdowner.Shutdown should not be called from an
// fx.Invoke, but from a fx.Lifecycle.OnStart hook.

Previously, this just wouldn't work.
That has likely changed since #805 so we can update the documentation,
but all usages of Shutdowner up until this point are inside a lifecycle hook (on or after App.Start),
so we need to test for that too.

)

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) {
Expand Down