Change api request retry strategy to use exponential backoff (#402)

Author: Cary Cheng

boxsdk/config.py

@@ -13,6 +13,7 @@ class API(object):
     UPLOAD_URL = 'https://upload.box.com/api/2.0'
     OAUTH2_API_URL = 'https://api.box.com/oauth2'  # <https://developers.box.com/docs/#oauth-2>
     OAUTH2_AUTHORIZE_URL = 'https://account.box.com/api/oauth2/authorize'  # <https://developers.box.com/docs/#oauth-2-authorize>
+    MAX_RETRY_ATTEMPTS = 5
 
 
 class Client(object):

boxsdk/session/session.py

@@ -2,6 +2,9 @@
 
 from __future__ import unicode_literals, absolute_import
 
+import random
+import math
+
 from functools import partial
 from logging import getLogger
 
@@ -17,6 +20,10 @@
 
 
 class Session(object):
+
+    _retry_randomization_factor = 0.5
+    _retry_base_interval = 1
+
     """
     Box API session. Provides automatic retry of failed requests.
     """
@@ -242,9 +249,13 @@ def with_default_network_request_kwargs(self, extra_network_parameters):
         kwargs['default_network_request_kwargs'].update(extra_network_parameters)
         return self.__class__(**kwargs)
 
+    # We updated our retry strategy to use exponential backoff instead of the header returned from the API response.
+    # This is something we can remove in latter major bumps.
+    # pylint: disable=unused-argument
     def _get_retry_after_time(self, attempt_number, retry_after_header):
         """
-        Get the amount of time to wait before retrying the API request.
+        Get the amount of time to wait before retrying the API request, using the attempt number that failed to
+        calculate the retry time for the next retry attempt.
 
         If the Retry-After header is supplied, use it; otherwise, use exponential backoff
         For 202 Accepted (thumbnail or file not ready) and 429 (too many requests), retry later, after a delay
@@ -258,10 +269,11 @@ def _get_retry_after_time(self, attempt_number, retry_after_header):
         :return:                        Number of seconds to wait before retrying.
         :rtype:                         `Number`
         """
-        # pylint:disable=no-self-use
-        if retry_after_header is not None:
-            return float(retry_after_header)
-        return 2 ** attempt_number
+        min_randomization = 1 - self._retry_randomization_factor
+        max_randomization = 1 + self._retry_randomization_factor
+        randomization = (random.uniform(0, 1) * (max_randomization - min_randomization)) + min_randomization
+        exponential = math.pow(2, attempt_number)
+        return exponential * self._retry_base_interval * randomization
 
     @staticmethod
     def _raise_on_unsuccessful_request(network_response, request):
@@ -362,7 +374,7 @@ def _prepare_and_send_request(
         while True:
             retry = self._get_retry_request_callable(network_response, attempt_number, request)
 
-            if retry is None or attempt_number >= 10:
+            if retry is None or attempt_number >= API.MAX_RETRY_ATTEMPTS:
                 break
 
             attempt_number += 1

test/functional/test_rate_limits.py

@@ -2,9 +2,12 @@
 
 from __future__ import unicode_literals
 
+from mock import patch
+
 
 def test_too_many_requests_causes_retry(box_client, mock_box, monkeypatch):
     monkeypatch.setattr(mock_box, 'RATE_LIMIT_THRESHOLD', 1)
-    box_client.folder('0').get()
-    box_client.folder('0').get()
+    with patch('random.uniform', return_value=1):
+        box_client.folder('0').get()
+        box_client.folder('0').get()
     assert len(mock_box.requests) == 6  # 3 auth requests, 2 real requests, and a retry

test/functional/test_recovery.py

@@ -66,7 +66,7 @@ def test_client_stops_retrying_after_10_server_errors(box_client, mock_box, erro
     with pytest.raises(BoxAPIException) as exc_info:
         box_client.folder('0').get()
         assert exc_info.value.status == error_code
-    assert len(mock_box.requests) == 14  # 3 auth requests, 1 try, and 10 retries
+    assert len(mock_box.requests) == 9  # 3 auth requests, 1 try, and 5 retries
 
 
 @pytest.mark.parametrize('chaos', [html, xml])

test/unit/session/test_session.py

@@ -6,7 +6,7 @@
 from io import IOBase
 from numbers import Number
 
-from mock import MagicMock, Mock, PropertyMock, call
+from mock import MagicMock, Mock, PropertyMock, call, patch
 import pytest
 
 from boxsdk.auth.oauth2 import OAuth2
@@ -145,11 +145,12 @@ def test_box_session_retries_response_after_retry_after(
     mock_network_layer.request.side_effect = [retry_after_response, generic_successful_response]
     mock_network_layer.retry_after.side_effect = lambda delay, request, *args, **kwargs: request(*args, **kwargs)
 
-    box_response = test_method(box_session, url=test_url)
+    with patch('random.uniform', return_value=0.68):
+        box_response = test_method(box_session, url=test_url)
     assert box_response.status_code == 200
     assert len(mock_network_layer.retry_after.call_args_list) == 1
     assert isinstance(mock_network_layer.retry_after.call_args[0][0], Number)
-    assert mock_network_layer.retry_after.call_args[0][0] == float(retry_after_response.headers['Retry-After'])
+    assert round(mock_network_layer.retry_after.call_args[0][0], 4) == 1.18
 
 
 @pytest.mark.parametrize('test_method', [
@@ -172,16 +173,17 @@ def test_box_session_retries_request_after_server_error(
     mock_network_layer.request.side_effect = [server_error_response, server_error_response, generic_successful_response]
     mock_network_layer.retry_after.side_effect = lambda delay, request, *args, **kwargs: request(*args, **kwargs)
 
-    box_response = test_method(box_session, url=test_url)
+    with patch('random.uniform', return_value=0.68):
+        box_response = test_method(box_session, url=test_url)
     assert box_response.status_code == 200
     assert box_response.json() == generic_successful_response.json()
     assert box_response.ok == generic_successful_response.ok
     assert box_response.content == generic_successful_response.content
     assert len(mock_network_layer.retry_after.call_args_list) == 2
     assert isinstance(mock_network_layer.retry_after.call_args_list[0][0][0], Number)
     assert isinstance(mock_network_layer.retry_after.call_args_list[1][0][0], Number)
-    assert mock_network_layer.retry_after.call_args_list[0][0][0] == 1
-    assert mock_network_layer.retry_after.call_args_list[1][0][0] == 2
+    assert round(mock_network_layer.retry_after.call_args_list[0][0][0], 4) == 1.18
+    assert round(mock_network_layer.retry_after.call_args_list[1][0][0], 4) == 2.36
 
 
 def test_box_session_seeks_file_after_retry(box_session, mock_network_layer, server_error_response, generic_successful_response, test_url):
@@ -274,3 +276,20 @@ def test_session_uses_local_config(box_session, mock_network_layer, generic_succ
     box_session.api_config.BASE_API_URL = example_dot_com
     monkeypatch.setattr(API, 'BASE_API_URL', 'https://api.box.com')
     assert example_dot_com in box_session.get_url('foo', 'bar')
+
+
+@pytest.mark.parametrize(
+    'attempt_number,retry_after_header,expected_result',
+    [
+        (0, '', 1.18),
+        (1, '', 2.36),
+        (2, '', 4.72),
+        (3, '', 9.44),
+        (4, '', 18.88),
+    ]
+)
+def test_get_retry_after_time(box_session, attempt_number, retry_after_header, expected_result):
+    with patch('random.uniform', return_value=0.68):
+        retry_time = box_session._get_retry_after_time(attempt_number, retry_after_header)  # pylint: disable=protected-access
+    retry_time = round(retry_time, 4)
+    assert retry_time == expected_result