From 87e69588fafafad7f4e67e6c76e4e4a751dbb8a5 Mon Sep 17 00:00:00 2001 From: Xiang Yan Date: Fri, 10 Apr 2020 10:08:18 -0700 Subject: [PATCH 1/5] not retry if status code < 400 --- sdk/core/azure-core/CHANGELOG.md | 4 ++++ .../azure-core/azure/core/pipeline/policies/_retry.py | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index f0b5d9b29a31..999a9c9d4bee 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -7,6 +7,10 @@ - Support "x-ms-retry-after-ms" in response header #10743 +### Bug fixes + +- Not retry if the status code is less than 400 #10778 + ## 1.4.0 (2020-04-06) ### Features diff --git a/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py b/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py index 28791b8cf420..b60f647696e8 100644 --- a/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py +++ b/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py @@ -263,12 +263,20 @@ def is_retry(self, settings, response): code is on the list of status codes to be retried upon on the presence of the aforementioned header. + The behavior is: + - If status_code < 400: don’t retry + - Else if Retry-After present: retry + - Else: retry based on the safe status code list ([408, 429, 500, 502, 503, 504]) + + :param dict settings: The retry settings. :param response: The PipelineResponse object :type response: ~azure.core.pipeline.PipelineResponse :return: True if method/status code is retryable. False if not retryable. :rtype: bool """ + if response.http_response.status_code and response.http_response.status_code < 400: + return False has_retry_after = bool(response.http_response.headers.get("Retry-After")) if has_retry_after and self._respect_retry_after_header: return True From 085d08b4ed645d9eef01794aa1d85cef33937a3e Mon Sep 17 00:00:00 2001 From: Xiang Yan Date: Fri, 10 Apr 2020 10:29:19 -0700 Subject: [PATCH 2/5] add test --- .../azure-core/tests/test_retry_policy.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/sdk/core/azure-core/tests/test_retry_policy.py b/sdk/core/azure-core/tests/test_retry_policy.py index 24e4b8e623fa..422643f9f6ad 100644 --- a/sdk/core/azure-core/tests/test_retry_policy.py +++ b/sdk/core/azure-core/tests/test_retry_policy.py @@ -109,6 +109,30 @@ def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineRe pipeline.run(http_request) assert transport._count == 2 +def test_retry_on_201(): + class MockTransport(HttpTransport): + def __init__(self): + self._count = 0 + def __exit__(self, exc_type, exc_val, exc_tb): + pass + def close(self): + pass + def open(self): + pass + + def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineResponse + self._count += 1 + response = HttpResponse(request, None) + response.status_code = 201 + return response + + http_request = HttpRequest('GET', 'http://127.0.0.1/') + http_retry = RetryPolicy(retry_total = 1) + transport = MockTransport() + pipeline = Pipeline(transport, [http_retry]) + pipeline.run(http_request) + assert transport._count == 1 + def test_retry_seekable_stream(): class MockTransport(HttpTransport): def __init__(self): From abd2c35aec7e6b7d4ead4e1479018c24cc461580 Mon Sep 17 00:00:00 2001 From: Xiang Yan Date: Fri, 10 Apr 2020 11:45:50 -0700 Subject: [PATCH 3/5] update --- .../azure-core/azure/core/pipeline/policies/_retry.py | 2 +- .../tests/azure_core_asynctests/test_retry_policy.py | 8 ++++++-- sdk/core/azure-core/tests/test_retry_policy.py | 8 ++++++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py b/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py index b60f647696e8..26219d262f3c 100644 --- a/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py +++ b/sdk/core/azure-core/azure/core/pipeline/policies/_retry.py @@ -275,7 +275,7 @@ def is_retry(self, settings, response): :return: True if method/status code is retryable. False if not retryable. :rtype: bool """ - if response.http_response.status_code and response.http_response.status_code < 400: + if response.http_response.status_code < 400: return False has_retry_after = bool(response.http_response.headers.get("Retry-After")) if has_retry_after and self._respect_retry_after_header: diff --git a/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py b/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py index a6647ddaf082..fcfe6fb09249 100644 --- a/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py +++ b/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py @@ -132,7 +132,9 @@ async def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> Pipe raise AzureError('fail on first') position = request.body.tell() assert position == 0 - return HttpResponse(request, None) + response = HttpResponse(request, None) + response.status_code = 400 + return response data = BytesIO(b"Lots of dataaaa") http_request = HttpRequest('GET', 'http://127.0.0.1/') @@ -166,7 +168,9 @@ async def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> Pipe if name and body and hasattr(body, 'read'): position = body.tell() assert not position - return HttpResponse(request, None) + response = HttpResponse(request, None) + response.status_code = 400 + return response file = tempfile.NamedTemporaryFile(delete=False) file.write(b'Lots of dataaaa') diff --git a/sdk/core/azure-core/tests/test_retry_policy.py b/sdk/core/azure-core/tests/test_retry_policy.py index 422643f9f6ad..67b06ad33b89 100644 --- a/sdk/core/azure-core/tests/test_retry_policy.py +++ b/sdk/core/azure-core/tests/test_retry_policy.py @@ -151,7 +151,9 @@ def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineRe raise AzureError('fail on first') position = request.body.tell() assert position == 0 - return HttpResponse(request, None) + response = HttpResponse(request, None) + response.status_code = 400 + return response data = BytesIO(b"Lots of dataaaa") http_request = HttpRequest('GET', 'http://127.0.0.1/') @@ -184,7 +186,9 @@ def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineRe if name and body and hasattr(body, 'read'): position = body.tell() assert not position - return HttpResponse(request, None) + response = HttpResponse(request, None) + response.status_code = 400 + return response file = tempfile.NamedTemporaryFile(delete=False) file.write(b'Lots of dataaaa') From 2892bcc4faa739f1c7b50b2e651e8e70a30dbb82 Mon Sep 17 00:00:00 2001 From: Xiang Yan Date: Fri, 10 Apr 2020 11:52:33 -0700 Subject: [PATCH 4/5] add test --- .../test_retry_policy.py | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py b/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py index fcfe6fb09249..2ffee74d2add 100644 --- a/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py +++ b/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py @@ -113,6 +113,31 @@ async def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> Pipe await pipeline.run(http_request) assert transport._count == 2 +@pytest.mark.asyncio +async def test_retry_on_201(): + class MockTransport(AsyncHttpTransport): + def __init__(self): + self._count = 0 + async def __aexit__(self, exc_type, exc_val, exc_tb): + pass + async def close(self): + pass + async def open(self): + pass + + async def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineResponse + self._count += 1 + response = HttpResponse(request, None) + response.status_code = 201 + return response + + http_request = HttpRequest('GET', 'http://127.0.0.1/') + http_retry = AsyncRetryPolicy(retry_total = 1) + transport = MockTransport() + pipeline = AsyncPipeline(transport, [http_retry]) + await pipeline.run(http_request) + assert transport._count == 1 + @pytest.mark.asyncio async def test_retry_seekable_stream(): class MockTransport(AsyncHttpTransport): From fca8165dc9d55d7a0ac7df7e1d8e05073f550b05 Mon Sep 17 00:00:00 2001 From: Xiang Yan Date: Fri, 10 Apr 2020 12:00:21 -0700 Subject: [PATCH 5/5] update tests --- .../tests/azure_core_asynctests/test_retry_policy.py | 4 +++- sdk/core/azure-core/tests/test_retry_policy.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py b/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py index 2ffee74d2add..a03570c0def0 100644 --- a/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py +++ b/sdk/core/azure-core/tests/azure_core_asynctests/test_retry_policy.py @@ -114,7 +114,7 @@ async def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> Pipe assert transport._count == 2 @pytest.mark.asyncio -async def test_retry_on_201(): +async def test_no_retry_on_201(): class MockTransport(AsyncHttpTransport): def __init__(self): self._count = 0 @@ -129,6 +129,8 @@ async def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> Pipe self._count += 1 response = HttpResponse(request, None) response.status_code = 201 + headers = {"Retry-After": "1"} + response.headers = headers return response http_request = HttpRequest('GET', 'http://127.0.0.1/') diff --git a/sdk/core/azure-core/tests/test_retry_policy.py b/sdk/core/azure-core/tests/test_retry_policy.py index 67b06ad33b89..5c7d0dd50d2c 100644 --- a/sdk/core/azure-core/tests/test_retry_policy.py +++ b/sdk/core/azure-core/tests/test_retry_policy.py @@ -109,7 +109,7 @@ def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineRe pipeline.run(http_request) assert transport._count == 2 -def test_retry_on_201(): +def test_no_retry_on_201(): class MockTransport(HttpTransport): def __init__(self): self._count = 0 @@ -124,6 +124,8 @@ def send(self, request, **kwargs): # type: (PipelineRequest, Any) -> PipelineRe self._count += 1 response = HttpResponse(request, None) response.status_code = 201 + headers = {"Retry-After": "1"} + response.headers = headers return response http_request = HttpRequest('GET', 'http://127.0.0.1/')