[Network] refactor lifecycle manager #1031
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1031 +/- ##
==========================================
+ Coverage 53.27% 53.42% +0.14%
==========================================
Files 318 318
Lines 21515 21521 +6
==========================================
+ Hits 11462 11497 +35
+ Misses 8487 8455 -32
- Partials 1566 1569 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| func RequireClosed(t *testing.T, ch <-chan struct{}, message string) { | ||
| select { | ||
| case <-ch: | ||
| default: | ||
| require.Fail(t, "channel is not closed: "+message) | ||
| } | ||
| } |
There was a problem hiding this comment.
Note: this is guaranteed to select from the channel if it is closed, and will only run the default case if nothing is available to be read from the channel: https://stackoverflow.com/questions/45580151/priority-of-case-versus-default-in-golang-select-statements
| func RequireNotClosed(t *testing.T, ch <-chan struct{}, message string) { | ||
| select { | ||
| case <-ch: | ||
| require.Fail(t, "channel is closed: "+message) | ||
| default: | ||
| } | ||
| } |
| lm.stateTransition.Lock() | ||
| if lm.shutdownCommenced || lm.startupCommenced { | ||
| lm.stateTransition.Unlock() | ||
| return | ||
| } | ||
| lm.startupCommenced = true | ||
| lm.stateTransition.Unlock() |
There was a problem hiding this comment.
| lm.stateTransition.Lock() | |
| if lm.shutdownCommenced || lm.startupCommenced { | |
| lm.stateTransition.Unlock() | |
| return | |
| } | |
| lm.startupCommenced = true | |
| lm.stateTransition.Unlock() | |
| func(){ | |
| lm.stateTransition.Lock() | |
| defer lm.stateTransition.Unlock() | |
| if lm.shutdownCommenced || lm.startupCommenced { | |
| return | |
| } | |
| lm.startupCommenced = true | |
| }() |
Using defer pattern is safer in this scenario.
There was a problem hiding this comment.
Thanks! In this particular case, I think it may introduce a bit more messiness than your suggested change, because when we return on line 79 we actually want to return from the enclosing function.
If we wrap the logic in a nested function, the return now only returns from the nested function, so we would probably have to add some if / else logic and return a boolean from the nested function instead.
I think that may not be necessary here, since the locking code is very short and contained
| lm.stateTransition.Lock() | ||
| if lm.shutdownCommenced { | ||
| lm.stateTransition.Unlock() | ||
| return | ||
| } | ||
| lm.shutdownCommenced = true | ||
| lm.stateTransition.Unlock() |
There was a problem hiding this comment.
Same defer pattern is suggested here.
module/lifecycle/lifecycle_test.go
Outdated
|
|
||
| // TestConsecutiveStart tests that calling OnStart multiple times concurrently only | ||
| // results in startup being performed once | ||
| func (suite *LifecycleManagerSuite) TestConsecutiveStart() { |
There was a problem hiding this comment.
The test scenario looks like concurrent than consecutive.
module/lifecycle/lifecycle_test.go
Outdated
|
|
||
| // TestConsecutiveStop tests that calling OnStop multiple times concurrently only | ||
| // results in shutdown being performed once | ||
| func (suite *LifecycleManagerSuite) TestConsecutiveStop() { |
There was a problem hiding this comment.
The test scenario looks like concurrent than consecutive.
module/lifecycle/lifecycle_test.go
Outdated
|
|
||
| suite.lm.OnStart(func() { | ||
| // simulate startup processing | ||
| time.Sleep(3 * time.Second) |
There was a problem hiding this comment.
**General comment: ** sleep durations in this PR are in order of seconds. This slows down building over CI. Would be great if you could please reduce them to milliseconds if possible. For most of the cases, 100 milliseconds is a preferable sleep duration
There was a problem hiding this comment.
or you could use a channel here as a gate. After the call to suite.lm.OnStop(), you can close the channel (a.k.a. open the gate). Then we won't have to rely on a sleep
| unittest.RequireCloseBefore(suite.T(), suite.lm.Started(), time.Second, "timed out waiting for startup") | ||
| time.Sleep(100 * time.Millisecond) // wait for potential race conditions to occur | ||
|
|
||
| suite.Assert().EqualValues(1, numStarts) |
There was a problem hiding this comment.
we can use assert.Eventuallyf here to factor in the sleep
| }() | ||
|
|
||
| return n.done | ||
| n.cancel() |
Context: #1026 (review)
This PR refactors the lifecycle management code in
Networkinto a separate struct.This struct can be used in the future to refactor many existing
ReadyDoneAwaremodules: https://github.com/dapperlabs/flow-go/issues/5702closes https://github.com/dapperlabs/flow-go/issues/5704