Retry policies don't sleep after operations time out (Azure#18548)

Author: chlowell

sdk/core/azure-core/azure/core/pipeline/policies/_retry.py

@@ -455,7 +455,7 @@ def send(self, request):
                 # succeed--we'll never have a response to it, so propagate the exception
                 raise
             except AzureError as err:
-                if self._is_method_retryable(retry_settings, request.http_request):
+                if absolute_timeout > 0 and self._is_method_retryable(retry_settings, request.http_request):
                     retry_active = self.increment(retry_settings, response=request, error=err)
                     if retry_active:
                         self.sleep(retry_settings, request.context.transport)

sdk/core/azure-core/azure/core/pipeline/policies/_retry_async.py

@@ -157,7 +157,7 @@ async def send(self, request):  # pylint:disable=invalid-overridden-method
                 # succeed--we'll never have a response to it, so propagate the exception
                 raise
             except AzureError as err:
-                if self._is_method_retryable(retry_settings, request.http_request):
+                if absolute_timeout > 0 and self._is_method_retryable(retry_settings, request.http_request):
                     retry_active = self.increment(retry_settings, response=request, error=err)
                     if retry_active:
                         await self.sleep(retry_settings, request.context.transport)

sdk/core/azure-core/tests/async_tests/test_retry_policy_async.py

@@ -11,7 +11,13 @@
 from unittest.mock import Mock
 import pytest
 from azure.core.configuration import ConnectionConfiguration
-from azure.core.exceptions import AzureError, ServiceResponseError, ServiceResponseTimeoutError
+from azure.core.exceptions import (
+    AzureError,
+    ServiceRequestError,
+    ServiceRequestTimeoutError,
+    ServiceResponseError,
+    ServiceResponseTimeoutError
+)
 from azure.core.pipeline.policies import (
     AsyncRetryPolicy,
     RetryMode,
@@ -258,3 +264,26 @@ async def send(request, **kwargs):
 
     await pipeline.run(HttpRequest("GET", "http://127.0.0.1/"))
     assert transport.send.call_count == 1, "policy should not retry: its first send succeeded"
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    "transport_error,expected_timeout_error",
+    ((ServiceRequestError, ServiceRequestTimeoutError), (ServiceResponseError, ServiceResponseTimeoutError)),
+)
+async def test_does_not_sleep_after_timeout(transport_error, expected_timeout_error):
+    # With default settings policy will sleep twice before exhausting its retries: 1.6s, 3.2s.
+    # It should not sleep the second time when given timeout=1
+    timeout = 1
+
+    transport = Mock(
+        spec=AsyncHttpTransport,
+        send=Mock(side_effect=transport_error("oops")),
+        sleep=Mock(wraps=asyncio.sleep),
+    )
+    pipeline = AsyncPipeline(transport, [AsyncRetryPolicy(timeout=timeout)])
+
+    with pytest.raises(expected_timeout_error):
+        await pipeline.run(HttpRequest("GET", "http://127.0.0.1/"))
+
+    assert transport.sleep.call_count == 1

sdk/core/azure-core/tests/test_retry_policy.py

@@ -9,7 +9,13 @@
     from cStringIO import StringIO as BytesIO
 import pytest
 from azure.core.configuration import ConnectionConfiguration
-from azure.core.exceptions import AzureError, ServiceResponseError, ServiceResponseTimeoutError
+from azure.core.exceptions import (
+    AzureError,
+    ServiceRequestError,
+    ServiceRequestTimeoutError,
+    ServiceResponseError,
+    ServiceResponseTimeoutError,
+)
 from azure.core.pipeline.policies import (
     RetryPolicy,
     RetryMode,
@@ -255,3 +261,25 @@ def send(request, **kwargs):
 
     pipeline.run(HttpRequest("GET", "http://127.0.0.1/"))
     assert transport.send.call_count == 1, "policy should not retry: its first send succeeded"
+
+
+@pytest.mark.parametrize(
+    "transport_error,expected_timeout_error",
+    ((ServiceRequestError, ServiceRequestTimeoutError), (ServiceResponseError, ServiceResponseTimeoutError)),
+)
+def test_does_not_sleep_after_timeout(transport_error, expected_timeout_error):
+    # With default settings policy will sleep twice before exhausting its retries: 1.6s, 3.2s.
+    # It should not sleep the second time when given timeout=1
+    timeout = 1
+
+    transport = Mock(
+        spec=HttpTransport,
+        send=Mock(side_effect=transport_error("oops")),
+        sleep=Mock(wraps=time.sleep),
+    )
+    pipeline = Pipeline(transport, [RetryPolicy(timeout=timeout)])
+
+    with pytest.raises(expected_timeout_error):
+        pipeline.run(HttpRequest("GET", "http://127.0.0.1/"))
+
+    assert transport.sleep.call_count == 1