Bugfixes and improvements to rate limit check #701
Conversation
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
==========================================
- Coverage 77.99% 75.02% -2.98%
==========================================
Files 36 36
Lines 2372 2346 -26
==========================================
- Hits 1850 1760 -90
- Misses 522 586 +64
Continue to review full report at Codecov.
|
| ip = Sockets.getsockname(tcp)[1] | ||
| rate = Float64(rate_limit.num) | ||
| rl = get!(RATE_LIMITS[Threads.threadid()], ip, RateLimit(rate, Dates.now())) | ||
| ip = Sockets.getpeername(tcp)[1] |
There was a problem hiding this comment.
How did this ever work before? It essentially acted like a global rate_limiter (same as max_connections) IIUC.
There was a problem hiding this comment.
Yeah, this was never very well-tested
| rate = Float64(rate_limit.num) | ||
| rl = get!(RATE_LIMITS[Threads.threadid()], ip, RateLimit(rate, Dates.now())) | ||
| ip = Sockets.getpeername(tcp)[1] | ||
| rl = get!(RATE_LIMITS[Threads.threadid()], ip, RateLimit(rate_limit, Dates.DateTime(0))) |
There was a problem hiding this comment.
Use DateTime(0) here to not "punish" first time connections.
| if rl.allowance > rate | ||
| @warn "throttling $ip" | ||
| rl.allowance = rate | ||
| end |
There was a problem hiding this comment.
Moved into the update! function with the warning removed. Why should HTTP warn here at all?
There was a problem hiding this comment.
I just thought it might be useful to know, server-side, if there are potentially malicious calls going on.
There was a problem hiding this comment.
But this is not the case here. For example if rate limiting allowed 2 connections per second, and it was 10 seconds since my last connection this would warn because my allowance would be 20 connections?
| @@ -51,14 +54,9 @@ soon, it is closed and discarded, otherwise, the timestamp for the | |||
| ip address is updated in the global cache. | |||
| """ | |||
| function check_rate_limit(tcp, rate_limit::Rational{Int}) | |||
| ip = Sockets.getsockname(tcp)[1] | |||
| rate = Float64(rate_limit.num) | |||
There was a problem hiding this comment.
This also fixes the a bug if you specify rate_limit = 3//2 or something (e.g. non-1 denominator).
- use the correct IP adress of the client - use DateTime(0) for first time connections - don't warn when capping the rate limit allowance - fix rate_limit specification with non-1 denominator
fredrikekre
force-pushed
the
fe/rate-limit
branch
from
9e32e0d to
8f915e6
Compare
Bugfixes and improvements to rate limit check
DateTime(0)for first time connections