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

Add an option to automatically obey rate limits #153

Closed
wants to merge 1 commit into from
Closed

Add an option to automatically obey rate limits #153

wants to merge 1 commit into from

Conversation

imjasonh
Copy link

This is done by checking whether the number of remaining requests we
have according to previous rate limit header responses is >5% of the
total number of requests per hour we are allowed, and if the client is
below that number, it will wait a short amount of time until issuing the
API request.

This is not fully threadsafe and may produce surprising results if two
identically-configured clients are making requests at the same time,
consuming the same quota pool. The 5% soft threshold is intended to
alleviate some of this, but it can still be fooled by a determined user.

This is only intended as a convenience to users, not as a guarantee that
rate limits won't be hit while using this option.

This also adds a test that checks that requests against a
rate-limit-enforcing server succeed with the option on and fail with it
off.

This attempts to address #152

This is done by checking whether the number of remaining requests we
have according to previous rate limit header responses is >5% of the
total number of requests per hour we are allowed, and if the client is
below that number, it will wait a short amount of time until issuing the
API request.

This is not fully threadsafe and may produce surprising results if two
identically-configured clients are making requests at the same time,
consuming the same quota pool. The 5% soft threshold is intended to
alleviate some of this, but it can still be fooled by a determined user.

This is only intended as a convenience to users, not as a guarantee that
rate limits won't be hit while using this option.

This also adds a test that checks that requests against a
rate-limit-enforcing server succeed with the option on and fail with it
off.
@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@willnorris
Copy link
Collaborator

(sorry for the stupid-long delay on getting around to review this 😞)

I'm not sure that sleeping is always the right thing to do in the event that a rate limit has been hit. Rather, I think the library should construct and return a 403 response (the same that GitHub would return if you're over the limit). The application can then decide to either sleep until the limit expires, or do something else.

So that makes the changes to the library much simpler: we already record the most recent Rate in Client, so we could just update Do() to consult that first, and return a constructed 403 response if Remaining is 0. The only catch is that client.Rate isn't a pointer, so it will initialize with a zero Remaining value... we'd also need to make sure that client.Rate.Limit is non-zero, otherwise the very first call would fail.

@imjasonh
Copy link
Author

Ha, no worries, I actually figured out it wasn't the right way to go and pretty much abandoned the idea. :)

The main problem is that GitHub's rate limiting is done in a, uh, suboptimal way. Instead of having a gradually refilling bucket of quota, it simply sets a reset time an hour after your first request and says you get all of your quota back at that time. So in a lot of cases these requests will pile up for an ~hour, which is a really long time to wait without any feedback.

As such, I think it's likely the developer will want to have more control/visibility into the process than just allowing go-github to handle things transparently. It would be possible to write such a thing as a custom wrapper around go-github if you really wanted to, but that would be up to each developer.

Your proposal to proactively self-rate-limit when we think we'll be over does make some sense, but I'd worry that you'd end up in cases where the server would have let you make the request but for whatever you self-limit. As Walter Sobchak reminds us, "the Supreme Court has roundly rejected prior restraint." :)

@imjasonh imjasonh closed this Jan 21, 2015
@willnorris
Copy link
Collaborator

agreed. Though I do think go-github could make it a little easier for an application to detect that it is a rate limit error versus some other kind of error. I'll leave #152 open, and rename it to capture that.

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

Successfully merging this pull request may close these issues.

3 participants