From 5cd0ede2aaeef51ff0e637f87d825baf434cb308 Mon Sep 17 00:00:00 2001
From: Rohan McGovern <rmcgover@redhat.com>
Date: Mon, 9 Sep 2019 13:46:37 +0800
Subject: [PATCH] Fix handling of errors with response=None

Our retry policy has custom handling for 404 responses.
The code for that had wrongly assumed that, whenever an exception
had a response attribute, that attribute would be something
other than None.

In practice, the response is None for certain exceptions generated
by the requests library where a failure occurred before a response
was available (e.g. SSL handshake errors, I/O errors). Make sure
we tolerate this, so those exceptions propagate correctly rather
than being transformed to an internal error in the retry policy.
---
 CHANGELOG.md                           |  4 ++-
 pubtools/pulplib/_impl/client/retry.py |  2 +-
 tests/client/test_retry.py             | 35 ++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 2 deletions(-)
 create mode 100644 tests/client/test_retry.py

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 122348d2..6b98f1a1 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,7 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-- n/a
+### Fixed
+- Fixed certain exceptions from requests library (such as SSL handshake errors) not being
+  propagated correctly.
 
 ## [2.0.0] - 2019-09-09
 
diff --git a/pubtools/pulplib/_impl/client/retry.py b/pubtools/pulplib/_impl/client/retry.py
index e6de17f2..aea029f5 100644
--- a/pubtools/pulplib/_impl/client/retry.py
+++ b/pubtools/pulplib/_impl/client/retry.py
@@ -28,7 +28,7 @@ def should_retry(self, attempt, future):
         retry = self._delegate.should_retry(attempt, future)
 
         exception = future.exception()
-        if exception and hasattr(exception, "response"):
+        if exception and getattr(exception, "response", None) is not None:
             # if returned status code is 404, never retry on that
             if exception.response.status_code == 404:
                 return False
diff --git a/tests/client/test_retry.py b/tests/client/test_retry.py
new file mode 100644
index 00000000..af38f75a
--- /dev/null
+++ b/tests/client/test_retry.py
@@ -0,0 +1,35 @@
+import requests
+from more_executors.futures import f_return_error
+
+from pubtools.pulplib._impl.client.retry import PulpRetryPolicy
+
+
+def test_retries_by_default():
+    """Retry policy will retry on generic exception types."""
+    policy = PulpRetryPolicy()
+    assert policy.should_retry(0, f_return_error(RuntimeError("oops!")))
+
+
+def test_retries_http_errors():
+    """Retry policy will retry on HTTP-level errors."""
+    policy = PulpRetryPolicy()
+    response = requests.Response()
+    response.status_code = 500
+    error = requests.HTTPError(response=response)
+    assert policy.should_retry(0, f_return_error(error))
+
+
+def test_retries_http_errors_no_response():
+    """Retry policy will retry on requests exception types with response=None."""
+    policy = PulpRetryPolicy()
+    error = requests.HTTPError(response=None)
+    assert policy.should_retry(0, f_return_error(error))
+
+
+def test_no_retries_http_404_errors():
+    """Retry policy does not retry on HTTP 404 responses."""
+    policy = PulpRetryPolicy()
+    response = requests.Response()
+    response.status_code = 404
+    error = requests.HTTPError(response=response)
+    assert not policy.should_retry(0, f_return_error(error))