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

Rate Limiter issues #82

Open
tyuo9980 opened this issue Nov 20, 2017 · 1 comment
Open

Rate Limiter issues #82

tyuo9980 opened this issue Nov 20, 2017 · 1 comment

Comments

@tyuo9980
Copy link

tyuo9980 commented Nov 20, 2017

I have a django server with an endpoint that calls upon riot-watcher a single time. In a script, I spawned 13 threads each with a request to my server and ran them all at once and hit a "cant set attribute" error. The problem is with the line below:

Limits.py :
RawLimit = namedtuple('RawLimit', ['count', 'limit', 'time'])
namedtuples are NOT mutable, so Line 100 of Limits.py will not work:
raw_limit.counts = self._raw_limit.counts
theres a mutable list implementation called namedlist which does the job, but has a default attribute called 'count' so just rename that to whatever.

I'm testing this with a production key so im definitely not hitting any limit, but this gets logged:

'overwriting time limit, previously %s, now %s. ' + 'This may cause rate limitting issues.'

Can you elaborate how the limiter works and any edge cases you may be aware of, especially with these comments in the code:

double check that we arent assigning a lower, non-1 value to
our count. This may be screwy if a bunch of requests are sent
as the rate limit is getting reset, but I dont think There
is a more elegant solution.

In addition, I tested it with a dev key (20calls/sec) with 25 threads and as expected 5 error429's were logged.

@pseudonym117
Copy link
Owner

one limitation of the current rate limiter that i know of is that pending requests are not checked before a request is made. if 25 requests are made at the same time with a limit of 20 request, 5 should currently get a 429.

That comment is specifically talking about when the rate limit resets. Assume you have the limit of 10 calls/min. If you make a 2 requests right as the rate limit is about to reset, request A and request B. Response A is sent back with the header saying 10 requests have been made (so the next call will be limited). Response B is sent back with a header saying that 1 request has been made (the rate limit has reset). If response B is processed before response A, then what will happen is that the count will be set to 1, and then set to 10, leading all subsequent calls to be rate limited for a minute.

I believe that 2nd case is not a very likely case, but i believe it is possible, which is why that comment exists.

regarding the issue writing to the tuple, that seems like a pretty clear bug and should be easy to solve.

Can you split this into 2 separate issues? one for the clear bug writing things that are not writable, and one for the fact that pending requests are not tracked?

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