Honor the rate limit headers in "hub api"

[mdaniel]

Previously, calling hub api --paginate with any substantial list of results would exhaust the Ratelimit allocation, since hub api did not pause before requesting the next page. With this change, hub api will check the rate limit headers and sleep until "Reset" if calling for the next page would exceed the limit.

[mislav]

Hi thank you for your contribution!

This is a very interesting feature. Since the rate limit reset window is 1 hour, this functionality could theoretically make the hub api execution last for up to an hour if the rate limit was exhausted immediately at the beginning of the hour. I'm not sure if it's good for this to be on by default, because I am sure that most users, particularly when running the hub api script manually in their own terminal, would perceive this delay as hub getting "frozen" and terminating the process.

However, I do see value in this being optionally enabled per-invocation. How would you imagine the flag for hub api to control this be called?

[mislav]

If we couldn't close a file for whatever reason, I think that it would be a much better idea to allow the program to continue (it's not a big deal, after all) rather than to halt it.

[mislav]

Nice, elegant implementation of this feature! 👍

[mdaniel]

Since the rate limit reset window is 1 hour, this functionality could theoretically make the hub api execution last for up to an hour

That wasn't my experience, but I was also using hub api with $GITHUB_TOKEN set, which I (perhaps erroneously) thought was the most common scenario. However, I tried hub api without providing credentials and it wasn't immediately obvious how to get that to work (there's even client.ensureAccessToken which implies it must be present)

Anyway, with my token set, the Reset window is measured in seconds for me

> X-Ratelimit-Limit: 30
> X-Ratelimit-Remaining: 29
> X-Ratelimit-Reset: 1571720200
$ gdate --date='@1571720200'
Mon Oct 21 21:56:40 PDT 2019
$ date
Mon Oct 21 21:56:53 PDT 2019

if the rate limit was exhausted immediately at the beginning of the hour. I'm not sure if it's good for this to be on by default, because I am sure that most users, particularly when running the hub api script manually in their own terminal, would perceive this delay as hub getting "frozen" and terminating the process.

FWIW, the change does include a message to stderr about why it is sleeping and for how long, but I hear you. I stopped short off adding a flag for --honor-rate-limit or --sleep-for-rate-limit ... because whew naming things, but also because I wasn't sure if it should be opt-in to have --paginate honor the rate limit, since maybe people currently just accept that pagination will fail without warning, or have it be opt-out to specifically say "I don't care about the limit, give me as much as you can and die as before". I welcome your input on flag-or-no-flag and for sure welcome what you think would be a good name for it

[mislav]

The diff looks way cleaner, thank you! Some questions/ideas that I have:

1. Considering that this might be opt-in via flag (I will keep thinking about what the appropriate CLI API for this could be), would it make sense for this behavior to also handle API error responses that are the direct result of the rate limit? E.g. if the very first hub api request results in an HTTP error because of the rate limit being hit, we could handle that error by waiting until reset. How difficult would it be to extend your current approach to cover that?

2. If this is opt-in, should we also apply the overall behavior to all hub api requests, not just in pagination mode?

[mislav]

I'm also wondering whether the appropriate place to implement the wait-until-ratelimit-reset behavior would be in a RoundTripper https://github.com/github/hub/blob/cd81a7bada3d2986f213db424acaa570523a9c9f/github/http.go#L51

This behavior could be off-by-default, but activated on-demand via command-line flag or environment variable. Just thinking out loud

[mdaniel]

Just to follow up so you don't think I have forgotten about this, I haven't yet made the time to try out the suggestions (about whether rate limiting is zero or one based, trying to catch the actual rate-limit status code, and trying out the RoundTripper), but -- aside from the opt-in flag -- are those merge blockers or nice-to-haves?

[mdaniel]

By coincidence, I actually experienced rate-limit exhaustion today, so I can report back some specifics:

Sadly, the API responds with HTTP/1.1 403 Forbidden instead of the much more sane 429, so I'm not comfortable right now saying that I want to trap 403s and go spelunking in the response body for {"message":"API rate limit exceeded for user ID ...", above and beyond the fact that retrying things under api makes me super nervous.

I also discovered that it is zero indexed, and it does actually deliver X-Ratelimit-Remaining: 0 coming back in the 200 response right before the 403, so I'll straighten that out

[mislav]

Awesome, thanks for the updates!

I think that it won't be too hard to detect the HTTP 403 that happened as a result of rate limiting. You can use ErrorInfo to inspect the message returned from API: https://github.com/github/hub/blob/03533a167e09c53323b892c38e3257bdfdb47a6d/github/http.go#L490

if resp.StatusCode == 403 {
  if info, err := resp.ErrorInfo(); err == nil {
    if strings.Contains(info.Message, "rate limit exceeded") { ... }
  }
}

I think that hub api --rate-limit should obey rate limits even --pagination wasn't used and even if the 1st request already failed due to rate limiting.

It's safe to repeat GET requests. I would strictly avoid repeating any requests other than GET or HEAD.

[mislav]

I would rather that we ignore errors instead of aborting the process on malformed response header values

[mdaniel]

I am strongly against the very idea of "ignoring errors," as one can tell from my previous attempt to actually trap them before being asked to roll that back. If the project really feels strongly that hub should swallow errors, I'll capitulate to get this PR in, but I can assure you that as an end user of software, I have been bitten more times by swallowed errors than I have ever thought to myself "wow, this software talks about errors too much"

[mislav]

Also, this feature could use a test! See features/api.feature

We try to avoid slow tests, so if a scenario is triggering the Sleep() call, we should take precaution to not trigger it for longer than 1s.

[mdaniel]

Also, this feature could use a test! See features/api.feature

Great, now I need to do ruby :-( That features directory could use a README.md explaining the relationship between steps.rb and the .feature files, and/or put that information in CONTRIBUTING.md (which does kind of hint at it, but "study the existing ones" is not as friendly for newcomers, IMHO)

The Vagrantfile has gone bit-rotten:

    linux: Setting up ruby1.9.1-dev (1.9.3.484-2ubuntu1.14) ...
    linux: ERROR:  Error installing bundler:
    linux: 	bundler requires Ruby version >= 2.3.0.

and it also erroneously assumes the project is in a $GOPATH, which is no longer required thanks to go.mod and friends, and similarly its version of go 1.4 is hilariously out of date

After straightening all that out, make test-all fails, but I was able to fish out the command that I care about cucumber -p all features/api.feature and that's all I tested locally. A miserable "run the tests before pushing" experience, for sure

[mdaniel]

I think that it won't be too hard to detect the HTTP 403 that happened as a result of rate limiting. You can use ErrorInfo to inspect the message returned from API:

So, as I asked previously: is that extra work mandatory to land this, or a nice-to-have? Because this started out as scratching an itch of mine, and finding how to trap rate limiting in all of those scenarios without inadvertently introducing weird bugs feels like Real Work™

[mislav]

I'm sorry you had a bad experience running tests. The Vagrantfile is misleading; it was for a different purpose than running tests in and is no longer accurate.

Don't worry about the rest of the functionality that I proposed. I can take over from here and get it over the finish line. Thank you for the work you've put into this.

[mdaniel]

That's awesome that there is now a docker container; thank you for getting that in

And my thanks for all your continued patience with this. I'm sorry I didn't have the mental bandwidth right now to chase this issue all the way down to its ideal solution

[mislav]

@mdaniel Thank you for your work so far. I've added some changes:

• Squashed your commits to simplify history
• Renamed the flag to --obey-ratelimit for clarity; "rate-limit" by itself just described the concept but not the behavior
• Cleaned up the implementation; e.g. removed the code and cukes around handling parse errors in ratelimit headers
• Have HTTP 403 responses with --obey-ratelimit be retried after waiting, even if no --paginate

Please review! 🙇