Retry on connection failures #292
Conversation
Fixes (iMicknl#252) - Implemented improved error handling and retry mechanism to handle transient network failures such as connection resets. - Added exponential backoff strategy for retry attempts - Ensured graceful handling of connection-related errors like ClientConnectorError, ClientOSError, and ServerDisconnectedError
There was a problem hiding this comment.
Looks good! Would you be willing to leverage backoff instead of your own retry implementation?
I am using this for some other libraries and it is very flexible and easy to use.
https://github.com/iMicknl/python-overkiz-api/blob/3f9758380fdee9a298957ffe21d02940292f1934/pyoverkiz/client.py#L394
|
Sure! Please review the modified implementation. |
bakonyiferenc
force-pushed
the
retry-connection-failures
branch
from
c3da94b
to
9eb018f
Compare
| ClientOSError, | ||
| ServerDisconnectedError, | ||
| ) as exception: | ||
| raise ConnectionError(str(exception)) from exception |
There was a problem hiding this comment.
Why would we want to raise a more general ConnectionError exception, instead of the underlying exception?
See https://docs.aiohttp.org/en/v3.8.2/client_reference.html#hierarchy-of-exceptions. You can actually already use ClientError to catch broader exceptions.
There was a problem hiding this comment.
There are two main reasons in my mind:
- To hide the internals of the API. If we ever change the underlying aiohttp library, aiohttp specific exceptions like ClientConnectorError or ClientError will be a source of issues later.
- Raising a single type of built-in exception (ConnectionError) simplifies the error handling logic. Consumers of the API don't need to worry about the specific connection errors that might occur; they can simply catch ConnectionError and handle it appropriately.
|
Happy to merge! Would you be able to run |
Fixes (#252)
ClientConnectorError,ClientOSError, andServerDisconnectedError