Skip to content

Commit

Permalink
Retry on client/timeout errors
Browse files Browse the repository at this point in the history
It looks like the existing logic to retry on timeouts or client errors
was not working as expected. For instance the DF loop would exit on
timeouts.

The aiohttp client can raise different errors
(https://docs.aiohttp.org/en/stable/client_reference.html#client-exceptions)
and also asyncio.TimeoutError.

I tried locally by misconfiguring the hostname (it cannot be resolved)
and the port (triggers a timeout).

However this means that retries will be performed upon startup even if
the hostname cannot be resolved for instance. Which is not 100% like the
Java BDK who has a different logic for authentication and datafeed
retries. The problem is how sessions are refreshed, here lazily so we
have two nested @Retry calls making it difficult to have different
strategies.

Fixes finos#256
  • Loading branch information
symphony-youri committed Feb 15, 2022
1 parent 5ad3d02 commit 5112b21
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 8 deletions.
7 changes: 4 additions & 3 deletions symphony/bdk/core/retry/strategy.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from aiohttp import ClientConnectorError
from aiohttp import ClientConnectionError
from asyncio import TimeoutError
from tenacity import RetryCallState

from symphony.bdk.core.auth.exception import AuthUnauthorizedError
Expand All @@ -14,12 +15,12 @@ def is_client_error(exception: Exception) -> bool:


def is_client_timeout_error(exception: Exception):
"""Checks if the exception is a :py:class:`ClientConnectorError` with a :py:class:`TimeoutError` as cause
"""Checks if the exception is a client timeout error
:param exception: The exception to be checked
:return: True if checks the predicate, False otherwise
"""
return isinstance(exception, ClientConnectorError) and isinstance(exception.__cause__, TimeoutError)
return isinstance(exception, ClientConnectionError) or isinstance(exception, TimeoutError)


def can_authentication_be_retried(exception: Exception) -> bool:
Expand Down
12 changes: 7 additions & 5 deletions tests/core/retry/strategy_test.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import asyncio
from unittest.mock import Mock, AsyncMock

import aiohttp
import pytest
from aiohttp import ClientConnectorError

Expand Down Expand Up @@ -108,15 +109,16 @@ async def test_should_retry():
strategies = [TestAuthenticationStrategy(), TestRefreshSessionStrategy(), TestReadDatafeedStrategy()]
connection_key = Mock()
connection_key.ssl = "ssl"
exception_from_a_timeout = ClientConnectorError(connection_key, TimeoutError())
exception_from_a_timeout.__cause__ = TimeoutError()
thing = FixedChainedExceptions([ApiException(429), ApiException(500), exception_from_a_timeout])
exception_from_client = aiohttp.ClientConnectionError
exception_from_a_timeout = asyncio.TimeoutError()
thing = FixedChainedExceptions([ApiException(429), ApiException(500),
exception_from_client, exception_from_a_timeout])

for s in strategies:
s._retry_config = minimal_retry_config_with_attempts(4)
s._retry_config = minimal_retry_config_with_attempts(5)

value = await s._retryable_coroutine(thing)

assert value is True
assert thing.call_count == 4
assert thing.call_count == 5
thing.reset() # Reset the counters

0 comments on commit 5112b21

Please sign in to comment.