Fixed retries for HTTP Errors
The code does not properly handle situation when server returns http error. Also added option 'retries_delay' to specify timeout between retries Change-Id: I2908f5d774d6d388085e48a965e5888773578530 Closes-Bug: #1539025
This commit is contained in:
parent
aff85a919e
commit
fb3f8ffe02
|
@ -37,7 +37,7 @@ class Configuration(object):
|
|||
"""The configuration holder."""
|
||||
|
||||
def __init__(self, http_proxy=None, https_proxy=None,
|
||||
retries_num=0, threads_num=0,
|
||||
retries_num=0, retry_interval=0, threads_num=0,
|
||||
ignore_errors_num=0):
|
||||
"""Initialises.
|
||||
|
||||
|
@ -46,6 +46,7 @@ class Configuration(object):
|
|||
:param https_proxy: the url of proxy for connections over https,
|
||||
no-proxy will be used if it is not specified
|
||||
:param retries_num: the number of retries on errors
|
||||
:param retry_interval: the minimal time between retries (in seconds)
|
||||
:param threads_num: the max number of active threads
|
||||
:param ignore_errors_num: the number of errors that may occurs
|
||||
before stop processing
|
||||
|
@ -55,6 +56,7 @@ class Configuration(object):
|
|||
self.https_proxy = https_proxy
|
||||
self.ignore_errors_num = ignore_errors_num
|
||||
self.retries_num = retries_num
|
||||
self.retry_interval = retry_interval
|
||||
self.threads_num = threads_num
|
||||
|
||||
|
||||
|
@ -69,7 +71,8 @@ class Context(object):
|
|||
self._connection = ConnectionsManager(
|
||||
proxy=config.http_proxy,
|
||||
secure_proxy=config.https_proxy,
|
||||
retries_num=config.retries_num
|
||||
retries_num=config.retries_num,
|
||||
retry_interval=config.retry_interval
|
||||
)
|
||||
self._threads_num = config.threads_num
|
||||
self._ignore_errors_num = config.ignore_errors_num
|
||||
|
|
|
@ -50,6 +50,13 @@ class Application(app.App):
|
|||
metavar="NUMBER",
|
||||
help="The number of retries."
|
||||
)
|
||||
parser.add_argument(
|
||||
"--retry-interval",
|
||||
type=int,
|
||||
default=1,
|
||||
metavar="SECONDS",
|
||||
help="The minimal time between retries in seconds."
|
||||
)
|
||||
parser.add_argument(
|
||||
"--threads-num",
|
||||
default=3,
|
||||
|
|
|
@ -40,10 +40,25 @@ class RangeError(urlerror.URLError):
|
|||
|
||||
|
||||
class RetryableRequest(urllib.Request):
|
||||
MAX_INTERVAL = 5
|
||||
|
||||
offset = 0
|
||||
retries_left = 1
|
||||
retry_interval = 0
|
||||
start_time = 0
|
||||
|
||||
def get_retry_interval(self):
|
||||
"""Calculates progressive retry interval in seconds.
|
||||
|
||||
:return: the time to wait before start retry
|
||||
"""
|
||||
# we uses progressive timeout between retries,
|
||||
# the greatest number of retry will have greatest timeout
|
||||
# but limited with MAX_INTERVAL
|
||||
coef = max(self.MAX_INTERVAL - self.retries_left, 1)
|
||||
interval = self.retry_interval * coef
|
||||
return min(interval, self.MAX_INTERVAL)
|
||||
|
||||
|
||||
class ResumableResponse(StreamWrapper):
|
||||
"""The http-response wrapper to add resume ability.
|
||||
|
@ -83,7 +98,11 @@ class RetryHandler(urllib.BaseHandler):
|
|||
|
||||
@staticmethod
|
||||
def http_request(request):
|
||||
"""Initialises http request."""
|
||||
"""Initialises http request.
|
||||
|
||||
:param request: the instance of RetryableRequest
|
||||
:return: the request
|
||||
"""
|
||||
logger.debug("start request: %s", request.get_full_url())
|
||||
if request.offset > 0:
|
||||
request.add_header('Range', 'bytes=%d-' % request.offset)
|
||||
|
@ -94,46 +113,59 @@ class RetryHandler(urllib.BaseHandler):
|
|||
"""Wraps response in a ResumableResponse.
|
||||
|
||||
Checks that partial request completed successfully.
|
||||
:param request: the instance of RetryableRequest
|
||||
:param response: the response object
|
||||
:return: ResumableResponse if success otherwise same response
|
||||
"""
|
||||
code, msg = response.getcode(), response.msg
|
||||
# the server should response partial content if range is specified
|
||||
if request.offset > 0 and code != http_client.PARTIAL_CONTENT:
|
||||
raise RangeError(msg)
|
||||
|
||||
if code >= 400:
|
||||
logger.error(
|
||||
"request failed: %s - %d(%s), retries left - %d.",
|
||||
request.get_full_url(), code, msg, request.retries_left - 1
|
||||
)
|
||||
if is_retryable_http_error(code) and request.retries_left > 0:
|
||||
time.sleep(request.get_retry_interval())
|
||||
request.retries_left -= 1
|
||||
response = self.parent.open(request)
|
||||
# pass response to next handler as is.
|
||||
return response
|
||||
|
||||
logger.debug(
|
||||
"finish request: %s - %d (%s), duration - %d ms.",
|
||||
"request completed: %s - %d (%s), duration - %d ms.",
|
||||
request.get_full_url(), response.getcode(), response.msg,
|
||||
int((time.time() - request.start_time) * 1000)
|
||||
)
|
||||
if request.offset > 0 and response.getcode() != 206:
|
||||
raise RangeError("Server does not support ranges.")
|
||||
return ResumableResponse(request, response, self.parent)
|
||||
|
||||
def http_error(self, req, fp, code, msg, hdrs):
|
||||
"""Checks error code and retries request if it is allowed."""
|
||||
logger.error(
|
||||
"fail request: %s - %d(%s), retries left - %d.",
|
||||
req.get_full_url(), code, msg, req.retries_left
|
||||
)
|
||||
if req.retries_left > 0 and is_retryable_http_error(code):
|
||||
req.retries_left -= 1
|
||||
return self.parent.open(req)
|
||||
return ResumableResponse(request, response, self.parent)
|
||||
|
||||
https_request = http_request
|
||||
https_response = http_response
|
||||
|
||||
|
||||
def is_retryable_http_error(code):
|
||||
"""Checks that http error can be retried."""
|
||||
return code == http_client.NOT_FOUND or \
|
||||
code >= http_client.INTERNAL_SERVER_ERROR
|
||||
"""Checks that http error can be retried.
|
||||
|
||||
:param code: the HTTP_CODE
|
||||
:return: True if request can be retried otherwise False
|
||||
"""
|
||||
return code >= http_client.INTERNAL_SERVER_ERROR
|
||||
|
||||
|
||||
class ConnectionsManager(object):
|
||||
"""The connections manager."""
|
||||
|
||||
def __init__(self, proxy=None, secure_proxy=None, retries_num=0):
|
||||
def __init__(self, proxy=None, secure_proxy=None,
|
||||
retries_num=0, retry_interval=0):
|
||||
"""Initialises.
|
||||
|
||||
:param proxy: the url of proxy for http-connections
|
||||
:param secure_proxy: the url of proxy for https-connections
|
||||
:param retries_num: the number of allowed retries
|
||||
:param retry_interval: the minimal time between retries (in seconds)
|
||||
"""
|
||||
if proxy:
|
||||
proxies = {
|
||||
|
@ -144,6 +176,7 @@ class ConnectionsManager(object):
|
|||
proxies = None
|
||||
|
||||
self.retries_num = retries_num
|
||||
self.retry_interval = retry_interval
|
||||
self.opener = urllib.build_opener(
|
||||
RetryHandler(),
|
||||
urllib.ProxyHandler(proxies)
|
||||
|
@ -163,6 +196,7 @@ class ConnectionsManager(object):
|
|||
|
||||
request = RetryableRequest(url)
|
||||
request.retries_left = self.retries_num
|
||||
request.retry_interval = self.retry_interval
|
||||
request.offset = offset
|
||||
return request
|
||||
|
||||
|
@ -188,6 +222,7 @@ class ConnectionsManager(object):
|
|||
"Failed to open url - %s: %s. retries left - %d.",
|
||||
url, six.text_type(e), request.retries_left
|
||||
)
|
||||
time.sleep(request.get_retry_interval())
|
||||
|
||||
def retrieve(self, url, filename, **attributes):
|
||||
"""Downloads remote file.
|
||||
|
|
|
@ -42,6 +42,7 @@ class TestCliCommands(base.TestCase):
|
|||
"--ignore-errors-num=3",
|
||||
"--threads-num=8",
|
||||
"--retries-num=10",
|
||||
"--retry-interval=1",
|
||||
"--http-proxy=http://proxy",
|
||||
"--https-proxy=https://proxy"
|
||||
]
|
||||
|
@ -78,6 +79,7 @@ class TestCliCommands(base.TestCase):
|
|||
self.assertEqual(3, config.ignore_errors_num)
|
||||
self.assertEqual(8, config.threads_num)
|
||||
self.assertEqual(10, config.retries_num)
|
||||
self.assertEqual(1, config.retry_interval)
|
||||
|
||||
def test_clone_cmd(self, api_mock, read_file_mock, stdout_mock):
|
||||
read_file_mock.side_effect = [
|
||||
|
|
|
@ -98,6 +98,29 @@ class TestConnectionManager(base.TestCase):
|
|||
"/test/file", "I/O error", 0
|
||||
)
|
||||
|
||||
@mock.patch.multiple(
|
||||
"packetary.library.connections.urllib.HTTPHandler",
|
||||
http_request=mock.DEFAULT,
|
||||
http_open=mock.DEFAULT
|
||||
)
|
||||
def test_retries_on_50x(self, logger, http_open, http_request):
|
||||
request = connections.RetryableRequest("http:///localhost/file1.txt")
|
||||
request.retries_left = 1
|
||||
http_request.return_value = request
|
||||
response_mock = mock.MagicMock(code=501, msg="not found")
|
||||
response_mock.getcode.return_value = response_mock.code
|
||||
http_open.return_value = response_mock
|
||||
manager = connections.ConnectionsManager(retries_num=2)
|
||||
with self.assertRaises(connections.urlerror.HTTPError) as trapper:
|
||||
manager.open_stream("http:///localhost/file1.txt")
|
||||
self.assertEqual(501, trapper.exception.code)
|
||||
self.assertEqual(2, http_request.call_count)
|
||||
for retry_num in six.moves.range(1):
|
||||
logger.error.assert_any_call(
|
||||
"request failed: %s - %d(%s), retries left - %d.",
|
||||
mock.ANY, 501, mock.ANY, retry_num
|
||||
)
|
||||
|
||||
@mock.patch("packetary.library.connections.urllib.build_opener")
|
||||
def test_raise_other_errors(self, *_):
|
||||
manager = connections.ConnectionsManager()
|
||||
|
@ -109,6 +132,23 @@ class TestConnectionManager(base.TestCase):
|
|||
|
||||
self.assertEqual(1, manager.opener.open.call_count)
|
||||
|
||||
@mock.patch("packetary.library.connections.urllib.build_opener")
|
||||
@mock.patch("packetary.library.connections.time")
|
||||
def test_progressive_delay_between_request(self, time_mock, *_):
|
||||
manager = connections.ConnectionsManager(
|
||||
retries_num=6, retry_interval=1
|
||||
)
|
||||
manager.opener.open.side_effect = IOError("I/O Error")
|
||||
|
||||
with self.assertRaises(IOError):
|
||||
manager.open_stream("/test/file")
|
||||
|
||||
self.assertEqual(7, manager.opener.open.call_count)
|
||||
self.assertEqual(
|
||||
[1, 1, 2, 3, 4, 5],
|
||||
[x[0][0] for x in time_mock.sleep.call_args_list]
|
||||
)
|
||||
|
||||
@mock.patch("packetary.library.connections.urllib.build_opener")
|
||||
@mock.patch("packetary.library.connections.ensure_dir_exist")
|
||||
@mock.patch("packetary.library.connections.os")
|
||||
|
@ -198,7 +238,7 @@ class TestRetryHandler(base.TestCase):
|
|||
r = self.handler.http_response(request, response)
|
||||
self.assertIsInstance(r, connections.ResumableResponse)
|
||||
logger.debug.assert_called_with(
|
||||
"finish request: %s - %d (%s), duration - %d ms.",
|
||||
"request completed: %s - %d (%s), duration - %d ms.",
|
||||
"/file/test", 200, "test", 10
|
||||
)
|
||||
|
||||
|
@ -214,20 +254,18 @@ class TestRetryHandler(base.TestCase):
|
|||
response.getcode.return_value = 206
|
||||
self.handler.http_response(request, response)
|
||||
|
||||
def test_error(self, logger):
|
||||
request = mock.MagicMock()
|
||||
def test_handle_error(self, logger):
|
||||
request = mock.MagicMock(retries_left=1, retry_interval=0, offset=0)
|
||||
request.get_retry_interval.return_value = 0
|
||||
request.get_full_url.return_value = "/test"
|
||||
request.retries_left = 1
|
||||
self.handler.http_error(
|
||||
request, mock.MagicMock(), 500, "error", mock.MagicMock()
|
||||
)
|
||||
response_mock = mock.MagicMock(code=500, msg="error")
|
||||
response_mock.getcode.return_value = response_mock.code
|
||||
self.handler.http_response(request, response_mock)
|
||||
logger.error.assert_called_with(
|
||||
"fail request: %s - %d(%s), retries left - %d.",
|
||||
"/test", 500, "error", 1
|
||||
)
|
||||
self.handler.http_error(
|
||||
request, mock.MagicMock(), 404, "error", mock.MagicMock()
|
||||
"request failed: %s - %d(%s), retries left - %d.",
|
||||
"/test", 500, "error", 0
|
||||
)
|
||||
self.handler.http_response(request, response_mock)
|
||||
self.handler.parent.open.assert_called_once_with(request)
|
||||
|
||||
|
||||
|
|
|
@ -59,13 +59,15 @@ class TestRepositoryApi(base.TestCase):
|
|||
def test_create_with_config(self, connection_mock, controller_mock):
|
||||
config = Configuration(
|
||||
http_proxy="http://localhost", https_proxy="https://localhost",
|
||||
retries_num=10, threads_num=8, ignore_errors_num=6
|
||||
retries_num=10, retry_interval=1, threads_num=8,
|
||||
ignore_errors_num=6
|
||||
)
|
||||
RepositoryApi.create(config, "deb", "x86_64")
|
||||
connection_mock.assert_called_once_with(
|
||||
proxy="http://localhost",
|
||||
secure_proxy="https://localhost",
|
||||
retries_num=10
|
||||
retries_num=10,
|
||||
retry_interval=1
|
||||
)
|
||||
controller_mock.load.assert_called_once_with(
|
||||
mock.ANY, "deb", "x86_64"
|
||||
|
@ -76,14 +78,16 @@ class TestRepositoryApi(base.TestCase):
|
|||
def test_create_with_context(self, connection_mock, controller_mock):
|
||||
config = Configuration(
|
||||
http_proxy="http://localhost", https_proxy="https://localhost",
|
||||
retries_num=10, threads_num=8, ignore_errors_num=6
|
||||
retries_num=10, retry_interval=1, threads_num=8,
|
||||
ignore_errors_num=6
|
||||
)
|
||||
context = Context(config)
|
||||
RepositoryApi.create(context, "deb", "x86_64")
|
||||
connection_mock.assert_called_once_with(
|
||||
proxy="http://localhost",
|
||||
secure_proxy="https://localhost",
|
||||
retries_num=10
|
||||
retries_num=10,
|
||||
retry_interval=1
|
||||
)
|
||||
controller_mock.load.assert_called_once_with(
|
||||
context, "deb", "x86_64"
|
||||
|
@ -212,6 +216,7 @@ class TestContext(base.TestCase):
|
|||
threads_num=2,
|
||||
ignore_errors_num=3,
|
||||
retries_num=5,
|
||||
retry_interval=10,
|
||||
http_proxy="http://localhost",
|
||||
https_proxy="https://localhost"
|
||||
)
|
||||
|
@ -222,7 +227,8 @@ class TestContext(base.TestCase):
|
|||
conn_manager.assert_called_once_with(
|
||||
proxy="http://localhost",
|
||||
secure_proxy="https://localhost",
|
||||
retries_num=5
|
||||
retries_num=5,
|
||||
retry_interval=10
|
||||
)
|
||||
|
||||
self.assertIs(
|
||||
|
|
Loading…
Reference in New Issue