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

Implements ShutdownCode option and ShutdownSignal os.Signal wrapper #912

Closed
wants to merge 9 commits into from

Conversation

jasonmills
Copy link
Contributor

@jasonmills jasonmills commented Aug 4, 2022

Provides API for users of the Shutdowner interface to specify an exit code that should be used to exit an application.
Addresses issue #763

  • Adds ShutdownCode shutdown option
  • Adds ShutdownSignal type which includes fields for both os.Signal and exit code
  • Adds Wait() method which operates like Done() but instead of returning a os.Signal channel, returns a ShutdownSignal channel
  • Adds ShutdownCode option example test
  • Includes broadcast signaling refactor by @abhinav
	app := fx.New(
		fx.Invoke(func(shutdowner fx.Shutdowner) {
			// Call the shutdowner Shutdown method with a shutdown code
			// option
			shutdowner.Shutdown(fx.ShutdownCode(1))
		}),
	)

	app.Run()

	wait := app.Wait()

	signal := <-wait

	os.Exit(signal.ExitCode)

@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #912 (196f699) into master (0de8ec7) will decrease coverage by 0.15%.
The diff coverage is 94.73%.

@@            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              
Impacted Files Coverage Δ
app.go 95.57% <94.00%> (-0.80%) ⬇️
shutdown.go 95.74% <95.55%> (-4.26%) ⬇️
fxevent/zap.go 100.00% <0.00%> (ø)
docs/internal/exectest/cmd.go
docs/internal/httptest/http.go
docs/internal/iotest/read.go
docs/internal/test/fake.go
docs/internal/exectest/output.go
docs/internal/apptest/run.go

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jasonmills jasonmills force-pushed the shutdown_with_exit_code branch 4 times, most recently from 764432a to 1c55c52 Compare August 4, 2022 23:49
Copy link
Contributor

@sywhang sywhang left a 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:

  1. Call Done() to get a channel
  2. Receive from that channel
  3. Create a ShutdownSignal from that channel
  4. 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:

  1. Call Exit() to get a channel
  2. Receive from that channel
  3. Check its exit code.

which removes the awkward step of having to create a random type out of something returned from the Done channel.

shutdown_code_example_test.go Outdated Show resolved Hide resolved
shutdown_code_example_test.go Outdated Show resolved Hide resolved
@jasonmills jasonmills force-pushed the shutdown_with_exit_code branch 2 times, most recently from 7dad518 to b8a589f Compare August 8, 2022 22:00
app.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
shutdown.go Outdated Show resolved Hide resolved
app.go Outdated Show resolved Hide resolved
abhinav added a commit that referenced this pull request Sep 28, 2022
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.
jasonmills and others added 7 commits October 27, 2022 20:22
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.
Comment on lines +623 to +625
app.runStart.Do(func() {
app.log().LogEvent(&fxevent.Started{Err: err})
})
Copy link
Collaborator

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?

Comment on lines +695 to +698
app.runStop.Do(func() {
app.log().LogEvent(&fxevent.Stopped{Err: err})
app.closeStopChannel()
})
Copy link
Collaborator

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need this to be included in this PR? I'm asking because this stuff was supposed to ship with the previous version (v1.17.2) and it broke some users: see #945 and #950 for context.

@jasonmills
Copy link
Contributor Author

I'm going to abandon this PR and separate it into some smaller chunks since it has some scope creep.

@jasonmills jasonmills closed this Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants