Added HTTP Retrying logic. (#30)

* Added HTTP Retrying logic.

* Renamed constant and changed import order and removed redundant imports.

* Removed redundant dependencies in requirements_test.txt

* Include requirements in install_requires

* Remove requirements from tests_requires

Co-authored-by: Markus D. Herrmann <hackermd@users.noreply.github.com>

setup.py

@@ -64,5 +64,6 @@
     install_requires=[
         'pydicom>=1.0',
         'requests>=2.18',
+        'retrying>=1.3.3',
     ]
 )

src/dicomweb_client/api.py

@@ -7,6 +7,7 @@
 import xml.etree.ElementTree as ET
 from collections import OrderedDict
 from io import BytesIO
+from http import HTTPStatus
 from urllib.parse import quote_plus, urlparse
 from typing import (
     Any,
@@ -21,9 +22,10 @@
 )
 
 import requests
+import retrying
 import pydicom
 
-from dicomweb_client.error import DICOMJSONError, HTTPError
+from dicomweb_client.error import DICOMJSONError
 
 
 logger = logging.getLogger(__name__)
@@ -237,6 +239,64 @@ class DICOMwebClient(object):
 
     '''
 
+    # Default HTTP error codes to retry.
+    _DEFAULT_RETRIABLE_ERROR_CODES = (HTTPStatus.TOO_MANY_REQUESTS,
+                                      HTTPStatus.REQUEST_TIMEOUT,
+                                      HTTPStatus.SERVICE_UNAVAILABLE,
+                                      HTTPStatus.GATEWAY_TIMEOUT)
+
+    def set_http_retry_params(
+        self, retry: bool = True,
+        max_attempts: Optional[int] = 5,
+        wait_exponential_multiplier: Optional[int] = 1000,
+        retriable_error_codes: Optional[
+          Tuple[HTTPStatus, ...]] = _DEFAULT_RETRIABLE_ERROR_CODES) -> None:
+        '''Sets parameters for HTTP retrying logic. These parameters are passed
+        to @retrying.retry which wraps the HTTP requests and retries all
+        responses that return an error code defined in |retriable_error_codes|.
+        The retrying method uses exponential back off using the multiplier
+        |wait_exponential_multiplier| for a max attempts defined by
+        |max_attempts|.
+
+        Parameters
+        ----------
+        retry: bool, optional
+            whether HTTP retrying should be performed, if it is set to false,
+            the rest of the parameters are ignored.
+        max_attempts: int, optional
+            the maximum number of request attempts.
+        wait_exponential_multiplier: float, optional
+            exponential multiplier applied to delay between attempts in ms.
+        retriable_error_codes: tuple, optional
+            tuple of HTTP error codes to retry if raised.
+        '''
+        self._http_retry = retry
+        if retry:
+          self._max_attempts = max_attempts
+          self._wait_exponential_multiplier = wait_exponential_multiplier
+          self._http_retrable_errors = retriable_error_codes
+
+        else:
+          self._max_attempts = 1
+          self._wait_exponential_multiplier = 1
+          self._http_retrable_errors = ()
+
+    def _is_retriable_http_error(
+        self, response: requests.models.Response) -> bool:
+        '''Determines whether the given response's status code is retriable.
+
+        Parameters
+        ----------
+        response: requests.models.Response
+            HTTP response object returned by the request method.
+
+        Returns
+        -------
+        bool
+            Whether the HTTP request should be retried.
+        '''
+        return response.status_code in self._http_retrable_errors
+
     def __init__(
             self,
             url: str,
@@ -327,6 +387,7 @@ def __init__(
         if callback is not None:
             self._session.hooks = {'response': callback}
         self._chunk_size = chunk_size
+        self.set_http_retry_params()
 
     def _parse_qido_query_parameters(
             self,
@@ -602,20 +663,26 @@ def _http_get(
             HTTP response message
 
         '''
+        @retrying.retry(
+            retry_on_result=self._is_retriable_http_error,
+            wait_exponential_multiplier=self._wait_exponential_multiplier,
+            stop_max_attempt_number=self._max_attempts)
+        def _invoke_get_request(url: str,
+                                headers: Optional[Dict[str, str]] = None
+                               )-> requests.models.Response:
+            # Setting stream allows for retrieval of data in chunks using
+            # the iter_content() method
+            return self._session.get(url=url, headers=headers, stream=True)
+
         if headers is None:
             headers = {}
         if params is None:
             params = {}
         url += self._build_query_string(params)
         logger.debug('GET: {} {}'.format(url, headers))
-        # Setting stream allows for retrieval of data in chunks using
-        # the iter_content() method
-        response = self._session.get(url=url, headers=headers, stream=True)
-        try:
-            response.raise_for_status()
-        except requests.exceptions.HTTPError as error:
-            raise HTTPError(error)
+        response = _invoke_get_request(url, headers)
         logger.debug('request status code: {}'.format(response.status_code))
+        response.raise_for_status()
         if response.status_code == 204:
             logger.warning('empty response')
         # The server may not return all results, but rather include a warning
@@ -1335,28 +1402,30 @@ def serve_data_chunks(data):
                 end = offset + self._chunk_size
                 yield data[offset:end]
 
+        @retrying.retry(
+            retry_on_result=self._is_retriable_http_error,
+            wait_exponential_multiplier=self._wait_exponential_multiplier,
+            stop_max_attempt_number=self._max_attempts)
+        def _invoke_post_request(url: str, data: bytes,
+                                 headers: Optional[Dict[str, str]] = None
+                                )-> requests.models.Response:
+            return self._session.post(url, headers, data)
+
         if self._chunk_size is not None and len(data) > self._chunk_size:
             logger.info('store data in chunks using chunked transfer encoding')
             chunked_headers = dict(headers)
             chunked_headers['Transfer-Encoding'] = 'chunked'
             chunked_headers['Cache-Control'] = 'no-cache'
             chunked_headers['Connection'] = 'Keep-Alive'
             data_chunks = serve_data_chunks(data)
-            response = self._session.post(
-                url=url,
-                data=data_chunks,
-                headers=chunked_headers
-            )
+            response = _invoke_post_request(url, data_chunks, chunked_headers)
         else:
-            response = self._session.post(url=url, data=data, headers=headers)
+            response = _invoke_post_request(url, data, headers)
         logger.debug('request status code: {}'.format(response.status_code))
         try:
             response.raise_for_status()
-        except requests.exceptions.HTTPError as error:
-
-            raise HTTPError(error)
         except requests.exceptions.ConnectionError as error:
-            raise HTTPError(error[0])
+            raise requests.exceptions.HTTPError(error[0])
         if not response.ok:
             logger.warning('storage was not successful for all instances')
             payload = response.content

src/dicomweb_client/error.py

@@ -6,7 +6,3 @@ class DICOMJSONError(ValueError):
     '''Exception class for malformatted DICOM JSON.'''
     pass
 
-
-class HTTPError(requests.exceptions.HTTPError):
-    '''Exception class for HTTP requests with failure status codes.'''
-    pass

tests/test_api.py

@@ -2,9 +2,12 @@
 import json
 import xml.etree.ElementTree as ET
 from io import BytesIO
+from http import HTTPStatus
 
 import pytest
 import pydicom
+from requests.exceptions import HTTPError
+from retrying import RetryError
 
 from dicomweb_client.api import (
     DICOMwebClient,
@@ -78,6 +81,20 @@ def test_lookup_keyword(httpserver, client):
     assert client.lookup_keyword('00080018') == 'SOPInstanceUID'
     assert client.lookup_keyword('7FE00010') == 'PixelData'
 
+def test_set_http_retry_params(httpserver, client):
+    retry = True
+    retriable_error_codes = (HTTPStatus.TOO_MANY_REQUESTS,
+                             HTTPStatus.SERVICE_UNAVAILABLE)
+    max_attempts = 10
+    wait_exponential_multiplier = 100
+    client = DICOMwebClient(httpserver.url)
+    client.set_http_retry_params(retry, max_attempts,
+                                 wait_exponential_multiplier,
+                                 retriable_error_codes)
+    assert client._http_retry == retry
+    assert client._http_retrable_errors == retriable_error_codes
+    assert client._max_attempts == max_attempts
+    assert client._wait_exponential_multiplier == wait_exponential_multiplier
 
 def test_search_for_studies(httpserver, client, cache_dir):
     cache_filename = str(cache_dir.joinpath('search_for_studies.json'))
@@ -94,6 +111,23 @@ def test_search_for_studies(httpserver, client, cache_dir):
         for mime in request.accept_mimetypes
     )
 
+def test_search_for_studies_with_retries(httpserver, client, cache_dir):
+    headers = {'content-type': 'application/dicom+json'}
+    httpserver.serve_content(
+        content='', code=HTTPStatus.REQUEST_TIMEOUT, headers=headers)
+    with pytest.raises(RetryError):
+        client.search_for_studies()
+    assert len(httpserver.requests) == client._max_attempts
+
+def test_search_for_studies_with_no_retries(httpserver, client, cache_dir):
+    client.set_http_retry_params(retry=False)
+    headers = {'content-type': 'application/dicom+json'}
+    httpserver.serve_content(
+        content='', code=HTTPStatus.REQUEST_TIMEOUT, headers=headers)
+    with pytest.raises(HTTPError):
+        client.search_for_studies()
+    assert len(httpserver.requests) == 1
+
 
 def test_search_for_studies_qido_prefix(httpserver, client, cache_dir):
     client.qido_url_prefix = 'qidors'
@@ -550,6 +584,36 @@ def test_retrieve_instance_frames_rendered_png(httpserver, client, cache_dir):
     assert request.path == expected_path
     assert request.accept_mimetypes[0][0][:10] == headers['content-type'][:10]
 
+def test_store_instance_error_with_retries(httpserver, client, cache_dir):
+    value = ['2018-11-21']
+    dicom_json = {
+        '00080020': {
+            'vr': 'DA',
+            'Value': value
+        }
+    }
+    dataset = load_json_dataset(dicom_json)
+    httpserver.serve_content(
+        content='', code=HTTPStatus.REQUEST_TIMEOUT, headers='')
+    with pytest.raises(RetryError):
+        client.store_instances([dataset])
+    assert len(httpserver.requests) == client._max_attempts
+
+def test_store_instance_error_with_no_retries(httpserver, client, cache_dir):
+    value = ['2018-11-21']
+    dicom_json = {
+        '00080020': {
+            'vr': 'DA',
+            'Value': value
+        }
+    }
+    dataset = load_json_dataset(dicom_json)
+    client.set_http_retry_params(retry=False)
+    httpserver.serve_content(
+        content='', code=HTTPStatus.REQUEST_TIMEOUT, headers='')
+    with pytest.raises(HTTPError):
+        client.store_instances([dataset])
+    assert len(httpserver.requests) == 1
 
 def test_load_json_dataset_da(httpserver, client, cache_dir):
     value = ['2018-11-21']