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
Handle rate limit errors appropriately :: GitHub Doesn't always send Retry-After
header for secondary rate limits
#1805
Comments
Retry-After
header.Retry-After
header for secondary rate limits
The guidance in "about secondary rate limits" is not great. It boils down to "if you make heavy use for our API, you'll probably hit the secondary rate limit sooner or later but we're not going to give you any way to know if you're getting close. That's a you problem." 😡 |
Thanks for filing this.
Not sure why we would want this. The determination of if there is an error is universal and should never need to be customized by users. Whatever you changes you want to do, do it in this library for the benefit of all.
Yes, do this please. The secondary rate limit is essentially a renamed abuse limit. The consequences are the same. If a client is not okay waiting they can use
Maybe? But we still need to handle the case. The docs actually mention it as a possible scenario.
Whatever smarts you're talking about implementing, it is very likely something that all users would benefit from. Similar to primary rate limit behavior, basically everyone needs this functionality. Unlike primary rate limits, there is no concrete information that we can provide to the clients that they could use to avoid exceeding the limit. You either stay under the limit or you don't. The guidance provided about how to estimate usage and limits (time from request to response and then totaling that per minute) is best implemented once for the benefit of all. |
I've looked at the code, and it seems like there's an inconsistency between
github-api/src/main/java/org/kohsuke/github/AbuseLimitHandler.java Lines 89 to 92 in 6b2a487
However, I think it will never get to here, because
It looks to me like
It looks like just this change alone should already make it safer when it comes to hitting secondary rate limits. However, to make it really safe, it looks like we need to do what GitHub suggests:
That is, increasing this one minute sleep exponentially. For this, either changes in the API will be needed (to pass a counter to But anyhow, it all seems to start with fixing the check in If this sounds reasonable, would you be able to fix it, or is it better if I send a PR? |
Please submit a PR. |
Describe the bug
The GitHub Docs now say:
There's also a whole complex list About secondary rate limits.
To Reproduce
Reach Secondary Quota Limits:
Expected behavior
GitHub SHOULD return a retry-after header but it does not. So I've thought of a couple ideas:
Retry-After
header as described in the GitHub Docs. This may not be ideal because note everyone is running a long-running project like I am so they may not be OK with a default 1-minute sleep before throwing a RetryRequestException.Desktop (please complete the following information):
Additional context
BTW I'm using a GitHub Enterprise User Account accessing GH Enterprise resources so maybe that's why there's no
Retry-After
header?Another Idea is related to
rateLimitChecker.checkRateLimit(this, request.rateLimitTarget());
inside the GitHubClient.sendRequest method. If it could also pass the GitHub request instead of just the rateLimitTarget then I could configure more smart ratelimit checker which tracks api calls of differing "points" to reduce throughput of the 'create content' requests which GitHub limits much more heavily than Read requests.The text was updated successfully, but these errors were encountered: