From 843db7e4556e646fd0c979481f73de3a24ac3bd6 Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Tue, 21 May 2024 22:48:43 -0500 Subject: [PATCH 1/4] Retry on URLError --- shotgun_api3/shotgun.py | 62 ++++++++++++++++++++++++----------------- tests/test_client.py | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 25 deletions(-) diff --git a/shotgun_api3/shotgun.py b/shotgun_api3/shotgun.py index 3adacb92f..c5b4e4483 100644 --- a/shotgun_api3/shotgun.py +++ b/shotgun_api3/shotgun.py @@ -73,6 +73,9 @@ """ LOG.setLevel(logging.WARN) +MAX_ATTEMPTS = 4 # Three retries on failure +BACKOFF = 0.75 # Seconds to wait before retry, times the attempt number + def _is_mimetypes_broken(): """ @@ -3431,10 +3434,7 @@ def _call_rpc(self, method, params, include_auth_params=True, first=False): req_headers["locale"] = "auto" attempt = 1 - max_attempts = 4 # Three retries on failure - backoff = 0.75 # Seconds to wait before retry, times the attempt number - - while attempt <= max_attempts: + while attempt <= MAX_ATTEMPTS: http_status, resp_headers, body = self._make_call( "POST", self.config.api_path, @@ -3452,9 +3452,9 @@ def _call_rpc(self, method, params, include_auth_params=True, first=False): # We've seen some rare instances of PTR returning 502 for issues that # appear to be caused by something internal to PTR. We're going to # allow for limited retries for those specifically. - if attempt != max_attempts and e.errcode in [502, 504]: + if attempt != MAX_ATTEMPTS and e.errcode in [502, 504]: LOG.debug("Got a 502 or 504 response. Waiting and retrying...") - time.sleep(float(attempt) * backoff) + time.sleep(float(attempt) * BACKOFF) attempt += 1 continue elif e.errcode == 403: @@ -4143,28 +4143,31 @@ def _upload_data_to_storage(self, data, content_type, size, storage_url): request.get_method = lambda: "PUT" attempt = 1 - max_attempts = 4 # Three retries on failure - backoff = 0.75 # Seconds to wait before retry, times the attempt number - - while attempt <= max_attempts: + while attempt <= MAX_ATTEMPTS: try: result = self._make_upload_request(request, opener) LOG.debug("Completed request to %s" % request.get_method()) except urllib.error.HTTPError as e: - if attempt != max_attempts and e.code in [500, 503]: + if attempt != MAX_ATTEMPTS and e.code in [500, 503]: LOG.debug("Got a %s response. Waiting and retrying..." % e.code) - time.sleep(float(attempt) * backoff) + time.sleep(float(attempt) * BACKOFF) attempt += 1 continue elif e.code in [500, 503]: raise ShotgunError("Got a %s response when uploading to %s: %s" % (e.code, storage_url, e)) else: raise ShotgunError("Unanticipated error occurred uploading to %s: %s" % (storage_url, e)) - + except urllib.error.URLError as e: + LOG.debug("Got a '%s' response. Waiting and retrying..." % e) + time.sleep(float(attempt) * BACKOFF) + attempt += 1 + continue else: break + else: + raise ShotgunError("Max attemps limit reached.") etag = result.info()["Etag"] LOG.debug("Part upload completed successfully.") @@ -4250,19 +4253,28 @@ def _send_form(self, url, params): opener = self._build_opener(FormPostHandler) - # Perform the request - try: - resp = opener.open(url, params) - result = resp.read() - # response headers are in str(resp.info()).splitlines() - except urllib.error.HTTPError as e: - if e.code == 500: - raise ShotgunError("Server encountered an internal error. " - "\n%s\n(%s)\n%s\n\n" % (url, self._sanitize_auth_params(params), e)) - else: - raise ShotgunError("Unanticipated error occurred %s" % (e)) + attempt = 1 + while attempt <= MAX_ATTEMPTS: + # Perform the request + try: + resp = opener.open(url, params) + result = resp.read() + # response headers are in str(resp.info()).splitlines() + except urllib.error.URLError as e: + LOG.debug("Got a %s response. Waiting and retrying..." % e) + time.sleep(float(attempt) * BACKOFF) + attempt += 1 + continue + except urllib.error.HTTPError as e: + if e.code == 500: + raise ShotgunError("Server encountered an internal error. " + "\n%s\n(%s)\n%s\n\n" % (url, self._sanitize_auth_params(params), e)) + else: + raise ShotgunError("Unanticipated error occurred %s" % (e)) - return six.ensure_text(result) + return six.ensure_text(result) + else: + raise ShotgunError("Max attemps limit reached.") class CACertsHTTPSConnection(http_client.HTTPConnection): diff --git a/tests/test_client.py b/tests/test_client.py index 9a6b0f9b8..3395c168f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -500,6 +500,62 @@ def test_upload_s3_500(self): self.assertTrue( max_attempts == self.sg._make_upload_request.call_count, "Call is repeated up to 3 times") + + @mock.patch("urllib.request.build_opener") + def test_upload_s3_urlerror__get_attachment_upload_info(self, mock_opener): + """ + Test URLError response is retried when uploading to S3. + """ + mock_opener.return_value.open.side_effect = urllib.error.URLError( + "[WinError 10054] An existing connection was forcibly closed by the remote host" + ) + self.sg._requires_direct_s3_upload = mock.Mock(spec=api.Shotgun._requires_direct_s3_upload, + return_value=True) + + this_dir, _ = os.path.split(__file__) + path = os.path.abspath(os.path.expanduser( + os.path.join(this_dir, "sg_logo.jpg"))) + + with self.assertRaises(api.ShotgunError) as cm: + self.sg.upload("Shot", 1234, path) + + # Test the max retries attempt + self.assertTrue( + 4 == mock_opener().open.call_count, + "Call is repeated up to 4 times") + + # Test the exception message + the_exception = cm.exception + self.assertTrue(str(the_exception) == "Max attemps limit reached.") + + def test_upload_s3_urlerror__upload_to_storage(self): + """ + Test URLError response is retried when uploading to S3. + """ + self.sg._make_upload_request = mock.Mock(spec=api.Shotgun._make_upload_request, + side_effect = urllib.error.URLError( + "[Errno 104] Connection reset by peer", + )) + + this_dir, _ = os.path.split(__file__) + storage_url = "http://foo.com/" + path = os.path.abspath(os.path.expanduser( + os.path.join(this_dir, "sg_logo.jpg"))) + + # Test the Internal function that is used to upload each + # data part in the context of multi-part uploads to S3, we + # simulate the HTTPError exception raised with 503 status errors + with self.assertRaises(api.ShotgunError) as cm: + self.sg._upload_file_to_storage(path, storage_url) + + # Test the max retries attempt + self.assertTrue( + 4 == self.sg._make_upload_request.call_count, + "Call is repeated up to 3 times") + + # Test the exception message + the_exception = cm.exception + self.assertTrue(str(the_exception) == "Max attemps limit reached.") def test_transform_data(self): """Outbound data is transformed""" From 327ff44707d9de06e91942b4b65c5f9e4f9b5c31 Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Wed, 22 May 2024 08:08:37 -0500 Subject: [PATCH 2/4] Fix test --- shotgun_api3/shotgun.py | 23 +++++++++--------- tests/base.py | 2 ++ tests/test_client.py | 54 +++++++++++++++++++++-------------------- 3 files changed, 41 insertions(+), 38 deletions(-) diff --git a/shotgun_api3/shotgun.py b/shotgun_api3/shotgun.py index c5b4e4483..dfeb5f90c 100644 --- a/shotgun_api3/shotgun.py +++ b/shotgun_api3/shotgun.py @@ -73,9 +73,6 @@ """ LOG.setLevel(logging.WARN) -MAX_ATTEMPTS = 4 # Three retries on failure -BACKOFF = 0.75 # Seconds to wait before retry, times the attempt number - def _is_mimetypes_broken(): """ @@ -503,6 +500,8 @@ class Shotgun(object): r"(\D?([01]\d|2[0-3])\D?([0-5]\d)\D?([0-5]\d)?\D?(\d{3})?)?$") _MULTIPART_UPLOAD_CHUNK_SIZE = 20000000 + MAX_ATTEMPTS = 4 # Three retries on failure + BACKOFF = 0.75 # Seconds to wait before retry, times the attempt number def __init__(self, base_url, @@ -3434,7 +3433,7 @@ def _call_rpc(self, method, params, include_auth_params=True, first=False): req_headers["locale"] = "auto" attempt = 1 - while attempt <= MAX_ATTEMPTS: + while attempt <= self.MAX_ATTEMPTS: http_status, resp_headers, body = self._make_call( "POST", self.config.api_path, @@ -3452,9 +3451,9 @@ def _call_rpc(self, method, params, include_auth_params=True, first=False): # We've seen some rare instances of PTR returning 502 for issues that # appear to be caused by something internal to PTR. We're going to # allow for limited retries for those specifically. - if attempt != MAX_ATTEMPTS and e.errcode in [502, 504]: + if attempt != self.MAX_ATTEMPTS and e.errcode in [502, 504]: LOG.debug("Got a 502 or 504 response. Waiting and retrying...") - time.sleep(float(attempt) * BACKOFF) + time.sleep(float(attempt) * self.BACKOFF) attempt += 1 continue elif e.errcode == 403: @@ -4143,16 +4142,16 @@ def _upload_data_to_storage(self, data, content_type, size, storage_url): request.get_method = lambda: "PUT" attempt = 1 - while attempt <= MAX_ATTEMPTS: + while attempt <= self.MAX_ATTEMPTS: try: result = self._make_upload_request(request, opener) LOG.debug("Completed request to %s" % request.get_method()) except urllib.error.HTTPError as e: - if attempt != MAX_ATTEMPTS and e.code in [500, 503]: + if attempt != self.MAX_ATTEMPTS and e.code in [500, 503]: LOG.debug("Got a %s response. Waiting and retrying..." % e.code) - time.sleep(float(attempt) * BACKOFF) + time.sleep(float(attempt) * self.BACKOFF) attempt += 1 continue elif e.code in [500, 503]: @@ -4161,7 +4160,7 @@ def _upload_data_to_storage(self, data, content_type, size, storage_url): raise ShotgunError("Unanticipated error occurred uploading to %s: %s" % (storage_url, e)) except urllib.error.URLError as e: LOG.debug("Got a '%s' response. Waiting and retrying..." % e) - time.sleep(float(attempt) * BACKOFF) + time.sleep(float(attempt) * self.BACKOFF) attempt += 1 continue else: @@ -4254,7 +4253,7 @@ def _send_form(self, url, params): opener = self._build_opener(FormPostHandler) attempt = 1 - while attempt <= MAX_ATTEMPTS: + while attempt <= self.MAX_ATTEMPTS: # Perform the request try: resp = opener.open(url, params) @@ -4262,7 +4261,7 @@ def _send_form(self, url, params): # response headers are in str(resp.info()).splitlines() except urllib.error.URLError as e: LOG.debug("Got a %s response. Waiting and retrying..." % e) - time.sleep(float(attempt) * BACKOFF) + time.sleep(float(attempt) * self.BACKOFF) attempt += 1 continue except urllib.error.HTTPError as e: diff --git a/tests/base.py b/tests/base.py index 01b39bd15..28547cbeb 100644 --- a/tests/base.py +++ b/tests/base.py @@ -150,6 +150,8 @@ def _setup_mock(self, s3_status_code_error=503): # create the server caps directly to say we have the correct version self.sg._server_caps = ServerCapabilities(self.sg.config.server, {"version": [2, 4, 0]}) + # prevent waiting for backoff + self.sg.BACKOFF = 0 def _mock_http(self, data, headers=None, status=None): """Setup a mock response from the PTR server. diff --git a/tests/test_client.py b/tests/test_client.py index 3395c168f..33b996700 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -501,46 +501,48 @@ def test_upload_s3_500(self): max_attempts == self.sg._make_upload_request.call_count, "Call is repeated up to 3 times") - @mock.patch("urllib.request.build_opener") - def test_upload_s3_urlerror__get_attachment_upload_info(self, mock_opener): + def test_upload_s3_urlerror__get_attachment_upload_info(self): """ - Test URLError response is retried when uploading to S3. + Test URLError response is retried when invoking _send_form """ + mock_opener = mock.Mock() mock_opener.return_value.open.side_effect = urllib.error.URLError( "[WinError 10054] An existing connection was forcibly closed by the remote host" ) - self.sg._requires_direct_s3_upload = mock.Mock(spec=api.Shotgun._requires_direct_s3_upload, - return_value=True) - + self.sg._build_opener = mock_opener this_dir, _ = os.path.split(__file__) - path = os.path.abspath(os.path.expanduser( - os.path.join(this_dir, "sg_logo.jpg"))) - + path = os.path.abspath( + os.path.expanduser(os.path.join(this_dir, "sg_logo.jpg")) + ) + with self.assertRaises(api.ShotgunError) as cm: - self.sg.upload("Shot", 1234, path) + self.sg._get_attachment_upload_info(False, path, False) # Test the max retries attempt - self.assertTrue( - 4 == mock_opener().open.call_count, - "Call is repeated up to 4 times") - + self.assertEqual( + 4, mock_opener.return_value.open.call_count, "Call is repeated up to 4 times" + ) + # Test the exception message the_exception = cm.exception - self.assertTrue(str(the_exception) == "Max attemps limit reached.") + self.assertEqual(str(the_exception), "Max attemps limit reached.") def test_upload_s3_urlerror__upload_to_storage(self): """ Test URLError response is retried when uploading to S3. """ - self.sg._make_upload_request = mock.Mock(spec=api.Shotgun._make_upload_request, - side_effect = urllib.error.URLError( - "[Errno 104] Connection reset by peer", - )) + self.sg._make_upload_request = mock.Mock( + spec=api.Shotgun._make_upload_request, + side_effect=urllib.error.URLError( + "[Errno 104] Connection reset by peer", + ), + ) this_dir, _ = os.path.split(__file__) storage_url = "http://foo.com/" - path = os.path.abspath(os.path.expanduser( - os.path.join(this_dir, "sg_logo.jpg"))) + path = os.path.abspath( + os.path.expanduser(os.path.join(this_dir, "sg_logo.jpg")) + ) # Test the Internal function that is used to upload each # data part in the context of multi-part uploads to S3, we @@ -549,13 +551,13 @@ def test_upload_s3_urlerror__upload_to_storage(self): self.sg._upload_file_to_storage(path, storage_url) # Test the max retries attempt - self.assertTrue( - 4 == self.sg._make_upload_request.call_count, - "Call is repeated up to 3 times") - + self.assertEqual( + 4, self.sg._make_upload_request.call_count, "Call is repeated up to 3 times" + ) + # Test the exception message the_exception = cm.exception - self.assertTrue(str(the_exception) == "Max attemps limit reached.") + self.assertEqual(str(the_exception), "Max attemps limit reached.") def test_transform_data(self): """Outbound data is transformed""" From 8f33e277069c3d265b4cfc26bb30d4546d9cae4e Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Wed, 22 May 2024 08:14:04 -0500 Subject: [PATCH 3/4] Reduce attempt --- shotgun_api3/shotgun.py | 2 +- tests/test_client.py | 14 ++++++-------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/shotgun_api3/shotgun.py b/shotgun_api3/shotgun.py index dfeb5f90c..84322f028 100644 --- a/shotgun_api3/shotgun.py +++ b/shotgun_api3/shotgun.py @@ -500,7 +500,7 @@ class Shotgun(object): r"(\D?([01]\d|2[0-3])\D?([0-5]\d)\D?([0-5]\d)?\D?(\d{3})?)?$") _MULTIPART_UPLOAD_CHUNK_SIZE = 20000000 - MAX_ATTEMPTS = 4 # Three retries on failure + MAX_ATTEMPTS = 3 # Retries on failure BACKOFF = 0.75 # Seconds to wait before retry, times the attempt number def __init__(self, diff --git a/tests/test_client.py b/tests/test_client.py index 33b996700..e0dda4e95 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -438,7 +438,7 @@ def test_call_rpc(self): self._mock_http(d, status=(502, "bad gateway")) self.assertRaises(api.ProtocolError, self.sg._call_rpc, "list", a) self.assertEqual( - 4, + 3, self.sg._http_request.call_count, "Call is repeated up to 3 times", ) @@ -449,7 +449,7 @@ def test_call_rpc(self): self._mock_http(d, status=(504, "gateway timeout")) self.assertRaises(api.ProtocolError, self.sg._call_rpc, "list", a) self.assertEqual( - 4, + 3, self.sg._http_request.call_count, "Call is repeated up to 3 times", ) @@ -462,7 +462,6 @@ def test_upload_s3_503(self): storage_url = "http://foo.com/" path = os.path.abspath(os.path.expanduser( os.path.join(this_dir, "sg_logo.jpg"))) - max_attempts = 4 # Max retries to S3 server attempts # Expected HTTPError exception error message expected = "The server is currently down or to busy to reply." \ "Please try again later." @@ -474,7 +473,7 @@ def test_upload_s3_503(self): self.sg._upload_file_to_storage(path, storage_url) # Test the max retries attempt self.assertTrue( - max_attempts == self.sg._make_upload_request.call_count, + 3 == self.sg._make_upload_request.call_count, "Call is repeated up to 3 times") def test_upload_s3_500(self): @@ -486,7 +485,6 @@ def test_upload_s3_500(self): storage_url = "http://foo.com/" path = os.path.abspath(os.path.expanduser( os.path.join(this_dir, "sg_logo.jpg"))) - max_attempts = 4 # Max retries to S3 server attempts # Expected HTTPError exception error message expected = "The server is currently down or to busy to reply." \ "Please try again later." @@ -498,7 +496,7 @@ def test_upload_s3_500(self): self.sg._upload_file_to_storage(path, storage_url) # Test the max retries attempt self.assertTrue( - max_attempts == self.sg._make_upload_request.call_count, + 3 == self.sg._make_upload_request.call_count, "Call is repeated up to 3 times") def test_upload_s3_urlerror__get_attachment_upload_info(self): @@ -520,7 +518,7 @@ def test_upload_s3_urlerror__get_attachment_upload_info(self): # Test the max retries attempt self.assertEqual( - 4, mock_opener.return_value.open.call_count, "Call is repeated up to 4 times" + 3, mock_opener.return_value.open.call_count, "Call is repeated up to 3 times" ) # Test the exception message @@ -552,7 +550,7 @@ def test_upload_s3_urlerror__upload_to_storage(self): # Test the max retries attempt self.assertEqual( - 4, self.sg._make_upload_request.call_count, "Call is repeated up to 3 times" + 3, self.sg._make_upload_request.call_count, "Call is repeated up to 3 times" ) # Test the exception message From 825a2fa2434e4ce02036a91ec47e66ea302879c9 Mon Sep 17 00:00:00 2001 From: Carlos Villavicencio Date: Tue, 28 May 2024 07:07:29 -0500 Subject: [PATCH 4/4] Use constant --- tests/test_client.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index e0dda4e95..6275e4d28 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -438,9 +438,9 @@ def test_call_rpc(self): self._mock_http(d, status=(502, "bad gateway")) self.assertRaises(api.ProtocolError, self.sg._call_rpc, "list", a) self.assertEqual( - 3, + self.sg.MAX_ATTEMPTS, self.sg._http_request.call_count, - "Call is repeated up to 3 times", + f"Call is repeated up to {self.sg.MAX_ATTEMPTS} times", ) # 504 @@ -449,9 +449,9 @@ def test_call_rpc(self): self._mock_http(d, status=(504, "gateway timeout")) self.assertRaises(api.ProtocolError, self.sg._call_rpc, "list", a) self.assertEqual( - 3, + self.sg.MAX_ATTEMPTS, self.sg._http_request.call_count, - "Call is repeated up to 3 times", + f"Call is repeated up to {self.sg.MAX_ATTEMPTS} times", ) def test_upload_s3_503(self): @@ -473,8 +473,8 @@ def test_upload_s3_503(self): self.sg._upload_file_to_storage(path, storage_url) # Test the max retries attempt self.assertTrue( - 3 == self.sg._make_upload_request.call_count, - "Call is repeated up to 3 times") + self.sg.MAX_ATTEMPTS == self.sg._make_upload_request.call_count, + f"Call is repeated up to {self.sg.MAX_ATTEMPTS} times") def test_upload_s3_500(self): """ @@ -496,8 +496,8 @@ def test_upload_s3_500(self): self.sg._upload_file_to_storage(path, storage_url) # Test the max retries attempt self.assertTrue( - 3 == self.sg._make_upload_request.call_count, - "Call is repeated up to 3 times") + self.sg.MAX_ATTEMPTS == self.sg._make_upload_request.call_count, + f"Call is repeated up to {self.sg.MAX_ATTEMPTS} times") def test_upload_s3_urlerror__get_attachment_upload_info(self): """ @@ -518,7 +518,9 @@ def test_upload_s3_urlerror__get_attachment_upload_info(self): # Test the max retries attempt self.assertEqual( - 3, mock_opener.return_value.open.call_count, "Call is repeated up to 3 times" + self.sg.MAX_ATTEMPTS, + mock_opener.return_value.open.call_count, + f"Call is repeated up to {self.sg.MAX_ATTEMPTS} times" ) # Test the exception message @@ -550,7 +552,9 @@ def test_upload_s3_urlerror__upload_to_storage(self): # Test the max retries attempt self.assertEqual( - 3, self.sg._make_upload_request.call_count, "Call is repeated up to 3 times" + self.sg.MAX_ATTEMPTS, + self.sg._make_upload_request.call_count, + f"Call is repeated up to {self.sg.MAX_ATTEMPTS} times" ) # Test the exception message