From cae19911e911c06252c69a82e56af82f67dcea28 Mon Sep 17 00:00:00 2001 From: Anatoli Kurtsevich Date: Fri, 6 Oct 2023 15:10:18 -0400 Subject: [PATCH] RAI-17360 Expanded retry mechanism to do retries on a ConnectionError (#137) * expanded retry mechanism to do retries on ConnectionError as well * updated CHANGELOG and version for new release --- CHANGELOG.md | 4 ++++ railib/__init__.py | 2 +- railib/rest.py | 9 ++------- test/test_unit.py | 32 ++++++++++++++++++-------------- 4 files changed, 25 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5902823..1ca3c0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## v0.6.16 + +* Expanded the retry mechanism for HTTP failures raised as `ConnectionError`. + ## v0.6.15 * Increase auth token expiration buffer from 5s to 60s. diff --git a/railib/__init__.py b/railib/__init__.py index 34195d7..95c8813 100644 --- a/railib/__init__.py +++ b/railib/__init__.py @@ -12,5 +12,5 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version_info__ = (0, 6, 15) +__version_info__ = (0, 6, 16) __version__ = ".".join(map(str, __version_info__)) diff --git a/railib/rest.py b/railib/rest.py index 845e091..77e71da 100644 --- a/railib/rest.py +++ b/railib/rest.py @@ -17,8 +17,6 @@ import json import logging from os import path -import socket -import time from urllib.error import URLError from urllib.parse import urlencode, urlsplit, quote from urllib.request import Request, urlopen @@ -228,11 +226,8 @@ def _urlopen_with_retry(req: Request, retries: int = 0): for attempt in range(attempts): try: return urlopen(req) - except URLError as e: - if isinstance(e.reason, socket.timeout): - logger.warning(f"Timeout occurred (attempt {attempt + 1}/{attempts}): {req.full_url}") - else: - logger.warning(f"URLError occurred {e.reason} (attempt {attempt + 1}/{attempts}): {req.full_url}") + except (URLError, ConnectionError) as e: + logger.warning(f"URL/Connection error occured {req.full_url} (attempt {attempt + 1}/{attempts}). Error message: {str(e)}") if attempt == attempts - 1: logger.error(f"Failed to connect to {req.full_url} after {attempts} attempt{'s' if attempts > 1 else ''}") diff --git a/test/test_unit.py b/test/test_unit.py index 98ad7b7..d5a4e7b 100644 --- a/test/test_unit.py +++ b/test/test_unit.py @@ -31,6 +31,9 @@ def test_validation(self): @patch('railib.rest.urlopen') class TestURLOpenWithRetry(unittest.TestCase): + WARNING_LOG_PREFIX = "URL/Connection error occured" + ERROR_LOG_PREFIX = "Failed to connect to" + def test_successful_response(self, mock_urlopen): # Set up the mock urlopen to return a successful response mock_response = MagicMock() @@ -55,7 +58,7 @@ def test_negative_retries(self, _): self.assertIn("Retries must be a non-negative integer", str(e.exception)) - def test_timeout_retry(self, mock_urlopen): + def test_url_error_retry(self, mock_urlopen): # Set up the mock urlopen to raise a socket timeout error mock_urlopen.side_effect = URLError(socket.timeout()) @@ -66,26 +69,27 @@ def test_timeout_retry(self, mock_urlopen): self.assertEqual(mock_urlopen.call_count, 3) # Expect 1 original call and 2 calls for retries self.assertEqual(len(log.output), 4) # Expect 3 log messages for retries and 1 for failure to connect - self.assertIn('Timeout occurred', log.output[0]) - self.assertIn('Timeout occurred', log.output[1]) - self.assertIn('Timeout occurred', log.output[2]) - self.assertIn('Failed to connect to', log.output[3]) + self.assertIn(TestURLOpenWithRetry.WARNING_LOG_PREFIX, log.output[0]) + self.assertIn(TestURLOpenWithRetry.WARNING_LOG_PREFIX, log.output[1]) + self.assertIn(TestURLOpenWithRetry.WARNING_LOG_PREFIX, log.output[2]) + self.assertIn(TestURLOpenWithRetry.ERROR_LOG_PREFIX, log.output[3]) - def test_other_error_retry(self, mock_urlopen): - # Set up the mock urlopen to raise a non-timeout URLError - mock_urlopen.side_effect = URLError('Some other error') + def test_connection_error_retry(self, mock_urlopen): + # Set up the mock urlopen to raise a error that is subclass of ConnectonError + mock_urlopen.side_effect = ConnectionResetError("connection reset by peer") req = Request('https://example.com') with self.assertLogs() as log: with self.assertRaises(Exception): - _urlopen_with_retry(req, retries=2) + _urlopen_with_retry(req, 2) - self.assertEqual(mock_urlopen.call_count, 3) # Expect 3 calls for retries + self.assertEqual(mock_urlopen.call_count, 3) # Expect 1 original call and 2 calls for retries self.assertEqual(len(log.output), 4) # Expect 3 log messages for retries and 1 for failure to connect - self.assertIn('URLError occurred', log.output[0]) - self.assertIn('URLError occurred', log.output[1]) - self.assertIn('URLError occurred', log.output[2]) - self.assertIn('Failed to connect to', log.output[3]) + self.assertIn(TestURLOpenWithRetry.WARNING_LOG_PREFIX, log.output[0]) + self.assertIn(TestURLOpenWithRetry.WARNING_LOG_PREFIX, log.output[1]) + self.assertIn(TestURLOpenWithRetry.WARNING_LOG_PREFIX, log.output[2]) + self.assertIn(TestURLOpenWithRetry.ERROR_LOG_PREFIX, log.output[3]) + if __name__ == '__main__':