meter: once all meters are stopped, reset arbiter #241
Conversation
|
hrm .. this is racy. going to mark as WIP for now |
jdef
changed the title
|
forced-pushed a fix for the race |
jdef
changed the title
|
Looks like CI flaked out. I don't see an option to restart the failed build. |
|
Took me a really long time to find the "rebuild" button because it's just hidden when you're not logged in 🤦♂️ . Let's see if it succeeds on second try. |
|
@mihasya thanks for the rebuild. Does this like something worth landing on master? |
|
PTAL, would love to avoid maintaining a fork for this |
|
My bad, will try to look in the next week.
On Thu, Sep 27, 2018 at 10:03 James DeFelice ***@***.***> wrote:
PTAL, would love to avoid maintaining a fork for this
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#241 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABYRO4YH7HcQ7fvcjTFq3oKgv_mlgmxks5ufOjLgaJpZM4V9du->
.
--
Sent from Russia, with love. But also from my phone.
|
|
Rebased |
|
@mihasya PTAL |
1 similar comment
|
@mihasya PTAL |
|
On Tue, Nov 13, 2018 at 06:16 James DeFelice ***@***.***> wrote:
@mihasya <https://github.com/mihasya> PTAL
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#241 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABYRHCsmubBecodNZ8iJhEv8fThOFT2ks5uufM1gaJpZM4V9du->
.
--
Sent from Russia, with love. But also from my phone.
|
|
Today's the day. Got one small question, otherwise should be good. Thank you for your patience. |
|
OK @mihasya, what's your question? |
|
I thought I added an inline comment, will double check.
On Thu, Dec 6, 2018 at 12:21 James DeFelice ***@***.***> wrote:
OK @mihasya <https://github.com/mihasya>, what's your question?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#241 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABYRO29ut4NG0k1lJwUCW4EfjivrRUZks5u2XxOgaJpZM4V9du->
.
--
Sent from Russia, with love. But also from my phone.
|
|
@mihasya I don't see any additional comments. Mind taking a few seconds to re-review this change? |
|
@mihasya PTAL |
1 similar comment
|
@mihasya PTAL |
| } | ||
|
|
||
| var arbiter = meterArbiter{ticker: time.NewTicker(5e9), meters: make(map[*StandardMeter]struct{})} | ||
|
|
||
| // Ticks meters on the scheduled interval | ||
| func (ma *meterArbiter) tick() { | ||
| func (ma *meterArbiter) tick(ticker *time.Ticker, cancel <-chan struct{}) { |
There was a problem hiding this comment.
Why make ticker out as its own argument rather than just referring to the member ticker? Seems like everywhere this is used, it's just calling ma.ticker.
|
@jdef I just realized why you weren't seeing my comment - I made a comment as part of a "review" but didn't click "Submit Review" - so it showed up as part of the PR thread when I looked at it, but not you 🤦♂ |
There's a background goroutine that always runs once a single (non-nil) meter has been added to the system. Even if that meter stops, the background goroutine continues to run. This confuses unit and component tests that check for goroutine leakage. Until this PR, there was no way to terminate the background (arbiter) goroutine created by this package.
With the introduction of this change, once all known meters have been stopped the arbiter is "reset" and the background goroutine terminated.
An example of a popular goroutine leak checker: https://github.com/fortytw2/leaktest