From e90e26f9323708df868cc1d839472183d0893d8c Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 20 Mar 2025 13:01:28 -0700 Subject: [PATCH 1/5] copy requests + don't retry client errors --- .../smithy/python/codegen/ClientGenerator.java | 6 +++++- packages/smithy-core/src/smithy_core/retries.py | 17 ++++++++++++----- packages/smithy-core/tests/unit/test_retries.py | 12 ++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java index aee41565d..513e7fafc 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java @@ -132,6 +132,7 @@ private void generateOperationExecutor(PythonWriter writer) { writer.addStdlibImport("typing", "Awaitable"); writer.addStdlibImport("typing", "cast"); writer.addStdlibImport("copy", "deepcopy"); + writer.addStdlibImport("copy", "copy"); writer.addStdlibImport("asyncio"); writer.addStdlibImports("asyncio", Set.of("sleep", "Future")); writer.addStdlibImport("dataclasses", "replace"); @@ -395,7 +396,10 @@ def _classify_error( output_context = await self._handle_attempt( deserialize, interceptor_chain, - request_context, + replace( + request_context, + transport_request = copy(request_context.transport_request) + ), config, operation, request_future, diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index 07b051da9..8d112d734 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -5,6 +5,8 @@ from dataclasses import dataclass from enum import Enum +from smithy_core.interfaces.retries import RetryErrorType + from .exceptions import SmithyRetryException from .interfaces import retries as retries_interface @@ -232,13 +234,18 @@ def refresh_retry_token_for_retry( :raises SmithyRetryException: If no further retry attempts are allowed. """ - retry_count = token_to_renew.retry_count + 1 - if retry_count >= self.max_attempts: + if error_info.error_type in [RetryErrorType.TRANSIENT, RetryErrorType.THROTTLING, RetryErrorType.SERVER_ERROR]: + retry_count = token_to_renew.retry_count + 1 + if retry_count >= self.max_attempts: + raise SmithyRetryException( + f"Reached maximum number of allowed attempts: {self.max_attempts}" + ) + retry_delay = self.backoff_strategy.compute_next_backoff_delay(retry_count) + return SimpleRetryToken(retry_count=retry_count, retry_delay=retry_delay) + else: raise SmithyRetryException( - f"Reached maximum number of allowed attempts: {self.max_attempts}" + "Non retryable error" ) - retry_delay = self.backoff_strategy.compute_next_backoff_delay(retry_count) - return SimpleRetryToken(retry_count=retry_count, retry_delay=retry_delay) def record_success(self, *, token: retries_interface.RetryToken) -> None: """Not used by this retry strategy.""" diff --git a/packages/smithy-core/tests/unit/test_retries.py b/packages/smithy-core/tests/unit/test_retries.py index e79173f66..91d612d7d 100644 --- a/packages/smithy-core/tests/unit/test_retries.py +++ b/packages/smithy-core/tests/unit/test_retries.py @@ -72,3 +72,15 @@ def test_simple_retry_strategy(max_attempts: int) -> None: strategy.refresh_retry_token_for_retry( token_to_renew=token, error_info=error_info ) + +def test_simple_retry_strategy_does_not_retry_client_errors() -> None: + strategy = SimpleRetryStrategy( + backoff_strategy=ExponentialRetryBackoffStrategy(backoff_scale_value=5), + max_attempts=2, + ) + error_info = RetryErrorInfo(error_type=RetryErrorType.CLIENT_ERROR) + token = strategy.acquire_initial_retry_token() + with pytest.raises(SmithyRetryException): + strategy.refresh_retry_token_for_retry( + token_to_renew=token, error_info=error_info + ) From 3f3604b3c42f76c27646588d41de6370029cae01 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 20 Mar 2025 13:03:43 -0700 Subject: [PATCH 2/5] lint cleanup --- packages/smithy-core/src/smithy_core/retries.py | 10 ++++++---- packages/smithy-core/tests/unit/test_retries.py | 1 + 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index 8d112d734..f76d2d71c 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -234,7 +234,11 @@ def refresh_retry_token_for_retry( :raises SmithyRetryException: If no further retry attempts are allowed. """ - if error_info.error_type in [RetryErrorType.TRANSIENT, RetryErrorType.THROTTLING, RetryErrorType.SERVER_ERROR]: + if error_info.error_type in [ + RetryErrorType.TRANSIENT, + RetryErrorType.THROTTLING, + RetryErrorType.SERVER_ERROR, + ]: retry_count = token_to_renew.retry_count + 1 if retry_count >= self.max_attempts: raise SmithyRetryException( @@ -243,9 +247,7 @@ def refresh_retry_token_for_retry( retry_delay = self.backoff_strategy.compute_next_backoff_delay(retry_count) return SimpleRetryToken(retry_count=retry_count, retry_delay=retry_delay) else: - raise SmithyRetryException( - "Non retryable error" - ) + raise SmithyRetryException("Non retryable error") def record_success(self, *, token: retries_interface.RetryToken) -> None: """Not used by this retry strategy.""" diff --git a/packages/smithy-core/tests/unit/test_retries.py b/packages/smithy-core/tests/unit/test_retries.py index 91d612d7d..4cb62fd79 100644 --- a/packages/smithy-core/tests/unit/test_retries.py +++ b/packages/smithy-core/tests/unit/test_retries.py @@ -73,6 +73,7 @@ def test_simple_retry_strategy(max_attempts: int) -> None: token_to_renew=token, error_info=error_info ) + def test_simple_retry_strategy_does_not_retry_client_errors() -> None: strategy = SimpleRetryStrategy( backoff_strategy=ExponentialRetryBackoffStrategy(backoff_scale_value=5), From 2dbcd0d7e3bdd5807fcff083ffd6d040d11fa7f0 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 20 Mar 2025 13:26:36 -0700 Subject: [PATCH 3/5] Update error message --- packages/smithy-core/src/smithy_core/retries.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index f76d2d71c..6be7e5684 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -247,7 +247,9 @@ def refresh_retry_token_for_retry( retry_delay = self.backoff_strategy.compute_next_backoff_delay(retry_count) return SimpleRetryToken(retry_count=retry_count, retry_delay=retry_delay) else: - raise SmithyRetryException("Non retryable error") + raise SmithyRetryException( + f"Error is not retryable: {error_info}" + ) def record_success(self, *, token: retries_interface.RetryToken) -> None: """Not used by this retry strategy.""" From 7f13497109082fc407450a11f1f105fbd2709b5d Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Thu, 20 Mar 2025 13:29:13 -0700 Subject: [PATCH 4/5] Lint fixes --- packages/smithy-core/src/smithy_core/retries.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index 6be7e5684..85ec80833 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -247,9 +247,7 @@ def refresh_retry_token_for_retry( retry_delay = self.backoff_strategy.compute_next_backoff_delay(retry_count) return SimpleRetryToken(retry_count=retry_count, retry_delay=retry_delay) else: - raise SmithyRetryException( - f"Error is not retryable: {error_info}" - ) + raise SmithyRetryException(f"Error is not retryable: {error_info}") def record_success(self, *, token: retries_interface.RetryToken) -> None: """Not used by this retry strategy.""" From b22fdba9df9f4da6190c91eafe4a244e249f81dd Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Fri, 21 Mar 2025 09:03:25 -0700 Subject: [PATCH 5/5] Update packages/smithy-core/src/smithy_core/retries.py Co-authored-by: Jordon Phillips --- packages/smithy-core/src/smithy_core/retries.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/smithy-core/src/smithy_core/retries.py b/packages/smithy-core/src/smithy_core/retries.py index 85ec80833..32275bc38 100644 --- a/packages/smithy-core/src/smithy_core/retries.py +++ b/packages/smithy-core/src/smithy_core/retries.py @@ -234,11 +234,7 @@ def refresh_retry_token_for_retry( :raises SmithyRetryException: If no further retry attempts are allowed. """ - if error_info.error_type in [ - RetryErrorType.TRANSIENT, - RetryErrorType.THROTTLING, - RetryErrorType.SERVER_ERROR, - ]: + if error_info.error_type is not RetryErrorType.CLIENT_ERROR: retry_count = token_to_renew.retry_count + 1 if retry_count >= self.max_attempts: raise SmithyRetryException(