Add configurable retry behavior with reasonable defaults for count, wait, backoff, and jitter

This makes retry behavior consistent between sync and async clients, and
in its default configuration will get users past network interruptions.

Author: mawelborn

indico/config/config.py

@@ -22,6 +22,10 @@ class IndicoConfig:
         api_token= (str, optional): The actual text of the API Token. Takes precedence over api_token_path
         verify_ssl= (bool, optional): Whether to verify the host's SSL certificate. Default=True
         requests_params= (dict, optional): Dictionary of requests. Session parameters to set
+        retry_count= (int, optional): Retry API calls this many times.
+        retry_wait= (float, optional): Wait this many seconds after the first error before retrying.
+        retry_backoff= (float, optional): Multiply the wait time by this amount for each additional error.
+        retry_jitter= (float, optional): Add a random amount of time (up to this percent as a decimal) to the wait time to prevent simultaneous retries.
 
     Returns:
         IndicoConfig object
@@ -42,6 +46,11 @@ def __init__(self, **kwargs: "Any"):
         self.requests_params: "Optional[AnyDict]" = None
         self._disable_cookie_domain: bool = False
 
+        self.retry_count: int = int(os.getenv("INDICO_retry_count", "4"))
+        self.retry_wait: float = float(os.getenv("INDICO_retry_wait", "1"))
+        self.retry_backoff: float = float(os.getenv("INDICO_retry_backoff", "4"))
+        self.retry_jitter: float = float(os.getenv("INDICO_retry_jitter", "1"))
+
         for key, value in kwargs.items():
             if hasattr(self, key):
                 setattr(self, key, value)

indico/http/client.py

@@ -17,7 +17,7 @@
 )
 from indico.http.serialization import aio_deserialize, deserialize
 
-from .retry import aioretry
+from .retry import retry
 
 if TYPE_CHECKING:  # pragma: no cover
     from http.cookiejar import Cookie
@@ -50,6 +50,14 @@ class HTTPClient:
     def __init__(self, config: "Optional[IndicoConfig]" = None):
         self.config = config or IndicoConfig()
         self.base_url = f"{self.config.protocol}://{self.config.host}"
+        self._decorate_with_retry = retry(
+            requests.RequestException,
+            count=self.config.retry_count,
+            wait=self.config.retry_wait,
+            backoff=self.config.retry_backoff,
+            jitter=self.config.retry_jitter,
+        )
+        self._make_request = self._decorate_with_retry(self._make_request)  # type: ignore[method-assign]
 
         self.request_session = requests.Session()
         if isinstance(self.config.requests_params, dict):
@@ -232,6 +240,14 @@ def __init__(self, config: "Optional[IndicoConfig]" = None):
         """
         self.config = config or IndicoConfig()
         self.base_url = f"{self.config.protocol}://{self.config.host}"
+        self._decorate_with_retry = retry(
+            aiohttp.ClientConnectionError,
+            count=self.config.retry_count,
+            wait=self.config.retry_wait,
+            backoff=self.config.retry_backoff,
+            jitter=self.config.retry_jitter,
+        )
+        self._make_request = self._decorate_with_retry(self._make_request)  # type: ignore[method-assign]
 
         self.request_session = aiohttp.ClientSession()
         if isinstance(self.config.requests_params, dict):
@@ -316,7 +332,6 @@ def _handle_files(
             for f in files:
                 f.close()
 
-    @aioretry(aiohttp.ClientConnectionError, aiohttp.ServerDisconnectedError)
     async def _make_request(
         self,
         method: str,

indico/http/retry.py

@@ -1,85 +1,102 @@
 import asyncio
 import time
 from functools import wraps
-from random import randint
-from typing import TYPE_CHECKING
+from inspect import iscoroutinefunction
+from random import random
+from typing import TYPE_CHECKING, overload
 
-if TYPE_CHECKING:  # pragma: no cover
-    from typing import Awaitable, Callable, Optional, Tuple, Type, TypeVar, Union
+if TYPE_CHECKING:
+    import sys
+    from collections.abc import Awaitable, Callable
 
-    from typing_extensions import ParamSpec
+    if sys.version_info >= (3, 10):
+        from typing import ParamSpec, TypeVar
+    else:
+        from typing import Type
+        from typing_extensions import ParamSpec, TypeVar
 
-    P = ParamSpec("P")
-    T = TypeVar("T")
+        type = Type
+
+    ArgumentsType = ParamSpec("ArgumentsType")
+    OuterReturnType = TypeVar("OuterReturnType")
+    InnerReturnType = TypeVar("InnerReturnType")
 
 
 def retry(
-    *ExceptionTypes: "Type[Exception]", tries: int = 3, delay: int = 1, backoff: int = 2
-) -> "Callable[[Callable[P, T]], Callable[P, T]]":
+    *errors: "type[Exception]",
+    count: int,
+    wait: float,
+    backoff: float,
+    jitter: float,
+) -> "Callable[[Callable[ArgumentsType, OuterReturnType]], Callable[ArgumentsType, OuterReturnType]]":  # noqa: E501
     """
-    Retry with exponential backoff
+    Decorate a function or coroutine to retry when it raises specified errors,
+    apply exponential backoff and jitter to the wait time,
+    and raise the last error if it retries too many times.
 
-    Original from: http://wiki.python.org/moin/PythonDecoratorLibrary#Retry
+    Arguments:
+        errors:  Retry the function when it raises one of these errors.
+        count:   Retry the function this many times.
+        wait:    Wait this many seconds after the first error before retrying.
+        backoff: Multiply the wait time by this amount for each additional error.
+        jitter:  Add a random amount of time (up to this percent as a decimal)
+                 to the wait time to prevent simultaneous retries.
     """
 
-    def retry_decorator(f: "Callable[P, T]") -> "Callable[P, T]":
-        @wraps(f)
-        def retry_fn(*args: "P.args", **kwargs: "P.kwargs") -> "T":
-            n_tries, n_delay = tries, delay
-            while n_tries > 1:
-                try:
-                    return f(*args, **kwargs)
-                except ExceptionTypes:
-                    time.sleep(n_delay)
-                    n_tries -= 1
-                    n_delay *= backoff
-            return f(*args, **kwargs)
-
-        return retry_fn
+    def wait_time(times_retried: int) -> float:
+        """
+        Calculate the sleep time based on number of times retried.
+        """
+        return wait * backoff**times_retried * (1 + jitter * random())
 
-    return retry_decorator
+    @overload
+    def retry_decorator(
+        decorated: "Callable[ArgumentsType, Awaitable[InnerReturnType]]",
+    ) -> "Callable[ArgumentsType, Awaitable[InnerReturnType]]": ...
 
+    @overload
+    def retry_decorator(
+        decorated: "Callable[ArgumentsType, InnerReturnType]",
+    ) -> "Callable[ArgumentsType, InnerReturnType]": ...
 
-def aioretry(
-    *ExceptionTypes: "Type[Exception]",
-    tries: int = 3,
-    delay: "Union[int, Tuple[int, int]]" = 1,
-    backoff: int = 2,
-    condition: "Optional[Callable[[Exception], bool]]" = None,
-) -> "Callable[[Callable[P, Awaitable[T]]], Callable[P, Awaitable[T]]]":
-    """
-    Retry with exponential backoff
-
-    Original from: http://wiki.python.org/moin/PythonDecoratorLibrary#Retry
-    Options:
-        condition: Callable to evaluate if an exception of a given type
-            is retryable for additional handling
-        delay: an initial time to wait (seconds). If a tuple, choose a random number
-            in that range to start. This can helps prevent retries at the exact
-            same time across multiple concurrent function calls
-    """
+    def retry_decorator(
+        decorated: "Callable[ArgumentsType, InnerReturnType]",
+    ) -> "Callable[ArgumentsType, Awaitable[InnerReturnType]] | Callable[ArgumentsType, InnerReturnType]":  # noqa: E501
+        """
+        Decorate either a function or coroutine as appropriate.
+        """
+        if iscoroutinefunction(decorated):
+
+            @wraps(decorated)
+            async def retrying_coroutine(  # type: ignore[return]
+                *args: "ArgumentsType.args", **kwargs: "ArgumentsType.kwargs"
+            ) -> "InnerReturnType":
+                for times_retried in range(count + 1):
+                    try:
+                        return await decorated(*args, **kwargs)  # type: ignore[no-any-return]
+                    except errors:
+                        if times_retried >= count:
+                            raise
+
+                    await asyncio.sleep(wait_time(times_retried))
+
+            return retrying_coroutine
+
+        else:
+
+            @wraps(decorated)
+            def retrying_function(  # type: ignore[return]
+                *args: "ArgumentsType.args", **kwargs: "ArgumentsType.kwargs"
+            ) -> "InnerReturnType":
+                for times_retried in range(count + 1):
+                    try:
+                        return decorated(*args, **kwargs)
+                    except errors:
+                        if times_retried >= count:
+                            raise
+
+                    time.sleep(wait_time(times_retried))
 
-    def retry_decorator(f: "Callable[P, Awaitable[T]]") -> "Callable[P, Awaitable[T]]":
-        @wraps(f)
-        async def retry_fn(*args: "P.args", **kwargs: "P.kwargs") -> "T":
-            n_tries = tries
-            if isinstance(delay, tuple):
-                # pick a random number to sleep
-                n_delay = randint(*delay)
-            else:
-                n_delay = delay
-            while True:
-                try:
-                    return await f(*args, **kwargs)
-                except ExceptionTypes as e:
-                    if condition and not condition(e):
-                        raise
-                    await asyncio.sleep(n_delay)
-                    n_tries -= 1
-                    n_delay *= backoff
-                    if n_tries <= 0:
-                        raise
-
-        return retry_fn
+            return retrying_function
 
     return retry_decorator

tests/unit/http/test_retry.py

@@ -0,0 +1,73 @@
+import pytest
+
+from indico.http.retry import retry
+
+
+def test_no_errors() -> None:
+    @retry(Exception, count=0, wait=0, backoff=0, jitter=0)
+    def no_errors() -> bool:
+        return True
+
+    assert no_errors()
+
+
+def test_raises_errors() -> None:
+    calls = 0
+
+    @retry(RuntimeError, count=4, wait=0, backoff=0, jitter=0)
+    def raises_errors() -> None:
+        nonlocal calls
+        calls += 1
+        raise RuntimeError()
+
+    with pytest.raises(RuntimeError):
+        raises_errors()
+
+    assert calls == 5
+
+
+def test_raises_other_errors() -> None:
+    calls = 0
+
+    @retry(RuntimeError, count=4, wait=0, backoff=0, jitter=0)
+    def raises_errors() -> None:
+        nonlocal calls
+        calls += 1
+        raise ValueError()
+
+    with pytest.raises(ValueError):
+        raises_errors()
+
+    assert calls == 1
+
+
+@pytest.mark.asyncio
+async def test_raises_errors_async() -> None:
+    calls = 0
+
+    @retry(RuntimeError, count=4, wait=0, backoff=0, jitter=0)
+    async def raises_errors() -> None:
+        nonlocal calls
+        calls += 1
+        raise RuntimeError()
+
+    with pytest.raises(RuntimeError):
+        await raises_errors()
+
+    assert calls == 5
+
+
+@pytest.mark.asyncio
+async def test_raises_other_errors_async() -> None:
+    calls = 0
+
+    @retry(RuntimeError, count=4, wait=0, backoff=0, jitter=0)
+    async def raises_errors() -> None:
+        nonlocal calls
+        calls += 1
+        raise ValueError()
+
+    with pytest.raises(ValueError):
+        await raises_errors()
+
+    assert calls == 1