Fix retry code

- Perform more explicit checks with retry handling of urllib level
  exceptions
- Update test requirements

blobxfer/retry.py

@@ -40,17 +40,19 @@ import urllib3
 
 
 # global defines
-_RETRYABLE_ERRNO = frozenset((
+_RETRYABLE_ERRNO_MAXRETRY = frozenset((
     '[Errno {}]'.format(errno.ECONNRESET),
     '[Errno {}]'.format(errno.ECONNREFUSED),
     '[Errno {}]'.format(errno.ECONNABORTED),
     '[Errno {}]'.format(errno.ENETRESET),
     '[Errno {}]'.format(errno.ETIMEDOUT),
 ))
-_NON_RETRYABLE_ERRNO = frozenset((
-    '[Errno -2] Name or service not known',
-    '[Errno 8] nodename nor servname',
-    '[Errno 11001] getaddrinfo failed',
+_RETRYABLE_ERRNO_PROTOCOL = frozenset((
+    '({},'.format(errno.ECONNRESET),
+    '({},'.format(errno.ECONNREFUSED),
+    '({},'.format(errno.ECONNABORTED),
+    '({},'.format(errno.ENETRESET),
+    '({},'.format(errno.ETIMEDOUT),
 ))
 
 
@@ -107,27 +109,36 @@ class ExponentialRetryWithMaxWait(azure.storage.common.retry._Retry):
         # appropriately from the lower layer
         if status is None:
             exc = context.exception
+            # default to not retry in unknown/unhandled exception case
+            ret = False
             # requests timeout, retry
             if isinstance(exc, requests.Timeout):
-                return True
+                ret = True
+            elif isinstance(exc, requests.exceptions.ContentDecodingError):
+                ret = True
             elif (isinstance(exc, requests.exceptions.ConnectionError) or
                   isinstance(exc, requests.exceptions.ChunkedEncodingError)):
                 # newer versions of requests do not expose errno on the
                 # args[0] reason object; manually string parse
-                if (isinstance(
-                        exc.args[0], urllib3.exceptions.MaxRetryError) and
-                        isinstance(
-                            exc.args[0].reason,
-                            urllib3.exceptions.NewConnectionError)):
-                    msg = exc.args[0].reason.args[0]
-                    if any(x in msg for x in _NON_RETRYABLE_ERRNO):
-                        return False
-                    elif any(x in msg for x in _RETRYABLE_ERRNO):
-                        return True
+                if isinstance(exc.args[0], urllib3.exceptions.MaxRetryError):
+                    try:
+                        msg = exc.args[0].reason.args[0]
+                    except (AttributeError, IndexError):
+                        # unexpected/malformed exception hierarchy, don't retry
+                        pass
                     else:
-                        # default to not retry in unknown case
-                        return False
-            return True
+                        if any(x in msg for x in _RETRYABLE_ERRNO_MAXRETRY):
+                            ret = True
+                elif isinstance(exc.args[0], urllib3.exceptions.ProtocolError):
+                    try:
+                        msg = exc.args[0].args[0]
+                    except (AttributeError, IndexError):
+                        # unexpected/malformed exception hierarchy, don't retry
+                        pass
+                    else:
+                        if any(x in msg for x in _RETRYABLE_ERRNO_PROTOCOL):
+                            ret = True
+            return ret
         elif 200 <= status < 300:
             # failure during respond body download or parsing, so success
             # codes should be retried

test_requirements.txt

@@ -1,5 +1,5 @@
-coverage>=4.4.1
-flake8>=3.4.1
-mock>=2.0.0; python_version < '3.3'
-pytest>=3.2.3
-pytest-cov>=2.5.1
+coverage==4.5.1
+flake8==3.5.0
+mock==2.0.0; python_version < '3.3'
+pytest==3.5.1
+pytest-cov==2.5.1

tests/test_blobxfer_retry.py

@@ -20,36 +20,30 @@ def test_should_retry():
     context = mock.MagicMock()
     context.count = 1
     er.max_attempts = 1
-
     assert not er._should_retry(context)
 
     context.count = 0
     er.max_attempts = 20
     context.response.status = None
     context.exception = requests.Timeout()
-
     assert er._should_retry(context)
 
+    # test malformed
     ex = requests.ConnectionError(
         urllib3.exceptions.MaxRetryError(
-            mock.MagicMock(), mock.MagicMock(),
-            reason=urllib3.exceptions.NewConnectionError(
-                list(retry._NON_RETRYABLE_ERRNO)[0], 'message')
-        )
+            mock.MagicMock(), mock.MagicMock())
     )
     context.exception = ex
-
     assert not er._should_retry(context)
 
     ex = requests.ConnectionError(
         urllib3.exceptions.MaxRetryError(
             mock.MagicMock(), mock.MagicMock(),
             reason=urllib3.exceptions.NewConnectionError(
-                list(retry._RETRYABLE_ERRNO)[0], 'message')
+                list(retry._RETRYABLE_ERRNO_MAXRETRY)[0], 'message')
         )
     )
     context.exception = ex
-
     assert er._should_retry(context)
 
     ex = requests.ConnectionError(
@@ -60,38 +54,51 @@ def test_should_retry():
         )
     )
     context.exception = ex
+    assert not er._should_retry(context)
 
+    # test malformed
+    ex = requests.ConnectionError(
+        urllib3.exceptions.ProtocolError()
+    )
+    context.exception = ex
+    assert not er._should_retry(context)
+
+    ex = requests.ConnectionError(
+        urllib3.exceptions.ProtocolError(
+            '({}, message)'.format(list(retry._RETRYABLE_ERRNO_PROTOCOL)[0])
+        )
+    )
+    context.exception = ex
+    assert er._should_retry(context)
+
+    ex = requests.ConnectionError(
+        urllib3.exceptions.ProtocolError('(N, message)')
+    )
+    context.exception = ex
     assert not er._should_retry(context)
 
     ex = requests.exceptions.ContentDecodingError()
     context.exception = ex
-
     assert er._should_retry(context)
 
     context.exception = None
     context.response.status = 200
-
     assert er._should_retry(context)
 
     context.response.status = 300
-
     assert not er._should_retry(context)
 
     context.response.status = 404
     context.location_mode = azure.storage.common.models.LocationMode.SECONDARY
-
     assert er._should_retry(context)
 
     context.response.status = 408
-
     assert er._should_retry(context)
 
     context.response.status = 500
-
     assert er._should_retry(context)
 
     context.response.status = 501
-
     assert not er._should_retry(context)