diff --git a/flickr_api/__init__.py b/flickr_api/__init__.py index ebff92b..d9163b1 100644 --- a/flickr_api/__init__.py +++ b/flickr_api/__init__.py @@ -44,4 +44,5 @@ from .method_call import set_timeout as set_timeout from .keys import set_keys as set_keys from .flickrerrors import FlickrRateLimitError as FlickrRateLimitError +from .flickrerrors import FlickrTimeoutError as FlickrTimeoutError from ._version import __version__ as __version__ diff --git a/flickr_api/flickrerrors.py b/flickr_api/flickrerrors.py index 17d6145..676dd7d 100644 --- a/flickr_api/flickrerrors.py +++ b/flickr_api/flickrerrors.py @@ -101,3 +101,29 @@ def __init__(self, retry_after: float | None, content: str) -> None: FlickrError.__init__(self, msg) self.retry_after = retry_after self.content = content + + +class FlickrTimeoutError(FlickrError): + """Exception for request timeout or connection errors. + + Raised when a request times out or fails due to connection issues + and max retries have been exhausted. + + Parameters: + ----------- + message: str + Error message describing the timeout/connection issue + """ + + message: str + + def __init__(self, message: str) -> None: + """Constructor + + Parameters: + ----------- + message: str + Error message describing the timeout/connection issue + """ + FlickrError.__init__(self, f"Request failed: {message}") + self.message = message diff --git a/flickr_api/method_call.py b/flickr_api/method_call.py index ee224cb..10a13b1 100644 --- a/flickr_api/method_call.py +++ b/flickr_api/method_call.py @@ -20,7 +20,12 @@ from . import keys from .utils import urlopen_and_read -from .flickrerrors import FlickrError, FlickrAPIError, FlickrServerError, FlickrRateLimitError +from .flickrerrors import ( + FlickrAPIError, + FlickrError, + FlickrServerError, +) +from . import retry as retry_module from .cache import SimpleCache REST_URL = "https://api.flickr.com/services/rest/" @@ -41,6 +46,13 @@ _RATE_LIMIT_LAST_REQUEST: float | None = None +# Initialize retry module with our config getters +def _init_retry_module() -> None: + """Initialize retry module with config getters.""" + retry_module.set_retry_config_getter(get_retry_config) + retry_module.set_rate_limit_wait_func(_maybe_wait_for_rate_limit) + + def enable_cache(cache_object: Any | None = None) -> None: """enable caching Parameters: @@ -208,12 +220,8 @@ def _calculate_retry_delay(attempt: int, retry_after: float | None) -> float: -------- Delay in seconds """ - if retry_after is not None and retry_after > 0: - return min(retry_after, RETRY_MAX_DELAY) - - # Exponential backoff: base_delay * 2^attempt - delay = RETRY_BASE_DELAY * (2**attempt) - return min(delay, RETRY_MAX_DELAY) + # Delegate to retry module for consistency + return retry_module.calculate_retry_delay(attempt, retry_after) def _parse_retry_after(response: requests.Response) -> float | None: @@ -228,16 +236,8 @@ def _parse_retry_after(response: requests.Response) -> float | None: -------- Seconds to wait, or None if header not present/parseable """ - retry_after = response.headers.get("Retry-After") - if retry_after is None: - return None - - try: - return float(retry_after) - except ValueError: - # Could be an HTTP-date format, but Flickr typically uses seconds - logger.warning("Could not parse Retry-After header: %s", retry_after) - return None + # Delegate to retry module for consistency + return retry_module.parse_retry_after(response) def _make_request_with_retry( @@ -245,7 +245,10 @@ def _make_request_with_retry( args: dict[str, Any], oauth_auth: Any, ) -> requests.Response: - """Make HTTP request with automatic retry on rate limit errors. + """Make HTTP request with automatic retry on transient errors. + + Handles HTTP 429 (rate limit), 5xx (server errors), timeouts, and + connection errors with configurable retry behavior. Parameters: ----------- @@ -263,40 +266,16 @@ def _make_request_with_retry( Raises: ------- FlickrRateLimitError: If rate limit exceeded and max retries exhausted + FlickrServerError: If server error and max retries exhausted + FlickrTimeoutError: If timeout/connection error and max retries exhausted """ - _maybe_wait_for_rate_limit() - - last_error: FlickrRateLimitError | None = None - - for attempt in range(MAX_RETRIES + 1): - resp = requests.post(request_url, args, auth=oauth_auth, timeout=get_timeout()) - - if resp.status_code != 429: - return resp - - # Rate limited - parse retry info and potentially retry - retry_after = _parse_retry_after(resp) - content = resp.content.decode("utf8") if resp.content else "Too Many Requests" - last_error = FlickrRateLimitError(retry_after, content) - - if attempt >= MAX_RETRIES: - logger.warning( - "Rate limit exceeded, max retries (%d) exhausted", - MAX_RETRIES, - ) - break - - delay = _calculate_retry_delay(attempt, retry_after) - logger.warning( - "Rate limit exceeded (attempt %d/%d), retrying in %.1f seconds", - attempt + 1, - MAX_RETRIES + 1, - delay, - ) - time.sleep(delay) - - # If we get here, we've exhausted retries - raise last_error # type: ignore[misc] + # Ensure retry module is initialized + _init_retry_module() + + def make_request() -> requests.Response: + return requests.post(request_url, args, auth=oauth_auth, timeout=get_timeout()) + + return retry_module.retry_request(make_request, operation_name="API call") def send_request(url, data): diff --git a/flickr_api/retry.py b/flickr_api/retry.py new file mode 100644 index 0000000..d884e8e --- /dev/null +++ b/flickr_api/retry.py @@ -0,0 +1,251 @@ +""" +Retry utilities for handling transient errors. + +This module provides reusable retry logic for HTTP requests, handling: +- HTTP 429 (rate limit) with Retry-After header support +- HTTP 5xx (server errors) +- ReadTimeout exceptions +- ConnectionError exceptions + +Author: python-flickr-api contributors +""" + +import logging +import time +from typing import Any, Callable, TypeVar + +import requests + +from .flickrerrors import FlickrRateLimitError, FlickrServerError, FlickrTimeoutError + +logger = logging.getLogger(__name__) + +T = TypeVar("T") + +# These are imported from method_call to share configuration +# We use a getter pattern to avoid circular imports +_get_retry_config: Callable[[], dict[str, Any]] | None = None +_get_rate_limit_wait: Callable[[], None] | None = None + + +def set_retry_config_getter(getter: Callable[[], dict[str, Any]]) -> None: + """Set the function to get retry configuration. + + This is called during module initialization to avoid circular imports. + """ + global _get_retry_config + _get_retry_config = getter + + +def set_rate_limit_wait_func(func: Callable[[], None]) -> None: + """Set the function to wait for rate limits. + + This is called during module initialization to avoid circular imports. + """ + global _get_rate_limit_wait + _get_rate_limit_wait = func + + +def _get_config() -> dict[str, Any]: + """Get current retry configuration.""" + if _get_retry_config is None: + # Default config if not initialized + return {"max_retries": 3, "base_delay": 1.0, "max_delay": 60.0} + return _get_retry_config() + + +def calculate_retry_delay(attempt: int, retry_after: float | None) -> float: + """Calculate delay before next retry. + + Uses Retry-After header if available, otherwise exponential backoff. + + Parameters: + ----------- + attempt: int + Current retry attempt number (0-indexed) + retry_after: float | None + Value from Retry-After header, if present + + Returns: + -------- + Delay in seconds + """ + config = _get_config() + max_delay = config["max_delay"] + base_delay = config["base_delay"] + + if retry_after is not None and retry_after > 0: + return min(retry_after, max_delay) + + # Exponential backoff: base_delay * 2^attempt + delay = base_delay * (2**attempt) + return min(delay, max_delay) + + +def parse_retry_after(response: requests.Response) -> float | None: + """Parse Retry-After header from response. + + Parameters: + ----------- + response: requests.Response + The HTTP response + + Returns: + -------- + Seconds to wait, or None if header not present/parseable + """ + retry_after = response.headers.get("Retry-After") + if retry_after is None: + return None + + try: + return float(retry_after) + except ValueError: + # Could be an HTTP-date format, but Flickr typically uses seconds + logger.warning("Could not parse Retry-After header: %s", retry_after) + return None + + +def retry_request( + make_request: Callable[[], requests.Response], + operation_name: str = "request", +) -> requests.Response: + """Execute a request with automatic retry on transient errors. + + Handles: + - HTTP 429 (rate limit) with Retry-After header support + - HTTP 5xx (server errors) + - requests.exceptions.ReadTimeout + - requests.exceptions.ConnectionError + + Parameters: + ----------- + make_request: Callable[[], requests.Response] + A function that makes the HTTP request and returns a Response + operation_name: str + Name of the operation for logging purposes + + Returns: + -------- + requests.Response + + Raises: + ------- + FlickrRateLimitError: If rate limit exceeded and max retries exhausted + FlickrServerError: If server error and max retries exhausted + FlickrTimeoutError: If timeout/connection error and max retries exhausted + """ + # Apply proactive rate limiting before first attempt + if _get_rate_limit_wait is not None: + _get_rate_limit_wait() + + config = _get_config() + max_retries = config["max_retries"] + + last_exception: Exception | None = None + + for attempt in range(max_retries + 1): + try: + resp = make_request() + + # Check for retryable HTTP status codes + if resp.status_code == 429: + # Rate limited + retry_after = parse_retry_after(resp) + content = resp.content.decode("utf8") if resp.content else "Too Many Requests" + last_exception = FlickrRateLimitError(retry_after, content) + + if attempt >= max_retries: + logger.warning( + "%s: Rate limit exceeded, max retries (%d) exhausted", + operation_name, + max_retries, + ) + raise last_exception + + delay = calculate_retry_delay(attempt, retry_after) + logger.warning( + "%s: Rate limit exceeded (attempt %d/%d), retrying in %.1f seconds", + operation_name, + attempt + 1, + max_retries + 1, + delay, + ) + time.sleep(delay) + continue + + if 500 <= resp.status_code < 600: + # Server error + content = resp.content.decode("utf8") if resp.content else "Server Error" + last_exception = FlickrServerError(resp.status_code, content) + + if attempt >= max_retries: + logger.warning( + "%s: Server error %d, max retries (%d) exhausted", + operation_name, + resp.status_code, + max_retries, + ) + raise last_exception + + delay = calculate_retry_delay(attempt, None) + logger.warning( + "%s: Server error %d (attempt %d/%d), retrying in %.1f seconds", + operation_name, + resp.status_code, + attempt + 1, + max_retries + 1, + delay, + ) + time.sleep(delay) + continue + + # Success or non-retryable error + return resp + + except (requests.exceptions.ReadTimeout, requests.exceptions.Timeout) as e: + last_exception = FlickrTimeoutError(str(e)) + + if attempt >= max_retries: + logger.warning( + "%s: Timeout, max retries (%d) exhausted", + operation_name, + max_retries, + ) + raise last_exception from e + + delay = calculate_retry_delay(attempt, None) + logger.warning( + "%s: Timeout (attempt %d/%d), retrying in %.1f seconds", + operation_name, + attempt + 1, + max_retries + 1, + delay, + ) + time.sleep(delay) + + except requests.exceptions.ConnectionError as e: + last_exception = FlickrTimeoutError(f"Connection error: {e}") + + if attempt >= max_retries: + logger.warning( + "%s: Connection error, max retries (%d) exhausted", + operation_name, + max_retries, + ) + raise last_exception from e + + delay = calculate_retry_delay(attempt, None) + logger.warning( + "%s: Connection error (attempt %d/%d), retrying in %.1f seconds", + operation_name, + attempt + 1, + max_retries + 1, + delay, + ) + time.sleep(delay) + + # Should not reach here, but just in case + if last_exception is not None: + raise last_exception + raise FlickrTimeoutError("Unknown error during retry") diff --git a/flickr_api/upload.py b/flickr_api/upload.py index ead5a89..3c6b05a 100644 --- a/flickr_api/upload.py +++ b/flickr_api/upload.py @@ -18,7 +18,8 @@ from .flickrerrors import FlickrError, FlickrAPIError from .objects import Photo, UploadTicket -from .method_call import get_timeout +from .method_call import get_timeout, get_retry_config, _maybe_wait_for_rate_limit +from . import retry as retry_module from . import auth import os from xml.etree import ElementTree as ET @@ -119,7 +120,14 @@ def percent_encode(s): files = {"photo": (os.path.basename(photo_file), photo_file_data.read())} - resp = requests.post(url, data=form_data, files=files, timeout=get_timeout()) + # Initialize retry module with config getters + retry_module.set_retry_config_getter(get_retry_config) + retry_module.set_rate_limit_wait_func(_maybe_wait_for_rate_limit) + + def make_request(): + return requests.post(url, data=form_data, files=files, timeout=get_timeout()) + + resp = retry_module.retry_request(make_request, operation_name="upload") data = resp.content if resp.status_code != 200: diff --git a/test/test_retry.py b/test/test_retry.py new file mode 100644 index 0000000..099a129 --- /dev/null +++ b/test/test_retry.py @@ -0,0 +1,301 @@ +"""Tests for shared retry logic in retry module.""" + +import sys +import unittest +from unittest.mock import patch + +from requests import Response +from requests.exceptions import ConnectionError, ReadTimeout + +import flickr_api as f +import flickr_api.upload # noqa: F401 - ensure module is loaded +from flickr_api import method_call +from flickr_api import retry as retry_module +from flickr_api.auth import AuthHandler +from flickr_api.flickrerrors import ( + FlickrServerError, + FlickrTimeoutError, +) + +upload_module = sys.modules["flickr_api.upload"] + + +class TestRetryOnTimeout(unittest.TestCase): + """Tests for retry on timeout exceptions.""" + + def setUp(self): + """Set up test fixtures.""" + auth_handler = AuthHandler( + key="test", + secret="test", + access_token_key="test", + access_token_secret="test", + ) + f.set_auth_handler(auth_handler) + self.original_config = method_call.get_retry_config() + method_call.set_retry_config(max_retries=2, base_delay=0.01, max_delay=0.1) + + def tearDown(self): + """Restore original config values.""" + method_call.set_retry_config( + max_retries=self.original_config["max_retries"], + base_delay=self.original_config["base_delay"], + max_delay=self.original_config["max_delay"], + ) + + def _create_ok_response(self): + """Create a mock successful response.""" + resp = Response() + resp.status_code = 200 + resp._content = b'{"stat": "ok", "user": {"id": "123", "username": {"_content": "testuser"}}}' + return resp + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_retries_on_read_timeout(self, mock_sleep, mock_post): + """Retries on ReadTimeout and succeeds.""" + # First two calls timeout, third succeeds + mock_post.side_effect = [ + ReadTimeout("Read timed out"), + ReadTimeout("Read timed out"), + self._create_ok_response(), + ] + + # Should not raise - succeeds on third attempt + f.Person.findByUserName("testuser") + + self.assertEqual(3, mock_post.call_count) + self.assertEqual(2, mock_sleep.call_count) + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_raises_timeout_error_after_max_retries(self, mock_sleep, mock_post): + """Raises FlickrTimeoutError after max retries exhausted.""" + mock_post.side_effect = ReadTimeout("Read timed out") + + with self.assertRaises(FlickrTimeoutError) as context: + f.Person.findByUserName("testuser") + + self.assertIn("Read timed out", str(context.exception)) + # Initial attempt + 2 retries = 3 calls + self.assertEqual(3, mock_post.call_count) + + +class TestRetryOnConnectionError(unittest.TestCase): + """Tests for retry on connection errors.""" + + def setUp(self): + """Set up test fixtures.""" + auth_handler = AuthHandler( + key="test", + secret="test", + access_token_key="test", + access_token_secret="test", + ) + f.set_auth_handler(auth_handler) + self.original_config = method_call.get_retry_config() + method_call.set_retry_config(max_retries=2, base_delay=0.01, max_delay=0.1) + + def tearDown(self): + """Restore original config values.""" + method_call.set_retry_config( + max_retries=self.original_config["max_retries"], + base_delay=self.original_config["base_delay"], + max_delay=self.original_config["max_delay"], + ) + + def _create_ok_response(self): + """Create a mock successful response.""" + resp = Response() + resp.status_code = 200 + resp._content = b'{"stat": "ok", "user": {"id": "123", "username": {"_content": "testuser"}}}' + return resp + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_retries_on_connection_error(self, mock_sleep, mock_post): + """Retries on ConnectionError and succeeds.""" + # First call fails with connection error, second succeeds + mock_post.side_effect = [ + ConnectionError("Connection refused"), + self._create_ok_response(), + ] + + # Should not raise + f.Person.findByUserName("testuser") + + self.assertEqual(2, mock_post.call_count) + self.assertEqual(1, mock_sleep.call_count) + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_raises_timeout_error_on_connection_exhausted(self, mock_sleep, mock_post): + """Raises FlickrTimeoutError after connection retries exhausted.""" + mock_post.side_effect = ConnectionError("Connection refused") + + with self.assertRaises(FlickrTimeoutError) as context: + f.Person.findByUserName("testuser") + + self.assertIn("Connection error", str(context.exception)) + + +class TestRetryOnServerError(unittest.TestCase): + """Tests for retry on HTTP 5xx server errors.""" + + def setUp(self): + """Set up test fixtures.""" + auth_handler = AuthHandler( + key="test", + secret="test", + access_token_key="test", + access_token_secret="test", + ) + f.set_auth_handler(auth_handler) + self.original_config = method_call.get_retry_config() + method_call.set_retry_config(max_retries=2, base_delay=0.01, max_delay=0.1) + + def tearDown(self): + """Restore original config values.""" + method_call.set_retry_config( + max_retries=self.original_config["max_retries"], + base_delay=self.original_config["base_delay"], + max_delay=self.original_config["max_delay"], + ) + + def _create_ok_response(self): + """Create a mock successful response.""" + resp = Response() + resp.status_code = 200 + resp._content = b'{"stat": "ok", "user": {"id": "123", "username": {"_content": "testuser"}}}' + return resp + + def _create_5xx_response(self, status_code=500): + """Create a mock 5xx response.""" + resp = Response() + resp.status_code = status_code + resp._content = b"Internal Server Error" + return resp + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_retries_on_500_error(self, mock_sleep, mock_post): + """Retries on HTTP 500 and succeeds.""" + mock_post.side_effect = [ + self._create_5xx_response(500), + self._create_ok_response(), + ] + + f.Person.findByUserName("testuser") + + self.assertEqual(2, mock_post.call_count) + self.assertEqual(1, mock_sleep.call_count) + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_retries_on_502_error(self, mock_sleep, mock_post): + """Retries on HTTP 502 Bad Gateway.""" + mock_post.side_effect = [ + self._create_5xx_response(502), + self._create_ok_response(), + ] + + f.Person.findByUserName("testuser") + self.assertEqual(2, mock_post.call_count) + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_retries_on_503_error(self, mock_sleep, mock_post): + """Retries on HTTP 503 Service Unavailable.""" + mock_post.side_effect = [ + self._create_5xx_response(503), + self._create_ok_response(), + ] + + f.Person.findByUserName("testuser") + self.assertEqual(2, mock_post.call_count) + + @patch.object(method_call.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_raises_server_error_after_max_retries(self, mock_sleep, mock_post): + """Raises FlickrServerError after max retries exhausted.""" + mock_post.return_value = self._create_5xx_response(500) + + with self.assertRaises(FlickrServerError) as context: + f.Person.findByUserName("testuser") + + self.assertEqual(500, context.exception.status_code) + + +class TestUploadRetry(unittest.TestCase): + """Tests for upload retry behavior.""" + + def setUp(self): + """Set up test fixtures.""" + auth_handler = AuthHandler( + key="test", + secret="test", + access_token_key="test", + access_token_secret="test", + ) + f.set_auth_handler(auth_handler) + self.original_config = method_call.get_retry_config() + method_call.set_retry_config(max_retries=2, base_delay=0.01, max_delay=0.1) + + def tearDown(self): + """Restore original config values.""" + method_call.set_retry_config( + max_retries=self.original_config["max_retries"], + base_delay=self.original_config["base_delay"], + max_delay=self.original_config["max_delay"], + ) + + def _create_ok_upload_response(self): + """Create a mock successful upload response.""" + resp = Response() + resp.status_code = 200 + resp._content = b'12345' + return resp + + @patch.object(upload_module.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_upload_retries_on_timeout(self, mock_sleep, mock_post): + """Upload retries on ReadTimeout.""" + from flickr_api import upload + from io import StringIO + + mock_post.side_effect = [ + ReadTimeout("Read timed out"), + self._create_ok_upload_response(), + ] + + result = upload(photo_file="/tmp/test.jpg", photo_file_data=StringIO("test")) + + self.assertEqual(2, mock_post.call_count) + self.assertEqual("12345", result.id) + + @patch.object(upload_module.requests, "post") + @patch.object(retry_module.time, "sleep") + def test_upload_raises_timeout_error_after_max_retries(self, mock_sleep, mock_post): + """Upload raises FlickrTimeoutError after max retries.""" + from flickr_api import upload + from io import StringIO + + mock_post.side_effect = ReadTimeout("Read timed out") + + with self.assertRaises(FlickrTimeoutError): + upload(photo_file="/tmp/test.jpg", photo_file_data=StringIO("test")) + + +class TestFlickrTimeoutError(unittest.TestCase): + """Tests for FlickrTimeoutError exception.""" + + def test_error_message(self): + """Error message is properly formatted.""" + error = FlickrTimeoutError("Read timed out after 10 seconds") + self.assertEqual("Read timed out after 10 seconds", error.message) + self.assertIn("Request failed", str(error)) + self.assertIn("Read timed out", str(error)) + + +if __name__ == "__main__": + unittest.main()