Add Shutdowner to allow shutdown from a module by SyrieBianco · Pull Request #660 · uber-go/fx

SyrieBianco · 2018-11-29T00:40:56Z

This adds a Shutdowner interface with a Shutdown method, providing it to all
Fx applications. This provides the ability to signal app shutdown from any Fx
module.

Note that a channel of signal receivers is used because os/signal.Notify,
and incidentally fx.App.Done() may be called multiple times, so we need
to notify all channels on shutdown.

Summary of Changes:

Add dones, a slice of done channels, and mu, a mutex, to the App object
in order to maintain references to all done channels created via the Done
method, and to be able to concurrency-safely read from and write to the
dones.
Change the Done method to record created channels on the app as well as
returning a read-only channel.
Create a Shutdowner interface with Shutdown() method by creating closure
upon construction that has access to the app object's dones and mu, and
holding a reference to it.
During the the construction of the App, instantiate the dones field and
provide the Shutdowner to the container.

Points of consideration:

What test functionality do we want to make available to users through
fxtest?
What ShutdownOptions should be added to the shutdown function and
available to modules? Should any shutdown-related Options be surfaced to
the end user, such as white-listing which modules are allowed to call
Shutdown()?

Resolves #615.

CLAassistant · 2018-11-29T00:41:03Z

All committers have signed the CLA.

codecov · 2018-11-29T16:12:50Z

Codecov Report

Merging #660 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
+ Coverage    95.8%   96.01%   +0.21%     
==========================================
  Files           9       10       +1     
  Lines         405      427      +22     
==========================================
+ Hits          388      410      +22     
  Misses         12       12              
  Partials        5        5

Impacted Files	Coverage Δ
app.go	`96.77% <100%> (+0.07%)`	⬆️
shutdown.go	`100% <100%> (ø)`

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 225986d...338afcb. Read the comment docs.

amckinney

Nice! Looks good overall. Some minor suggestions and points for discussion.

app.go

shutdown.go

amckinney · 2018-11-30T23:56:50Z

shutdown.go

+		}
+
+		if unsent != 0 {
+			err := fmt.Errorf("failed to communicate with %v out of %v channels", unsent, len(app.dones))


I'm curious about this case. Should we consider this case an error with respect to a call to Shutdown? This means that all of the application's Done channels have received the os.Signal and we are about to start shutting down, so I'm not sure how we would expect our users to interpret the non-nil result from Shutdown. But, it is a special case in that we failed to send the given os.Signal to at least one of the Done channels.

This isn't necessarily a bad idea though. It makes sense in a "I failed to send the Shutdown signal since it's already sent". I guess the better question is: how do we intend users to use this information, if at all?

To me, this is a lot like the error you get back from closing a file - usually, there's nothing you can do about it, so you log it but don't need to take any special programmatic action. We should still be returning it, though, because this probably indicates that something is deadlocked or otherwise unhealthy, and that information should end up in our exception tracking system.

More generally, we need an error return value here to make options sane later. Many options will take user input, and we'll need a way to indicate to the user that their input was invalid.

shutdown_test.go

shutdown.go

app.go

shutdown.go

akshayjshah · 2018-12-03T19:49:46Z

shutdown.go

+		}
+
+		if unsent != 0 {
+			err := fmt.Errorf("failed to communicate with %v out of %v channels", unsent, len(app.dones))


To me, this is a lot like the error you get back from closing a file - usually, there's nothing you can do about it, so you log it but don't need to take any special programmatic action. We should still be returning it, though, because this probably indicates that something is deadlocked or otherwise unhealthy, and that information should end up in our exception tracking system.

More generally, we need an error return value here to make options sane later. Many options will take user input, and we'll need a way to indicate to the user that their input was invalid.

shutdown_test.go

akshayjshah · 2018-12-04T06:03:33Z

app_test.go

+			found = true
+		}))
+		defer app.RequireStart().RequireStop()
+		assert.True(t, found)


Do we need the extra assertion on found? The rest of the unit tests are already asserting that invoked functions are always run.

So I patterned this test off of the "ProvidesLifecycle" line for line. I don't think that the assertion is necessary, but I figured that the tests to confirm whether or not a type is provided to the container should be the same in content. lmk if you think I should remove the found assertion from both tests or leave it.

shutdown_test.go

akshayjshah · 2018-12-04T06:14:29Z

Overall, I like the way this came out - nice work!

I think we may want to expand the Shutdowner API a bit, though. In production, we'll want to know which module/package/whatever called Shutdown, otherwise it's going to be a complete mystery why applications are randomly shutting down. I'd prefer not to rely on each user of Shutdown doing the right thing and logging a good message; instead, I'd like the Shutdowner itself to collect structured logs and metrics about this.

Unfortunately, this will take a bit of work - the logger (or whatever other struct we're using to track this info) needs to come from inside the container, so it's not available at the Fx layer. Instead, I think we'll want applications to be able to register callbacks that execute just before we actually shut down the app. (Something like OnShutdown(func(*ShutdownInfo)).) We don't need to actually implement that functionality in this PR, but we do need to add a method that enables callback registration to the Shutdowner interface.

app.go

shutdown.go

shutdown_test.go

…struct; 2) remove defer statement; 3) don't instantiate blank dones channel; 4) provide the shutdowner without anonymous function

shutdown_test.go

akshayjshah

Looks great - thanks for going along with the many rounds of review on this. Trust you to add punctuation to comments without further review :)

akshayjshah · 2019-01-16T00:43:23Z

app.go

 // development, users can send the application SIGTERM by pressing Ctrl-C in
 // the same terminal as the running process.
+// Alternatively, a signal can be broadcast to all done channels manually by
+// using the Shutdown functionality (see the Shutdowner documentation for details)


nit: please add a period, exclamation point, or terminal punctuation of your choice at the end of this sentence.

Rather than noting that we're providing the Shutdowner to the container, I changed it to mention that we provided the ability to shut the application down from inside the container since providing it to the container is implicit at that point.

SyrieBianco force-pushed the syrie/trigger_app_shutdown branch from 98dd0d9 to 54b5cdf Compare November 29, 2018 16:12

SyrieBianco force-pushed the syrie/trigger_app_shutdown branch 2 times, most recently from 7eee94e to 276b70a Compare November 29, 2018 19:18

SyrieBianco changed the title ~~[WIP] Add a Shutdowner interface to allow manual shutdown from a module~~ [WIP] Add and provide Shutdowner to allow manual shutdown from a module Nov 29, 2018

SyrieBianco force-pushed the syrie/trigger_app_shutdown branch 3 times, most recently from 9f1b41b to 7e1e34e Compare November 30, 2018 01:20

SyrieBianco changed the title ~~[WIP] Add and provide Shutdowner to allow manual shutdown from a module~~ Add and provide Shutdowner to allow manual shutdown from a module Nov 30, 2018

SyrieBianco force-pushed the syrie/trigger_app_shutdown branch from 7e1e34e to 18b208c Compare November 30, 2018 23:43

amckinney reviewed Dec 1, 2018

View reviewed changes

akshayjshah reviewed Dec 4, 2018

View reviewed changes

abhinav reviewed Dec 4, 2018

View reviewed changes

SyrieBianco force-pushed the syrie/trigger_app_shutdown branch 5 times, most recently from 523c3b6 to 13e7abd Compare January 14, 2019 16:37

abhinav reviewed Jan 15, 2019

View reviewed changes

shutdown.go Outdated Show resolved Hide resolved

shutdown_test.go Outdated Show resolved Hide resolved

shutdown_test.go Outdated Show resolved Hide resolved

SyrieBianco added 5 commits January 14, 2019 16:49

Add a Shutdowner interface to allow manual shutdown from a module

2c5fbc4

Address comments on messaging (error messages, comments, etc)

8865173

Reorganize: 1) Reorder shutdown.go; 2) group done and donesMu in app …

9655851

…struct; 2) remove defer statement; 3) don't instantiate blank dones channel; 4) provide the shutdowner without anonymous function

Refactor to be a method on the app

88796c2

Refactor shutdown tests

f96250a

SyrieBianco force-pushed the syrie/trigger_app_shutdown branch from 2b273a6 to f96250a Compare January 15, 2019 00:49

abhinav reviewed Jan 15, 2019

View reviewed changes

shutdown_test.go Outdated Show resolved Hide resolved

Refine test file, return fmt.Errorf directly

828bd22

SyrieBianco and others added 3 commits January 14, 2019 17:54

Simplify 'Provides...' tests

c780022

nit: var group and single Populate call

1144da2

start before shutdown

63187b2

abhinav approved these changes Jan 15, 2019

View reviewed changes

Update Changelog

0db680b

akshayjshah approved these changes Jan 16, 2019

View reviewed changes

abhinav added 2 commits January 15, 2019 22:12

docs/Done: Add period

8cd6131

CHANGELOG: Reduce implementation notes

338afcb

Rather than noting that we're providing the Shutdowner to the container, I changed it to mention that we provided the ability to shut the application down from inside the container since providing it to the container is implicit at that point.

abhinav changed the title ~~Add and provide Shutdowner to allow manual shutdown from a module~~ Add Shutdowner to allow shutdown from a module Jan 16, 2019

abhinav merged commit 5f6023c into uber-go:master Jan 16, 2019

Conversation

SyrieBianco commented Nov 29, 2018 • edited by abhinav Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amckinney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amckinney Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

akshayjshah Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshayjshah Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akshayjshah Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

SyrieBianco Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akshayjshah commented Dec 4, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshayjshah left a comment

Choose a reason for hiding this comment

Uh oh!

akshayjshah Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

SyrieBianco commented Nov 29, 2018 •

edited by abhinav

Loading

CLAassistant commented Nov 29, 2018 •

edited

Loading

codecov bot commented Nov 29, 2018 •

edited

Loading