From b67b0ae83180371d63435e2b4bdea4fe9a85d80b Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Fri, 28 May 2021 08:04:17 +0000 Subject: [PATCH 1/5] Cache: sync if the cache can't be found --- pixels/canvas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixels/canvas.py b/pixels/canvas.py index 5a7e091..30d021c 100644 --- a/pixels/canvas.py +++ b/pixels/canvas.py @@ -55,7 +55,7 @@ async def _populate_cache(self, conn: Connection) -> None: async def is_cache_out_of_date(self, conn: Connection) -> bool: """Return true if the cache can be considered out of date.""" cache = await self.get_pixels() - if len(cache)//3 != constants.width*constants.height: + if not cache or len(cache) // 3 != constants.width * constants.height: # Canvas size has changed, force a cache refresh return True From 74ee28b849f806e956b49ac06b9cb365c01ea81a Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Fri, 28 May 2021 08:04:43 +0000 Subject: [PATCH 2/5] Rate limits: add rolling window logic --- pixels/utils/ratelimits.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pixels/utils/ratelimits.py b/pixels/utils/ratelimits.py index 4c16a44..6f2b82e 100644 --- a/pixels/utils/ratelimits.py +++ b/pixels/utils/ratelimits.py @@ -6,8 +6,10 @@ import inspect import logging import typing +import uuid from collections import namedtuple from dataclasses import dataclass +from time import time import fastapi from aioredis import Redis @@ -249,13 +251,15 @@ async def _record_interaction(self, request_id: int) -> None: key = f"interaction-{self.ROUTE_NAME}-{self.state[request_id].user_id}" log.debug(f"Recorded interaction of user {self.state[request_id].user_id} on {self.ROUTE_NAME}.") - await self.redis.incr(key) + await self.redis.zadd(key, time() + self.LIMITS.time_unit, str(uuid.uuid4())) await self.redis.expire(key, self.LIMITS.time_unit) async def _calculate_remaining_requests(self, request_id: int) -> int: key = f"interaction-{self.ROUTE_NAME}-{self.state[request_id].user_id}" - remaining = self.LIMITS.requests - int(await self.redis.get(key) or 0) + # Cleanup expired entries + await self.redis.zremrangebyscore(key, max=time()) + remaining = self.LIMITS.requests - int(await self.redis.zcount(key) or 0) log.debug(f"Remaining interactions of user {self.state[request_id].user_id} on {self.ROUTE_NAME}: {remaining}.") return remaining From 1f3298caa4c8cc1d9edfca913312ce6b59874480 Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Fri, 28 May 2021 08:16:28 +0000 Subject: [PATCH 3/5] Tech doc: update rate limit to mention rolling window --- pixels/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pixels/README.md b/pixels/README.md index 69c0798..751debd 100644 --- a/pixels/README.md +++ b/pixels/README.md @@ -36,5 +36,5 @@ To avoid having one person fill the entire board, we set up rate limits, leverag One of the main requirements is that the request count must be removed after a set amount of time. Redis [TTL functionality](https://redis.io/commands/TTL) is perfect to automatically remove expired requests. -We insert request counts in Redis as soon as we receive them, set the expiry time to be the reset time, and set dummy values for users under cooldown. -The lack of needing a background clearing task makes it fast and efficient, even if we have to handle a lot of concurrent connections. +Another requirement is to have a rolling window mechanism, so requesting the same endpoint every X seconds or in burst will result in the same speed. With that in mind, a sorted set is created for each bucket. Each entry contains a random dummy value and its score is set to be the current timestamp plus the rate limit duration. +Before counting entries, we simply remove entries from `-inf` to the current timestamp using a [ZREMBYSCORE](https://redis.io/commands/zremrangebyscore) operation, allowing it to stay O(log n). From c159b741065dfd3c168e4eb7cda71c047c4b092f Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Sat, 29 May 2021 17:16:12 +0200 Subject: [PATCH 4/5] Ratelimit: new headers setup New headers: - Requests-Remaining: (int) amount of interactions remaining. - Requests-Limit: (int) maximum amount of interactions that can be done in the same period - Requests-Period: (int) duration, in second, of a period - Requests-Reset: (float) duration, in second, before getting all the interactions back --- pixels/utils/ratelimits.py | 39 ++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/pixels/utils/ratelimits.py b/pixels/utils/ratelimits.py index e39a721..8a9dd73 100644 --- a/pixels/utils/ratelimits.py +++ b/pixels/utils/ratelimits.py @@ -118,15 +118,12 @@ async def head_endpoint(request: requests.Request) -> Response: await self._init_state(request_id, request) if await self._check_cooldown(request_id): - response.headers["CooldownReset"] = str( - await self._get_remaining_cooldown(request_id) + response.headers.append( + "Cooldown-Reset", + str(await self._get_remaining_cooldown(request_id)) ) else: - remaining_requests = await self.get_remaining_requests(request_id) - - response.headers.append("Requests-Remaining", str(remaining_requests)) - response.headers.append("Requests-Limit", str(self.LIMITS.requests)) - response.headers.append("Requests-Reset", str(self.LIMITS.time_unit)) + await self.add_headers(response, request_id) return response # functools.wraps is used here to wrap the endpoint while maintaining the signature @@ -172,11 +169,7 @@ async def caller(*_args, **_kwargs) -> typing.Union[JSONResponse, Response]: clean_result = jsonable_encoder(result) response = JSONResponse(content=clean_result) - remaining_requests = await self.get_remaining_requests(request_id) - - response.headers.append("Requests-Remaining", str(remaining_requests)) - response.headers.append("Requests-Limit", str(self.LIMITS.requests)) - response.headers.append("Requests-Reset", str(self.LIMITS.time_unit)) + await self.add_headers(response, request_id) # Setup post interaction tasks state = self.state[request_id] @@ -195,6 +188,16 @@ async def caller(*_args, **_kwargs) -> typing.Union[JSONResponse, Response]: return caller + async def add_headers(self, response: Response, request_id: int) -> None: + """Add ratelimit headers to the provided request.""" + remaining_requests = await self.get_remaining_requests(request_id) + request_reset = await self._reset_time(request_id) + + response.headers.append("Requests-Remaining", str(remaining_requests)) + response.headers.append("Requests-Limit", str(self.LIMITS.requests)) + response.headers.append("Requests-Period", str(self.LIMITS.time_unit)) + response.headers.append("Requests-Reset", str(request_reset)) + async def _increment(self, request_id: int) -> None: """Reduce remaining quota, and check if a cooldown is needed.""" if await self._check_cooldown(request_id): @@ -241,6 +244,10 @@ async def _get_remaining_cooldown(self, request_id: int) -> int: """Return the time, in seconds, until a cooldown ends.""" raise NotImplementedError() + async def _reset_time(self, request_id: int) -> int: + """Return the time, in seconds, before getting every interaction back.""" + raise NotImplementedError() + class UserRedis(__BucketBase): """A per user request bucket backed by Redis.""" @@ -309,3 +316,11 @@ async def _get_remaining_cooldown(self, request_id: int) -> int: key = f"cooldown-{self.ROUTE_NAME}-{self.state[request_id].user_id}" return await self.redis.ttl(key) + + async def _reset_time(self, request_id: int) -> int: + key = f"interaction-{self.ROUTE_NAME}-{self.state[request_id].user_id}" + + if not (newest_uuid := await self.redis.zrange(key, 0, 0)): + return -1 + + return await self.redis.zscore(key, newest_uuid[0]) - time() From d8e84d3472a39a3b4496ba15276d761dbcf19d00 Mon Sep 17 00:00:00 2001 From: Matteo Bertucci Date: Sat, 29 May 2021 17:26:58 +0200 Subject: [PATCH 5/5] Ratelimit: limit size of Requests-Reset Co-authored-by: Chris --- pixels/utils/ratelimits.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixels/utils/ratelimits.py b/pixels/utils/ratelimits.py index 8a9dd73..283b9ba 100644 --- a/pixels/utils/ratelimits.py +++ b/pixels/utils/ratelimits.py @@ -196,7 +196,7 @@ async def add_headers(self, response: Response, request_id: int) -> None: response.headers.append("Requests-Remaining", str(remaining_requests)) response.headers.append("Requests-Limit", str(self.LIMITS.requests)) response.headers.append("Requests-Period", str(self.LIMITS.time_unit)) - response.headers.append("Requests-Reset", str(request_reset)) + response.headers.append("Requests-Reset", str(request_reset)[:6]) async def _increment(self, request_id: int) -> None: """Reduce remaining quota, and check if a cooldown is needed."""