app: Add internal option to override os.Exit #792
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abhinav
force-pushed
the
abg/os-exit-option
branch
from
b4a38c2 to
ca83c7a
Compare
shirchen
approved these changes
Sep 17, 2021
sywhang
approved these changes
Sep 17, 2021
Add an option exposed only to tests in the go.uber.org/fx package that allows overriding how `os.Exit` behaves. The way the new option works is, - `App` gets a new `osExit func(int)` field which defaults to `os.Exit` - app_internal_test.go uses `package fx` and therefore, it can access internal fields of the `App`. We define a new exported option in this package that sets `osExit`. Because this option is defined in a test file, though, it is not made part of the public API of the package. - app_test.go uses `package fx_test` so it's an external test--it cannot access unexported fields, but it can access the exported option defined in app_internal_test.go. This will allow testing the `app.Run` function directly.
abhinav
force-pushed
the
abg/os-exit-option
branch
from
ca83c7a to
14f0965
Compare
abhinav
added a commit
that referenced
this pull request
Sep 17, 2021
As reported in #782, `App.Run` had a bug so that if an application using it failed to start up because of a timeout no event would be logged to indicate this. The system would silently exit with a non-zero status code. This PR resolves this by ensuring that even for timeouts, we log `Started` or `Stopped` events with non-nil `Err` values. This matches the behavior for other startup or shutdown failures. To test this, I wanted to use `App.Run` directly so I added an internal test-only option `WithExit` that lets us override the behavior of `os.Exit` without a global variable. (#792) The issue resolution was pretty straightforward: - Start: `defer LogEvent(..)` in Start instead of the wrapped `start` method which runs inside a separate gorotuine. - Stop: Apply `defer LogEvent(..)` in Stop instead of `App.Run`. This actually exposed a bug too: we did not previously log `Stopped` if `App.Stop` was called. This resulted in some test changes. There was a small non-determinism in `withTimeout`: What if the context times out at the same time as the callback finishing? `select` will pick one of the `case`s randomly. I resolved this non-determinism by checking `ctx.Err()` in the success case again. I also moved `App.start` next to `App.Start`, and `App.run` next to `App.Run` to aid in readability. The move is in #791. Resolves #782 Resolves GO-866
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add an option exposed only to tests in the go.uber.org/fx package that
allows overriding how
os.Exitbehaves.The way the new option works is,
Appgets a newosExit func(int)field which defaults toos.Exitpackage fxand therefore, it can accessinternal fields of the
App. We define a new exported option in thispackage that sets
osExit. Because this option is defined in a testfile, though, it is not made part of the public API of the package.
package fx_testso it's an external test--it cannotaccess unexported fields, but it can access the exported option
defined in app_internal_test.go.
This will allow testing the
app.Runfunction directly.