From 57bd6494e1c4e5042fd31a556de0076544997bec Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 4 Apr 2017 16:05:25 +0100 Subject: [PATCH 1/5] WIP - adding request timeout --- app/celery/research_mode_tasks.py | 3 ++- app/clients/sms/firetext.py | 3 ++- app/clients/sms/mmg.py | 8 ++++++-- app/notifications/notifications_sms_callback.py | 2 ++ 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index d7ecb399e..274fa3fe4 100644 --- a/app/celery/research_mode_tasks.py +++ b/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=5 ) response.raise_for_status() except RequestException as e: diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 911c88bda..17e09e967 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -97,7 +97,8 @@ class FiretextClient(SmsClient): response = request( "POST", self.url, - data=data + data=data, + timeout=60 ) response.raise_for_status() try: diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 642a247a9..7e3a18e47 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -1,7 +1,7 @@ import json from monotonic import monotonic from requests import (request, RequestException) - +from requests.exceptions import HTTPError from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE) from app.clients.sms import (SmsClient, SmsClientResponseException) @@ -104,7 +104,8 @@ class MMGClient(SmsClient): headers={ 'Content-Type': 'application/json', 'Authorization': 'Basic {}'.format(self.api_key) - } + }, + timeout=60 ) response.raise_for_status() @@ -117,6 +118,9 @@ class MMGClient(SmsClient): except RequestException as e: self.record_outcome(False, e.response) raise MMGClientResponseException(response=e.response, exception=e) + except HTTPError as e: + self.record_outcome(False, e.response) + raise MMGClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time self.statsd_client.timing("clients.mmg.request-time", elapsed_time) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 3c569e158..c1eacdee4 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -11,6 +11,8 @@ register_errors(sms_callback_blueprint) @sms_callback_blueprint.route('/mmg', methods=['POST']) def process_mmg_response(): + from time import sleep + sleep(20) client_name = 'MMG' data = json.loads(request.data) errors = validate_callback_data(data=data, From 72ecca4dab544fb59e29bc213d4e815ba37c0316 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 15:31:37 +0100 Subject: [PATCH 2/5] Added a 60 second timeout to the MMG and fire text clients. - This is a CONNECT and a READ timeout. - Gets wrapped in the standard client exception, with a status code of 504, message Gateway timeout. - Is quiet noisy in logs to allow us to see it - Ensures we flick across the provider. To test change the timeout to 0 and it will timeout. --- app/clients/sms/firetext.py | 10 +++++++--- app/clients/sms/mmg.py | 14 +++++++------- tests/app/clients/test_firetext.py | 25 ++++++++++++++++++++++++- tests/app/clients/test_mmg.py | 25 ++++++++++++++++++++++++- 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 17e09e967..e97091c0b 100644 --- a/app/clients/sms/firetext.py +++ b/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 - self.text = response.text + status_code = response.status_code if response is not None else 504 + 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: diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 7e3a18e47..3c8cf6338 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -1,7 +1,6 @@ import json from monotonic import monotonic from requests import (request, RequestException) -from requests.exceptions import HTTPError 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 - self.text = response.text + status_code = response.status_code if response is not None else 504 + 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: @@ -118,9 +121,6 @@ class MMGClient(SmsClient): except RequestException as e: self.record_outcome(False, e.response) raise MMGClientResponseException(response=e.response, exception=e) - except HTTPError as e: - self.record_outcome(False, e.response) - raise MMGClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time self.statsd_client.timing("clients.mmg.request-time", elapsed_time) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 5685db48b..4251f89bb 100644 --- a/tests/app/clients/test_firetext.py +++ b/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_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_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' diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index d87f2f914..2b4ce7bd8 100644 --- a/tests/app/clients/test_mmg.py +++ b/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_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_firetext_rejects_with_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' From 43a31fa49771dac5f653044fdd9f29918ad83c1f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 15:33:02 +0100 Subject: [PATCH 3/5] Removed a stray sleep used for testing. --- app/notifications/notifications_sms_callback.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index c1eacdee4..3c569e158 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -11,8 +11,6 @@ register_errors(sms_callback_blueprint) @sms_callback_blueprint.route('/mmg', methods=['POST']) def process_mmg_response(): - from time import sleep - sleep(20) client_name = 'MMG' data = json.loads(request.data) errors = validate_callback_data(data=data, From c8a26fcd4050348792243812edb62dd1ca8a371f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 16:30:53 +0100 Subject: [PATCH 4/5] Fixed test names --- tests/app/clients/test_firetext.py | 4 ++-- tests/app/clients/test_mmg.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 4251f89bb..1edcab564 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -129,7 +129,7 @@ def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetex assert request_args['from'][0] == 'fromservice' -def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock, mock_firetext_client): +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: @@ -140,7 +140,7 @@ def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock, mock_firetext_c assert exc.value.text == 'Gateway Time-out' -def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock, mock_firetext_client): +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: diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 2b4ce7bd8..73bbc012e 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -116,7 +116,7 @@ def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' -def test_send_sms_raises_if_mmg_rejects_with_timeout(rmock): +def test_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock): to = content = reference = 'foo' with pytest.raises(MMGClientResponseException) as exc: @@ -127,7 +127,7 @@ def test_send_sms_raises_if_mmg_rejects_with_timeout(rmock): assert exc.value.text == 'Gateway Time-out' -def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock): +def test_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock): to = content = reference = 'foo' with pytest.raises(MMGClientResponseException) as exc: From bce75ae0db31c2d1bf6d5b8791f1df1a2e433285 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 16:39:46 +0100 Subject: [PATCH 5/5] Bump timeout on research mode to match normal behaviour --- app/celery/research_mode_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 274fa3fe4..ced35b1d4 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -56,7 +56,7 @@ def make_request(notification_type, provider, data, headers): api_call, headers=headers, data=data, - timeout=5 + timeout=60 ) response.raise_for_status() except RequestException as e: