-
Notifications
You must be signed in to change notification settings - Fork 611
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
Ensure task reaper stopChan is closed no more than once #2491
Conversation
LGTM but add some assurance that run cannot be called twice. |
// TODO(dperny) calling stop on the task reaper twice will cause a panic | ||
// because we try to close a channel that will already have been closed. | ||
close(tr.stopChan) | ||
tr.closeOnce.Do(func() { |
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.
What is the reason to have multiple invocation for this function ?
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.
Kicked off a rebuild. Lets close this out soon ? @nishanttotla
Codecov Report
@@ Coverage Diff @@
## master #2491 +/- ##
==========================================
- Coverage 62.09% 61.53% -0.56%
==========================================
Files 49 129 +80
Lines 6898 21315 +14417
==========================================
+ Hits 4283 13117 +8834
- Misses 2174 6796 +4622
- Partials 441 1402 +961 |
@@ -282,8 +286,8 @@ func (tr *TaskReaper) tick() { | |||
|
|||
// Stop stops the TaskReaper and waits for the main loop to exit. | |||
func (tr *TaskReaper) Stop() { | |||
// TODO(dperny) calling stop on the task reaper twice will cause a panic |
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.
@dperny do you remember the reason for this? I'm looking into it too.
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.
It's never happened, IIRC, it's just possible for it to happen. Other components have Stop channels that are guarded against double close.
I noticed that This is a very corner case scenario, but it seems realistic because both
WDYT @anshulpundir @dperny @stevvooe? |
Makes sense. Lets add that comment to TaskReaper.Stop() and land this change @nishanttotla |
Actually, I do have a change request, can we log an error or warning message if we try to stop the taskreaper twice? It's a corner case, and it's better to log an error than to panic the program, but it's an undesirable occurrence. Something like log.L.Warn("Stop called twice on taskreaper") |
6751f66
to
4657b27
Compare
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
4657b27
to
57139d0
Compare
// shutting down, and the other when the manager (the leader) is | ||
// becoming a follower. Since these two instances could race with | ||
// each other, we use closeOnce here to ensure that TaskReaper.Stop() | ||
// is called only once to avoid a panic. |
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.
@anshulpundir added a comment here.
I think the test flakiness is related, I'm investigating. |
@dperny @anshulpundir have we seen this kind of flakiness in other PRs?
|
Okay tests seem to pass now, the flakiness is strange but I can't say that it's related to this PR anymore. As far as @dperny's comment about logging is concerned, it's not possible to do it with a Ping @dperny @anshulpundir |
The flakiness is fixed in 7d507f6 |
I merged it, not sure if you were planning to address #2491 (comment) If so, I can add it to my next PR @nishanttotla |
@anshulpundir I responded to that comment briefly in my last one. It's not possible to add just a simple log message with |
Signed-off-by: Nishant Totla nishanttotla@gmail.com