-
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
Implements ShutdownCode option and ShutdownSignal os.Signal wrapper #912
Conversation
Codecov Report
@@ Coverage Diff @@
## master #912 +/- ##
==========================================
- Coverage 98.67% 98.52% -0.16%
==========================================
Files 38 32 -6
Lines 1589 1560 -29
==========================================
- Hits 1568 1537 -31
- Misses 15 17 +2
Partials 6 6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
764432a
to
1c55c52
Compare
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.
Great progress. Didn't fully go through the implementation, but I'm not so sure if this is what we want; it's a little awkward. So, what we have now is:
- Call
Done()
to get a channel - Receive from that channel
- Create a ShutdownSignal from that channel
- Check its exit code
An easier thing to do might be exposing a method under App
that returns a different channel than the one returned by App.Done()
.
Say:
app := fx.App(
// .. stuff
fx.Invoke(func(s fx.Shutdowner) {
shutdowner.Shutdown(fx.ShutdownCode(100))
}),
)
exit := <-app.Exit() // name TBD
os.Exit(exit.ExitCode()) // exits with 100
so exit
is a channel that receives ShutdownerSignal
type.
With this, we get:
- Call
Exit()
to get a channel - Receive from that channel
- Check its exit code.
which removes the awkward step of having to create a random type out of something returned from the Done channel.
7dad518
to
b8a589f
Compare
b8a589f
to
e68ea41
Compare
This is a proposed change to #912 by @jasonmills that DRYs up internal state management by unifying `chan os.Signal` and `chan ShutdownSignal` into a single interface as suggested in this comment: #912 (comment) This change isn't quite right because mapping os.Signal to a ShutdownSignal currently relies on a goroutine which isn't reliably shut down -- so we have leaking tests. Note that this also fixes a behavioral bug in #912: `Wait()` channels would not resolve if a plain signal was received.
This is a proposed change to uber-go#912 by @jasonmills that DRYs up internal state management by unifying `chan os.Signal` and `chan ShutdownSignal` into a single interface as suggested in this comment: uber-go#912 (comment) This change isn't quite right because mapping os.Signal to a ShutdownSignal currently relies on a goroutine which isn't reliably shut down -- so we have leaking tests. Note that this also fixes a behavioral bug in uber-go#912: `Wait()` channels would not resolve if a plain signal was received.
a94d8e2
to
848742a
Compare
afe2df5
to
a69d369
Compare
app.runStart.Do(func() { | ||
app.log().LogEvent(&fxevent.Started{Err: err}) | ||
}) |
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.
Does the LogEvent need to be guarded in the Once? We're allowing multiple actual Start invocations, so they should be able to log multiple times?
app.runStop.Do(func() { | ||
app.log().LogEvent(&fxevent.Stopped{Err: err}) | ||
app.closeStopChannel() | ||
}) |
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.
Same. I'm not sure we need this guard? If there's a stop channel because of Start, then there should be a defer close.
sigReceivers []signalReceiver | ||
signalOnce sync.Once | ||
|
||
// Used to make sure Start/Stop is called only once. |
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.
I'm going to abandon this PR and separate it into some smaller chunks since it has some scope creep. |
Provides API for users of the
Shutdowner
interface to specify an exit code that should be used to exit an application.Addresses issue #763
ShutdownCode
shutdown optionShutdownSignal
type which includes fields for both os.Signal and exit codeWait()
method which operates likeDone()
but instead of returning a os.Signal channel, returns aShutdownSignal
channelShutdownCode
option example test