Added rate limit protection #254
Conversation
Reviewer's Guide by SourceryThis PR implements rate limit tracking and protection in the Twitter API client. The implementation adds a dictionary to store rate limit information per URL endpoint and introduces methods to check and update rate limits based on response headers. The rate limit check is integrated into the main request flow to prevent exceeding API limits. Class diagram for rate limit tracking in Twitter API clientclassDiagram
class Client {
+dict rate_limits
+async rate_limit_check(url) bool
+async rate_limit_update(url, response: Response) void
+async request(method, url, **kwargs) tuple[dict | Any, Response]
}
note for Client "Added rate limit tracking and protection methods"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe changes introduce a rate limiting mechanism to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @g4cko - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider documenting the default rate limit values (50 requests, 900 seconds) and their source/reasoning
- The empty try-except block in rate_limit_update silently swallows all exceptions, which could hide important errors. Consider logging the exception or handling specific exception types
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -113,6 +115,34 @@ def __init__( | |||
|
|
|||
| self.gql = GQLClient(self) | |||
| self.v11 = V11Client(self) | |||
| self.rate_limits = {} | |||
There was a problem hiding this comment.
issue (bug_risk): The rate_limits dictionary access should be protected against race conditions in async context
Consider using asyncio.Lock to synchronize access to self.rate_limits to prevent potential race conditions when multiple coroutines access it simultaneously.
| self.rate_limits[url]["remaining"] = int(response.headers["x-rate-limit-remaining"]) | ||
| if "x-rate-limit-reset" in response.headers: | ||
| self.rate_limits[url]["reset"] = float(response.headers["x-rate-limit-reset"]) | ||
| except Exception: |
There was a problem hiding this comment.
issue (bug_risk): Avoid silently catching all exceptions in rate limit handling
Consider logging the exception or handling specific exception types to avoid masking potential issues with rate limit tracking.
| self.rate_limits = {} | ||
|
|
||
|
|
||
| async def rate_limit_check(self, url) -> bool: |
There was a problem hiding this comment.
issue (complexity): Consider using a context manager pattern to handle rate limiting logic
The rate limiting logic can be simplified using a context manager pattern. This will:
- Reduce nesting in the request method
- Consolidate rate limit handling in one place
- Make error handling explicit
async def _handle_rate_limits(self, url: str) -> AsyncGenerator[None, Response]:
if url not in self.rate_limits:
self.rate_limits[url] = {"reset": time.time() + 900, "rate_limit_max": 50, "remaining": 50}
elif time.time() > self.rate_limits[url]["reset"]:
self.rate_limits[url]["remaining"] = self.rate_limits[url]["rate_limit_max"]
elif self.rate_limits[url]["remaining"] <= 0:
raise TooManyRequests(f"Rate limit exceeded, retry after {self.rate_limits[url]['reset'] - time.time():.1f} seconds")
try:
yield
finally:
if response := yield:
limits = self.rate_limits[url]
limits["rate_limit_max"] = int(response.headers.get("x-rate-limit-limit", limits["rate_limit_max"]))
limits["remaining"] = int(response.headers.get("x-rate-limit-remaining", limits["remaining"]))
limits["reset"] = float(response.headers.get("x-rate-limit-reset", limits["reset"]))
async def request(self, method: str, url: str, auto_unlock: bool = True, raise_exception: bool = True, **kwargs):
cookies_backup = self.get_cookies().copy()
async with self._handle_rate_limits(url):
response = await self.http.request(method, url, **kwargs)
self._remove_duplicate_ct0_cookie()
# ... rest of error handling code ...This approach:
- Combines rate limit checking and updating into a single method
- Handles rate limit headers safely without silent exception handling
- Reduces nesting in the request method
- Makes the control flow easier to follow
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
twikit/client/client.py (5)
123-132: Refactor: Simplify therate_limit_checkmethodThe
if-elif-elsestructure in therate_limit_checkmethod can be simplified by combining conditions using logical operators and returning the condition directly. This enhances code readability and conciseness.Apply this diff to simplify the method:
def rate_limit_check(self, url) -> bool: if url not in self.rate_limits: self.rate_limits[url] = { "reset": time.time() + 900, "rate_limit_max": 50, "remaining": 50 } return True - elif time.time() > self.rate_limits[url]["reset"]: - self.rate_limits[url]["remaining"] = self.rate_limits[url]["rate_limit_max"] - return True - elif self.rate_limits[url]["remaining"] > 0: - return True - else: - return False + if time.time() > self.rate_limits[url]["reset"]: + self.rate_limits[url]["remaining"] = self.rate_limits[url]["rate_limit_max"] + return self.rate_limits[url]["remaining"] > 0🧰 Tools
🪛 Ruff
126-129: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
128-132: Return the condition
self.rate_limits[url]['remaining'] > 0directlyReplace with
return self.rate_limits[url]['remaining'] > 0(SIM103)
126-129: Refactor: Combineifbranches using logicaloroperatorAs suggested by the static analysis hint (SIM114), you can combine the
ifbranches to simplify the code.Apply this diff:
- elif time.time() > self.rate_limits[url]["reset"]: - self.rate_limits[url]["remaining"] = self.rate_limits[url]["rate_limit_max"] - return True - elif self.rate_limits[url]["remaining"] > 0: + elif time.time() > self.rate_limits[url]["reset"] or self.rate_limits[url]["remaining"] > 0: return True🧰 Tools
🪛 Ruff
126-129: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
128-132: Refactor: Return the condition directlyAccording to static analysis hint (SIM103), you can return the condition directly to make the code more concise.
Apply this diff:
- elif self.rate_limits[url]["remaining"] > 0: - return True - else: - return False + return self.rate_limits[url]["remaining"] > 0🧰 Tools
🪛 Ruff
128-132: Return the condition
self.rate_limits[url]['remaining'] > 0directlyReplace with
return self.rate_limits[url]['remaining'] > 0(SIM103)
136-144: Refactor: Avoid broadly catchingExceptionwithout handlingIn the
rate_limit_updatemethod, catching all exceptions withexcept Exception:and then usingpasscan suppress unexpected errors, making debugging difficult. It's better to catch specific exceptions or log the exception details.Apply this diff to handle exceptions appropriately:
try: if "x-rate-limit-limit" in response.headers: self.rate_limits[url]["rate_limit_max"] = int(response.headers["x-rate-limit-limit"]) if "x-rate-limit-remaining" in response.headers: self.rate_limits[url]["remaining"] = int(response.headers["x-rate-limit-remaining"]) if "x-rate-limit-reset" in response.headers: self.rate_limits[url]["reset"] = float(response.headers["x-rate-limit-reset"]) -except Exception: - pass +except KeyError as e: + # Handle missing header keys + print(f"Missing expected header in response: {e}") +except ValueError as e: + # Handle invalid header values + print(f"Invalid header value: {e}")
135-135: Nitpick: Remove empty commentThere's an empty comment at line 135 that doesn't serve any purpose. Removing it can clean up the code.
Apply this diff:
- #
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
twikit/__pycache__/__init__.cpython-313.pycis excluded by!**/*.pyc
📒 Files selected for processing (1)
twikit/client/client.py(5 hunks)
🧰 Additional context used
🪛 Ruff
twikit/client/client.py
126-129: Combine if branches using logical or operator
Combine if branches
(SIM114)
128-132: Return the condition self.rate_limits[url]['remaining'] > 0 directly
Replace with return self.rate_limits[url]['remaining'] > 0
(SIM103)
| elif time.time() > self.rate_limits[url]["reset"]: | ||
| return True | ||
| elif self.rate_limits[url]["remaining"] > 0: | ||
| return True | ||
| else: | ||
|
|
||
| return False |
There was a problem hiding this comment.
Critical Issue: Reset rate limit data when reset time has passed
In the rate_limit_check method, when the current time exceeds the reset time (time.time() > self.rate_limits[url]["reset"]), the remaining count is not reset. This means that requests might be incorrectly blocked even after the reset time has passed because remaining may still be zero. To fix this, reset the rate limit data when the reset time has passed.
Apply this diff to reset the rate limit data:
elif time.time() > self.rate_limits[url]["reset"]:
+ self.rate_limits[url]["remaining"] = self.rate_limits[url]["rate_limit_max"]
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elif time.time() > self.rate_limits[url]["reset"]: | |
| return True | |
| elif self.rate_limits[url]["remaining"] > 0: | |
| return True | |
| else: | |
| return False | |
| elif time.time() > self.rate_limits[url]["reset"]: | |
| self.rate_limits[url]["remaining"] = self.rate_limits[url]["rate_limit_max"] | |
| return True | |
| elif self.rate_limits[url]["remaining"] > 0: | |
| return True | |
| else: | |
| return False |
🧰 Tools
🪛 Ruff
126-129: Combine if branches using logical or operator
Combine if branches
(SIM114)
128-132: Return the condition self.rate_limits[url]['remaining'] > 0 directly
Replace with return self.rate_limits[url]['remaining'] > 0
(SIM103)
Added method to keep track of rate limits (returned via headers) for each method
Summary by Sourcery
Implement rate limit protection by adding methods to track and update rate limits for API requests, ensuring compliance with rate limits and preventing excessive requests.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes