Add timeout to all requests #50
Conversation
WalkthroughThe changes involve the introduction of a new constant 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
trakt/core.py (2)
75-76: Consider making the timeout configurableWhile 30 seconds is a reasonable default timeout, it would be better to make this configurable to accommodate different network conditions and use cases.
Consider this implementation:
-#: Timeout in seconds for all requests -TIMEOUT = 30 +#: Timeout in seconds for all requests (default: 30) +TIMEOUT = int(os.getenv('PYTRAKT_TIMEOUT', '30'))
Line range hint
553-563: Enhance error handling for timeout scenariosThe current error handling doesn't specifically handle request timeouts. Consider adding explicit handling for
requests.exceptions.Timeoutto provide clearer feedback to users.Here's a suggested implementation:
def _handle_request(self, method, url, data=None): try: if method == 'get': response = session.request(method, url, headers=HEADERS, params=data, timeout=TIMEOUT) else: response = session.request(method, url, headers=HEADERS, data=json.dumps(data), timeout=TIMEOUT) + except requests.exceptions.Timeout: + self.logger.error('Request timed out after %s seconds: %s %s', + TIMEOUT, method, url) + raise errors.TraktTimeout(f"Request timed out after {TIMEOUT} seconds") self.logger.debug('RESPONSE [%s] (%s): %s', method, url, str(response)) if response.status_code in self.error_map: raise self.error_map[response.status_code](response)You'll also need to add a new exception class in
errors.py:class TraktTimeout(TraktException): """Request timed out""" http_code = 408 # Request Timeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
trakt/core.py(12 hunks)
🔇 Additional comments (2)
trakt/core.py (2)
147-147: Well-implemented timeout across all request methods
The timeout parameter has been consistently applied to all HTTP requests, including authentication, device code generation, token refresh, and general API calls. This implementation effectively prevents requests from hanging indefinitely.
Also applies to: 237-237, 278-278, 421-421, 548-548, 551-551
548-551: Verify complete coverage of request timeouts
Let's verify that all HTTP requests in the codebase are routed through _handle_request and thus have the timeout applied.
✅ Verification successful
Let me gather more information to verify if all requests are properly routed through the core request handling mechanism.
Let me do one final check to understand the class structure and request routing.
All request timeouts are properly implemented
Based on the verification results:
- All HTTP methods (GET, POST, PUT, DELETE) are implemented as decorators that route through
_handle_request - The only direct session calls are for authentication-related endpoints, and they all have the timeout parameter properly set
- There are no other request calls that could bypass the timeout implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct requests.Session or requests calls that might bypass _handle_request
rg -l "requests\.(get|post|put|delete|request)" | grep -v "core\.py"
# Search for potential timeout parameter usage elsewhere
rg "timeout\s*="
Length of output: 656
Script:
#!/bin/bash
# Search for all HTTP-related method definitions and usages in core.py
ast-grep --pattern 'def _handle_request'
# Look for any direct session usage
rg "session\." -A 2
# Check for any request methods defined in the class
ast-grep --pattern 'def (get|post|put|delete|request)'
Length of output: 1438
Script:
#!/bin/bash
# Check the class structure and public methods
ast-grep --pattern 'class $_ {
$$$
}'
# Look for any method that might be calling these requests
rg "def" -A 1 trakt/core.py
Length of output: 2772
|
formatting changes and code changes must be separate commits, even better separate pull request |
Since the default requests timeout is infinite individual calls might hang for a very long time if something in the network breaks. See also Taxel/PlexTraktSync#2107.