Open jdef opened 6 years ago
hrm .. this is racy. going to mark as WIP for now
forced-pushed a fix for the race
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 notifications@github.com 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 https://github.com/rcrowley/go-metrics/pull/241#issuecomment-425127094, or mute the thread https://github.com/notifications/unsubscribe-auth/AABYRO4YH7HcQ7fvcjTFq3oKgv_mlgmxks5ufOjLgaJpZM4V9du- .
-- Sent from Russia, with love. But also from my phone.
@jdef I think both this patch and #246 updated the .Stop
logic to use CAS, so now there's a conflict. It's probably a very simple rebase. Would you mind giving it a go? Thank you, and sorry about the trouble.
Rebased
@mihasya PTAL
@mihasya PTAL
High travel period for me, my apologies. @wadey @rcrowley any chance you can look?
On Tue, Nov 13, 2018 at 06:16 James DeFelice notifications@github.com wrote:
@mihasya https://github.com/mihasya PTAL
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/rcrowley/go-metrics/pull/241#issuecomment-438047553, or mute the thread https://github.com/notifications/unsubscribe-auth/AABYRHCsmubBecodNZ8iJhEv8fThOFT2ks5uufM1gaJpZM4V9du- .
-- Sent from Russia, with love. But also from my phone.
@mihasya @wadey @rcrowley PTAL
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 notifications@github.com 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 https://github.com/rcrowley/go-metrics/pull/241#issuecomment-445016303, 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
@mihasya PTAL
@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