No way to stop SendLoop when used with time.Ticker

[jlordiales]

The SendLoop func on many of the metric reporters takes in a <-chan time.Time and blocks until that channel is closed.
As the docs say, this is most typically used by creating a time.Ticker and passing its C channel.

This works great except for the fact that the C channel in the ticker can never be closed. You can call Stop on the ticker but this doesn't close the channel (golang/go#2650).
This effectively means that there's no straightforward way to return from the SendLoop func when used together with a time.Ticker.

I guess it would be nice if the func could take a ctx as first parameter and return when ctx.Done() is closed.

[peterbourgon]

As far as I know there is no practical reason to shut down any of these reporters, neither in actual programs nor in tests.

[peterbourgon]

Said another way: changing the API is a breaking change, and I don't see a good reason to do it. But I'm open to arguments otherwise.

[jlordiales]

This became a problem for me when I tried to use it together with the run.Group package, because having no way to stop this one actor meant that the g.Run also never returned (and the application had to be forcefully killed). Conceptually it felt like the perfect kind of thing you'd run in there. Maybe this was never intended to run as part of a group of components, but rather as an independent goroutine? In that case I think even a small sentence in the func docs would be really helpful. I ended up re-implementing the SendLoop in my code, which only takes a few lines of code. So it's not a big deal but it can be quite a surprising behavior when you're not aware of it. Feel free to close this.

…

On Tue, Feb 19, 2019, 15:43 Peter Bourgon ***@***.***> wrote: Said another way: changing the API is a breaking change, and I don't see a good reason to do it. But I'm open to arguments otherwise. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#836 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABqqahZhnAZpkKCvVAIhOye5I3J9L8GBks5vPEXXgaJpZM4a_xzx> .

[peterbourgon]

That's a great point. I'll happily take a PR. I agree that ctx is the right approach.