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.

Sign up for GitHub

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

Add timeout to all requests #50

Merged
merged 2 commits into from
Jan 2, 2025
Merged

Add timeout to all requests #50

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c7ae84
Add timeout to all requests
U1F984 Dec 11, 2024
8530cb9
Revert unwanted formatting changes
glensc Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions trakt/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@
#: The ID of the application to register with, when using PIN authentication
APPLICATION_ID = None

#: Timeout in seconds for all requests
TIMEOUT = 30

#: Global session to make requests with
session = requests.Session()

Expand Down Expand Up @@ -141,7 +144,7 @@ def pin_auth(pin=None, client_id=None, client_secret=None, store=False):
'client_id': CLIENT_ID,
'client_secret': CLIENT_SECRET}

response = session.post(''.join([BASE_URL, '/oauth/token']), data=args)
response = session.post(''.join([BASE_URL, '/oauth/token']), data=args, timeout=TIMEOUT)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add timeout exception handling for authentication requests

While adding timeouts is good, the code should explicitly handle requests.exceptions.Timeout exceptions for these critical authentication operations. A timeout during authentication should provide clear feedback to the user.

Example implementation for the pin_auth function:

+from requests.exceptions import Timeout
+
 def pin_auth(pin=None, client_id=None, client_secret=None, store=False):
     # ... existing code ...
-    response = session.post(''.join([BASE_URL, '/oauth/token']), data=args, timeout=TIMEOUT)
+    try:
+        response = session.post(''.join([BASE_URL, '/oauth/token']), 
+                              data=args, timeout=TIMEOUT)
+    except Timeout:
+        raise errors.TraktException(
+            "Authentication timed out after {} seconds. Please try again."
+            .format(TIMEOUT)
+        )

Similar error handling should be added to:

  • get_device_code
  • get_device_token
  • _refresh_token

Also applies to: 237-237, 278-278, 421-421

OAUTH_TOKEN = response.json().get('access_token', None)

if store:
Expand Down Expand Up @@ -231,7 +234,7 @@ def get_device_code(client_id=None, client_secret=None):
data = {"client_id": CLIENT_ID}

device_response = session.post(device_code_url,
json=data, headers=headers).json()
json=data, headers=headers, timeout=TIMEOUT).json()
print('Your user code is: {user_code}, please navigate to '
'{verification_url} to authenticate'.format(
user_code=device_response.get('user_code'),
Expand Down Expand Up @@ -272,7 +275,7 @@ def get_device_token(device_code, client_id=None, client_secret=None,
}

response = session.post(
urljoin(BASE_URL, '/oauth/device/token'), json=data
urljoin(BASE_URL, '/oauth/device/token'), json=data, timeout=TIMEOUT
)

# We only get json on success.
Expand Down Expand Up @@ -415,7 +418,7 @@ def _refresh_token(s):
'redirect_uri': REDIRECT_URI,
'grant_type': 'refresh_token'
}
response = session.post(url, json=data, headers=HEADERS)
response = session.post(url, json=data, headers=HEADERS, timeout=TIMEOUT)
s.logger.debug('RESPONSE [post] (%s): %s - %s', url, str(response), response.content)
if response.status_code == 200:
data = response.json()
Expand Down Expand Up @@ -542,10 +545,10 @@ def _handle_request(self, method, url, data=None):
self.logger.debug('method, url :: %s, %s', method, url)
if method == 'get': # GETs need to pass data as params, not body
response = session.request(method, url, headers=HEADERS,
params=data)
params=data, timeout=TIMEOUT)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling and documentation in _handle_request

The timeout parameter has been added, but there are two improvements needed:

  1. Add explicit handling for timeout exceptions
  2. Document the timeout behavior in the method's docstring

Apply these changes to improve error handling and documentation:

 def _handle_request(self, method, url, data=None):
     """Handle actually talking out to the trakt API, logging out debug
     information, raising any relevant `TraktException` Exception types,
     and extracting and returning JSON data

     :param method: The HTTP method we're executing on. Will be one of
         post, put, delete, get
     :param url: The fully qualified url to send our request to
     :param data: Optional data payload to send to the API
+    :raises TraktTimeout: If the request times out after TIMEOUT seconds
     :raises TraktException: If any non-200 return code is encountered
     :return: The decoded JSON response from the Trakt API
     """
     self.logger.debug('%s: %s', method, url)
     HEADERS['trakt-api-key'] = CLIENT_ID
     HEADERS['Authorization'] = 'Bearer {0}'.format(OAUTH_TOKEN)
     self.logger.debug('method, url :: %s, %s', method, url)
+    try:
         if method == 'get':  # GETs need to pass data as params, not body
             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 to %s timed out after %s seconds', url, TIMEOUT)
+        raise errors.TraktTimeout(f"Request timed out after {TIMEOUT} seconds")

Also, add the new exception class to errors.py:

class TraktTimeout(TraktException):
    """Request timed out"""
    http_code = 0  # Not an HTTP error

Also applies to: 551-551

else:
response = session.request(method, url, headers=HEADERS,
data=json.dumps(data))
data=json.dumps(data), timeout=TIMEOUT)
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)
Expand Down