What happens if a client is using this library in a threaded way and multiple threads are accessing the API?

From reading the code it looks like a wait forever condition, should there be a hard timeout?

Can there be an opt-out for consumers that handle rate limiting themselves?

I have specifically refused to add this automagic handling to github3.py. I'd be happy to have an object that provides an interface to handle rate-limiting but users would always need to opt into that.

@omgjlk
Do you refer to the exponential back-off that could lead to infinite waits?

In general I have to admit to not have put any thought into concurrency issues yet. In any case, we need to differentiate between rate limit and abuse detection since they need to be handled quite differently: rate limiting is transparent and can be easily complied with by reading headers. On the other hand, how the abuse detection mechanism works is rather opaque and can only be avoided by common sense and caution or needs to be dealt with after the fact.

With that in mind I can say that the logic for dealing with rate limits should work with threads requesting concurrently in principle (bar access to the cached rate limit values).

As you point out correctly, the abuse detection handling currently blocks every time and would even increase back off time indefinitely. This is definitely a bad design. Instead of (or complementary to) an upper limit for the wait time, I think it would be best to avoid running into the abuse limit with multiple threads by holding a lock to the entire request method; ie other threads should be blocked from contacting the API in order to avoid worsening the situation.

@sigmavirus24
Thanks for the frank feedback early on (man, am I happy that I didn't fix all these tests yet 😉). Do you want me to pursue an opt-in version at all or should I just drop this merge request?

I developed this for a project where it is implemented by inheriting from GithubSession and overwritingrequest. One approach for making this optional could be to factor out any code related to rate limits and create a child class RateLimitedSession which could be used in place of GithubSession in case a certain flag is set when initializing a new Github object.

Is this (or another approach) something you would accept?

@omgjlk, do these suggestions address your concerns appropriately?

Do you want me to pursue an opt-in version at all or should I just drop this merge request?

If you want to. I'm not your boss. I'd love to not have to write this myself but I won't dictate how you spend your free time. I sincerely appreciate the fact that you even wrote and submitted this in the first place.

I developed this for a project where it is implemented by inheriting from GithubSession and overwriting request. One approach for making this optional could be to factor out any code related to rate limits and create a child class RateLimitedSession which could be used in place of GithubSession in case a certain flag is set when initializing a new Github object.

Is this (or another approach) something you would accept?

So I had two approaches in mind and I really didn't have a strong feeling on which way to go with them.

The first approach was what you described: A RateLimitedGitHubSession. I think the one thing we differ on (based on what you have here) is that the library itself shouldn't implement the sleeps. I think we should raise an exception that includes the ratelimit values as well as the suggested sleep for users to use to sleep on their own (or schedule a task to retry if they're using celery or whatever).

The second approach was a variation on the first: A RateLimitedGitHubTransportAdapter this could be mounted on the session so that someone could do:

gh = GitHub()
gh.login(...)
gh.session.mount('https://', RateLimitedGitHubTransportAdapter())  # or
gh.mount_ratelimiting_adapter(RateLimitedGitHubTransportAadapter)

This would also allow someone to subclass just the transport adapter and perhaps add sleeps there if they so choose. It would be less work than subclassing the session etc.

Finally, I don't know that we want to introduce a new keyword for ratelimiting, but instead provide methods to do the work for them.

# Ideas
gh.use_ratelimiting_session(RateLimitedGitHubSession)
gh.mount_ratelimiting_adapter(RateLimitedGitHubAdapter)
gh.use_ratelimiting_with(...)  # Could be the same method regardless of which direction we go

Nice to hear that you are interested in the functionality. I will try to get it into shape.

So I had two approaches in mind and I really didn't have a strong feeling on which way to go with them.

Although creating an adapter sounds like a good approach, I prefer extending the session class, because that is what I know already. I might reconsider if the approach I already have is easily adaptable to an adapter.

the library itself shouldn't implement the sleeps

IMHO, if the library does not add sleeps, then the entire class is superfluous:

• Exceptions are thrown already when a limit is hit.
• The information how long to wait is also available in the response headers.

What added value would this pull request then bring and why would it need to be opt-in?

Finally, I don't know that we want to introduce a new keyword for ratelimiting, but instead provide methods to do the work for them.

Fair enough.

What added value would this pull request then bring and why would it need to be opt-in?

Some added value could be to create more descriptive Exception classes like class AbuseLimitError(ForbiddenError) and class RateLimitError(ForbiddenError) that contain information from the headers in fields (such as suggested wait times).

This is even backwards compatible because any existing code that excepts ForbiddenError would also catch the newly introduced exceptions.

I still think that would not need to be opt-in and it is but a tiny fraction of what this pull request was about: Automatic handling of rate limits.

So if github3.py uses sleeps directly then that removes a the user's ability to handle the ratelimiting the way they want to. This is one area where I don't think we should be prescriptive. I can imagine a large system talking to GitHub hitting its ratelimit and all of a sudden all of the workers are tied up in time.sleep because of github3.py. That is a horrible thing to do to users. Libraries shouldn't be surprising. We can include a sleeping rate-limiter that people use. Definitely as the smallest possible change, though, more specific rate limit exceptions would be fantastic. I'm definitely on board with those.

Locally I have replaced most of the changes in this merge request with improved exceptions, as discussed above. The way I had the rate limit handling implemented hooked into sessions, i.e. before exceptions are raised. That means the rate limit handling in this state cannot take advantage of the improved exceptions.

You, @sigmavirus24, seem prefer having this separated in such a way that users of the framework need to explicitly opt-in to automatic handling of ratelimit errors.

A possibly way of combining this could be by having decorators that a user attaches to any code they want rate limits handled in. The decorators could catch exceptions and introduce sleeps where necessary.

This is a mock-up:

def sleep_ratelimit_handler_decorator(func):
    def wrapper(*args, **kwargs):
        while true:
            try:
                return func(*args, **kwargs)
            except RateLimitError as e:
                sleep_time = e.ratelimit_reset - datetime.now()
                time.sleep(sleep_time.total_seconds)
            except AbuseLimitError as e:
                time.sleep(e.retry_after)
    return wrapper

I think something like that would be a great addition. How does creating a module, e.g., github3.contrib sound? I think we also have enough decorators (where we want to do the right thing) that we might need to introduce wrapt as a dependency to make sure they play well.