Merge pull request #888 from alphagov/throw-exception-on-long-running-client-call

Throw exception on long running client call

app/celery/research_mode_tasks.py

@@ -55,7 +55,8 @@ def make_request(notification_type, provider, data, headers):
             "POST",
             api_call,
             headers=headers,
-            data=data
+            data=data,
+            timeout=60
         )
         response.raise_for_status()
     except RequestException as e:

app/clients/sms/firetext.py

@@ -42,8 +42,10 @@ def get_firetext_responses(status):
 
 class FiretextClientResponseException(SmsClientResponseException):
     def __init__(self, response, exception):
-        self.status_code = response.status_code
+        status_code = response.status_code if response is not None else 504
-        self.text = response.text
+        text = response.text if response is not None else "Gateway Time-out"
+        self.status_code = status_code
+        self.text = text
         self.exception = exception
 
     def __str__(self):
@@ -68,11 +70,13 @@ class FiretextClient(SmsClient):
         return self.name
 
     def record_outcome(self, success, response):
+        status_code = response.status_code if response else 503
+
         log_message = "API {} request {} on {} response status_code {}".format(
             "POST",
             "succeeded" if success else "failed",
             self.url,
-            response.status_code
+            status_code
         )
 
         if success:
@@ -97,7 +101,8 @@ class FiretextClient(SmsClient):
             response = request(
                 "POST",
                 self.url,
-                data=data
+                data=data,
+                timeout=60
             )
             response.raise_for_status()
             try:

app/clients/sms/mmg.py

@@ -1,7 +1,6 @@
 import json
 from monotonic import monotonic
 from requests import (request, RequestException)
-
 from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE)
 from app.clients.sms import (SmsClient, SmsClientResponseException)
 
@@ -45,8 +44,11 @@ def get_mmg_responses(status):
 
 class MMGClientResponseException(SmsClientResponseException):
     def __init__(self, response, exception):
-        self.status_code = response.status_code
+        status_code = response.status_code if response is not None else 504
-        self.text = response.text
+        text = response.text if response is not None else "Gateway Time-out"
+
+        self.status_code = status_code
+        self.text = text
         self.exception = exception
 
     def __str__(self):
@@ -68,11 +70,12 @@ class MMGClient(SmsClient):
         self.mmg_url = current_app.config.get('MMG_URL')
 
     def record_outcome(self, success, response):
+        status_code = response.status_code if response else 503
         log_message = "API {} request {} on {} response status_code {}".format(
             "POST",
             "succeeded" if success else "failed",
             self.mmg_url,
-            response.status_code
+            status_code
         )
 
         if success:
@@ -104,7 +107,8 @@ class MMGClient(SmsClient):
                 headers={
                     'Content-Type': 'application/json',
                     'Authorization': 'Basic {}'.format(self.api_key)
-                }
+                },
+                timeout=60
             )
 
             response.raise_for_status()

tests/app/clients/test_firetext.py

@@ -1,10 +1,11 @@
 from requests import HTTPError
 from urllib.parse import parse_qs
+from requests.exceptions import ConnectTimeout, ReadTimeout
 
 import pytest
 import requests_mock
 
-from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException
+from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException, FiretextClientResponseException
 
 
 def test_should_return_correct_details_for_delivery():
@@ -126,3 +127,25 @@ def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetex
 
     request_args = parse_qs(request_mock.request_history[0].text)
     assert request_args['from'][0] == 'fromservice'
+
+
+def test_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client):
+    to = content = reference = 'foo'
+
+    with pytest.raises(FiretextClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://www.firetext.co.uk/api/sendsms/json', exc=ConnectTimeout)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'
+
+
+def test_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firetext_client):
+    to = content = reference = 'foo'
+
+    with pytest.raises(FiretextClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://www.firetext.co.uk/api/sendsms/json', exc=ReadTimeout)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_mmg.py

@@ -3,11 +3,12 @@ import json
 import pytest
 import requests_mock
 from requests import HTTPError
+from requests.exceptions import ConnectTimeout, ReadTimeout
 
 from app import mmg_client
 from app.clients.sms import SmsClientResponseException
 
-from app.clients.sms.mmg import get_mmg_responses
+from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException
 
 
 def test_should_return_correct_details_for_delivery():
@@ -113,3 +114,25 @@ def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
     assert 'app.clients.sms.mmg.MMGClientResponseException: Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc)  # noqa
     assert exc.value.status_code == 200
     assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'
+
+
+def test_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
+    to = content = reference = 'foo'
+
+    with pytest.raises(MMGClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://api.mmg.co.uk/json/api.php', exc=ConnectTimeout)
+        mmg_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'
+
+
+def test_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
+    to = content = reference = 'foo'
+
+    with pytest.raises(MMGClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://api.mmg.co.uk/json/api.php', exc=ReadTimeout)
+        mmg_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'