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.

Sign up for GitHub

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

Jump to bottom

Support app shutdown with exit codes #808

Closed
wants to merge 10 commits into from
Closed

Support app shutdown with exit codes #808

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f0230d9
Add exitcodeOption to shutdowner
manjari25 Nov 10, 2021
e5a682a
Update app.Run() to look for an exit code if one exists
manjari25 Nov 10, 2021
c482a9a
Add String() implementation for exitCodeOption
manjari25 Nov 10, 2021
85ed3ac
Add test for shutdown with exit code when app is started with app.Run()
manjari25 Nov 10, 2021
b3f67f4
Remove comment
manjari25 Nov 10, 2021
afe8f0e
Add DoneWithCode() for use when an app does not use app.Run()
manjari25 Nov 11, 2021
4b32dc6
Add test for shutdown with DoneWithCode()
manjari25 Nov 11, 2021
ade54ab
Reword comment
manjari25 Nov 11, 2021
978250b
Remove redundant comment
manjari25 Nov 11, 2021
5d714d6
Move exit code computation to app.run()
manjari25 Nov 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app.go
View file
Open in desktop
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
16 changes: 16 additions & 0 deletions shutdown.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@ 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.


// 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 +61,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