Skip to content
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

Allow *Cilent to opt-out of ratelimiting? #98

Closed
jbizzle opened this issue May 4, 2017 · 3 comments
Closed

Allow *Cilent to opt-out of ratelimiting? #98

jbizzle opened this issue May 4, 2017 · 3 comments

Comments

@jbizzle
Copy link
Contributor

jbizzle commented May 4, 2017

Is there some way to disable the builtin ratelimiting started in NewClient()?

Our calling pattern makes it hard to share clients across requests, and so we would prefer to create a new client per request and do our own higher-level ratelimiting.

AFAICT the current implementation of NewClient() starts a goroutine which will never exit, since the ratelimiter channel it feeds is never closed. Additionally, it seems to be using time.Tick(), in which case the ticker 'cannot be recovered by the garbage collector; it "leaks,"' (time Godoc) so we leak 2 things per client.

Am I misunderstanding the code? If not, is there some way I'm overlooking to close the ratelimiter-related resources, or disable it entirely? If not that, then how do you feel about my adding it?

@domesticmouse
Copy link
Contributor

This feels like a fairly major change, where in we would need to create a "one shot" client that doesn't have rate limiting associated with the client.

I'm happy to review a pull request for this, but maybe a fork is more appropriate for your change? It feels like you could wind up with a much simpler client due to your higher level rate limiting functionality.

@domesticmouse
Copy link
Contributor

I believe this is now done and dusted thanks to @jbizzle's fine handiwork.

@jbizzle
Copy link
Contributor Author

jbizzle commented May 30, 2017

Thanks for looking so quickly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants