Support app shutdown with exit codes #808

manjari25 · 2021-11-10T20:33:49Z

We provide a way to shutdown an app via the Shutdowner interface but, we do not provide a way to specify/change the exit code. This commit adds support for shutting down apps with an exit code. Exit code can be controlled by providing a new Option to configure the Shutdowner. This can be done via shutdowner.Shutdown(WithExitCode(...))

For apps that use app.Run(), the exit code is read directly and returned.
For apps that are orchestrated with app.Start() and app.Stop(), a new function app.DoneWithCode() is presented that works similar to app.Done(). The following code illustrats its usage -

app := ...
app.Start(...)
done := app.DoneWithCode()
res := <-done
app.Stop(...)
res.ExitCode // captures the exit code

Refs: GO-981, #763

codecov · 2021-11-10T20:34:46Z

Codecov Report

Merging #808 (5d714d6) into master (cc87b09) will decrease coverage by 0.09%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
- Coverage   99.25%   99.16%   -0.10%     
==========================================
  Files          24       24              
  Lines         941      953      +12     
==========================================
+ Hits          934      945      +11     
- Misses          6        7       +1     
  Partials        1        1

Impacted Files	Coverage Δ
shutdown.go	`94.73% <75.00%> (-5.27%)`	⬇️
app.go	`99.67% <100.00%> (+<0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc87b09...5d714d6. Read the comment docs.

abhinav

RE: testing:
check out the hidden WithExit option (only available in tests).

fx/app_internal_test.go

Line 77 in 14f0965

func WithExit(f func(int)) Option {

usage:
https:/uber-go/fx/blob/master/app_test.go#L842

Using that, you can capture the exit code that an app.Run would have exited with.

shutdown.go

sywhang

Can you modify the PR title/description so that it includes the public API's name (DoneWithCode) in it? It'll make it easier to locate it from commit history later :)

shutdown.go

app.go

sywhang · 2021-11-11T07:01:32Z

app.go

- if code := app.run(app.Done()); code != 0 {
+ code := app.run(app.Done())
+ if app.exitCode != 0 {
+ code = app.exitCode


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?

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

manjari25 · 2021-11-11T08:41:54Z

Can you modify the PR title/description so that it includes the public API's name (DoneWithCode) in it? It'll make it easier to locate it from commit history later :)

Done

abhinav · 2021-11-11T16:34:55Z

shutdown.go

+func (e exitCodeOption) apply(s *shutdowner) {
+ s.app.exitCode = int(e)
+}


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.

abhinav · 2021-11-12T13:18:59Z

shutdown.go

+}
+
+func (e exitCodeOption) String() string {
+ return fmt.Sprintf("s.Shutdown(%v)", int(e))


This should probably be fx.WithExitCode.

abhinav · 2021-11-12T13:20:59Z

shutdown.go

+// WithExitCode allows the user to configure the exitCode upon application
+// shutdown.


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.

abhinav · 2021-11-12T13:23:34Z

app.go

+// 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 {


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.

abhinav · 2021-11-12T13:24:04Z

app.go

@@ -804,6 +810,25 @@ func (app *App) Done() <-chan os.Signal {
 return c
 }

+// Shutdown captures a signal and exit code.


Shutdown is a request to shut down the application.

abhinav · 2021-11-12T13:25:56Z

app.go

+ Signal os.Signal
+ ExitCode int


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.

abhinav · 2021-11-12T13:27:55Z

app.go

@@ -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


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.

abhinav · 2021-11-12T13:30:07Z

shutdown_test.go

+ t,
+ fx.Populate(&s),
+ fx.WithExit(exit),
+ fx.Invoke(shutdown),


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.

sywhang · 2022-12-23T19:48:20Z

This has been completed with #989.

manjari25 added 2 commits November 10, 2021 12:29

Add exitcodeOption to shutdowner

f0230d9

Update app.Run() to look for an exit code if one exists

e5a682a

abhinav reviewed Nov 10, 2021

View reviewed changes

shutdown.go Show resolved Hide resolved

manjari25 added 5 commits November 10, 2021 13:44

Add String() implementation for exitCodeOption

c482a9a

Add test for shutdown with exit code when app is started with app.Run()

85ed3ac

Remove comment

b3f67f4

Add DoneWithCode() for use when an app does not use app.Run()

afe8f0e

Add test for shutdown with DoneWithCode()

4b32dc6

manjari25 requested a review from abhinav November 11, 2021 00:27

manjari25 marked this pull request as ready for review November 11, 2021 01:41

sywhang reviewed Nov 11, 2021

View reviewed changes

manjari25 added 2 commits November 11, 2021 00:24

Reword comment

ade54ab

Remove redundant comment

978250b

manjari25 requested a review from sywhang November 11, 2021 08:42

manjari25 force-pushed the manjari/GO-981 branch from e2a945d to 978250b Compare November 11, 2021 10:21

Move exit code computation to app.run()

5d714d6

abhinav requested changes Nov 12, 2021

View reviewed changes

sywhang closed this Dec 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support app shutdown with exit codes #808

Support app shutdown with exit codes #808

manjari25 commented Nov 10, 2021 •

edited

Loading

codecov bot commented Nov 10, 2021 •

edited

Loading

abhinav left a comment

sywhang left a comment

sywhang Nov 11, 2021

manjari25 Nov 11, 2021

manjari25 commented Nov 11, 2021

abhinav Nov 11, 2021

abhinav Nov 12, 2021

abhinav Nov 12, 2021

abhinav Nov 12, 2021

abhinav Nov 12, 2021

abhinav Nov 12, 2021

abhinav Nov 12, 2021

abhinav Nov 12, 2021

sywhang commented Dec 23, 2022

		// WithExitCode allows the user to configure the exitCode upon application
		// shutdown.

Support app shutdown with exit codes #808

Support app shutdown with exit codes #808

Conversation

manjari25 commented Nov 10, 2021 • edited Loading

codecov bot commented Nov 10, 2021 • edited Loading

Codecov Report

abhinav left a comment

Choose a reason for hiding this comment

sywhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manjari25 commented Nov 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sywhang commented Dec 23, 2022

manjari25 commented Nov 10, 2021 •

edited

Loading

codecov bot commented Nov 10, 2021 •

edited

Loading