Stricter Control over Starting/Stopping Lifecycle #994
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
Currently, it is possible for a user to start their app (and thus its lifecycle) twice. This can lead to panics if the user has registered any lifecycle hooks (Ref: uber-go#991). This change will explicitly keep track of whether a lifecycle is currently running, under a lock, to ensure that the lifecycle can never be running twice at the same time.
Codecov Report
@@ Coverage Diff @@
## master #994 +/- ##
==========================================
- Coverage 98.71% 98.32% -0.40%
==========================================
Files 39 39
Lines 1872 1908 +36
==========================================
+ Hits 1848 1876 +28
- Misses 17 25 +8
Partials 7 7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
sywhang
reviewed
Dec 6, 2022
internal/lifecycle/lifecycle.go
Outdated
| type appState int | ||
|
|
||
| const ( | ||
| notRunning appState = iota |
There was a problem hiding this comment.
nit: consider renaming this to stopped.
internal/lifecycle/lifecycle.go
Outdated
| l.mu.Lock() | ||
| if l.state != started && l.state != incompleteStart { | ||
| l.mu.Unlock() | ||
| return errors.New("attempted to stop lifecycle when not running") |
There was a problem hiding this comment.
this error message might make more sense if you printed the current state of l.state rather than just "not running" here since it could be either not running or starting or stopping.
jasonmills
pushed a commit
to jasonmills/fx
that referenced
this pull request
Dec 7, 2022
Currently, there is nothing preventing somebody from making calls to `app.Start` and `app.Stop` when it doesn't make sense, for example calling `app.Start` or `app.Stop` twice, or trying to run the application twice concurrently. Because of this lack of control, it's possible for users to cause race conditions and panics (uber-go#991) by putting the lifecycle is strange positions. This PR places a small state machine in `Lifecycle` so that we can check if calls to `(*Lifecycle).Start` and `(*Lifecycle).Stop` make sense before going through. This will disallow starting an already starting/started app, and stopping an already stopping/stopped app. Tests to verify that are also added.
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.
Currently, there is nothing preventing somebody from making calls to
app.Startandapp.Stopwhen it doesn't make sense, for example callingapp.Startorapp.Stoptwice, or trying to run the application twice concurrently. Because of this lack of control, it's possible for users to cause race conditions and panics (#991) by putting the lifecycle is strange positions.This PR places a small state machine in
Lifecycleso that we can check if calls to(*Lifecycle).Startand(*Lifecycle).Stopmake sense before going through. This will disallow starting an already starting/started app, and stopping an already stopping/stopped app. Tests to verify that are also added.