Close DB connection whilst making HTTP to SMS providers

At the moment, when we are processing and sending an SMS we open
a DB connection at the start of the celery task and then close it
at the end of the celery task. Nice and simple.

However, during that celery task we make an HTTP call out to our
SMS providers. If our SMS providers have problems or response times
start to slow then it means we have an open DB connection sat waiting
for our SMS providers to respond which could take seconds. If our
SMS providers grind to a halt, this would cause all of the
celery tasks to hold on to their connections and we would run out
of DB connections and Notify would fall over.

We think we can solve this by closing the DB session which releases
the DB connection back to the pool.

Note, we've seen this happen in staging during load testing if our
SMS provider stub has fallen over. We've never seen it in production
and it may be less unlikely to happen as we are balancing traffic
across two providers and they generally have very good uptime.

One downside to be aware of is there could be a slight increase in
time spent to send an SMS as we will now spend a bit of extra time
closing the DB session and then reopening it again after the HTTP
request is done.

Note, there is no reason this approach couldn't be copied for our
email provider too if it appears successful.

app/delivery/send_to_providers.py

@@ -10,7 +10,7 @@ from notifications_utils.template import (
     SMSMessageTemplate,
 )
 
-from app import create_uuid, notification_provider_clients, statsd_client
+from app import create_uuid, db, notification_provider_clients, statsd_client
 from app.celery.research_mode_tasks import (
     send_email_response,
     send_sms_response,
@@ -64,13 +64,21 @@ def send_sms_to_provider(notification):
 
         else:
             try:
-                provider.send_sms(
+                # End DB session here so that we don't have a connection stuck open waiting on the call
-                    to=notification.normalised_to,
+                # to one of the SMS providers
-                    content=str(template),
+                # We don't want to tie our DB connections being open to the performance of our SMS
-                    reference=str(notification.id),
+                # providers as a slow down of our providers can cause us to run out of DB connections
-                    sender=notification.reply_to_text,
+                # Therefore we pull all the data from our DB models into `send_sms_kwargs`now before
-                    international=notification.international
+                # closing the session (as otherwise it would be reopened immediately)
-                )
+                send_sms_kwargs = {
+                    'to': notification.normalised_to,
+                    'content': str(template),
+                    'reference': str(notification.id),
+                    'sender': notification.reply_to_text,
+                    'international': notification.international,
+                }
+                db.session.close()  # no commit needed as no changes to objects have been made above
+                provider.send_sms(**send_sms_kwargs)
             except Exception as e:
                 notification.billable_units = template.fragment_count
                 dao_update_notification(notification)

tests/app/delivery/test_send_to_providers.py

@@ -218,6 +218,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
     mocker.patch('app.mmg_client.send_sms')
 
     version_on_notification = sample_template.version
+    expected_template_id = sample_template.id
 
     # Change the template
     from app.dao.templates_dao import (
@@ -241,11 +242,13 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
         international=False
     )
 
+    t = dao_get_template_by_id(expected_template_id)
+
     persisted_notification = notifications_dao.get_notification_by_id(db_notification.id)
     assert persisted_notification.to == db_notification.to
-    assert persisted_notification.template_id == sample_template.id
+    assert persisted_notification.template_id == expected_template_id
     assert persisted_notification.template_version == version_on_notification
-    assert persisted_notification.template_version != sample_template.version
+    assert persisted_notification.template_version != t.version
     assert persisted_notification.status == 'sending'
     assert not persisted_notification.personalisation
 
@@ -349,6 +352,7 @@ def test_send_sms_should_use_service_sms_sender(
 
     sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456', is_default=False)
     db_notification = create_notification(template=sample_template, reply_to_text=sms_sender.sms_sender)
+    expected_sender_name = sms_sender.sms_sender
 
     send_to_providers.send_sms_to_provider(
         db_notification,
@@ -358,7 +362,7 @@ def test_send_sms_should_use_service_sms_sender(
         to=ANY,
         content=ANY,
         reference=ANY,
-        sender=sms_sender.sms_sender,
+        sender=expected_sender_name,
         international=False
     )
 
@@ -653,16 +657,6 @@ def test_should_send_sms_to_international_providers(
     get_provider_details_by_identifier('firetext').priority = 100
     get_provider_details_by_identifier('mmg').priority = 0
 
-    notification_uk = create_notification(
-        template=sample_template,
-        to_field="+447234123999",
-        personalisation={"name": "Jo"},
-        status='created',
-        international=False,
-        reply_to_text=sample_template.service.get_default_sms_sender(),
-        normalised_to="447234123999"
-    )
-
     notification_international = create_notification(
         template=sample_template,
         to_field="+6011-17224412",
@@ -672,17 +666,6 @@ def test_should_send_sms_to_international_providers(
         reply_to_text=sample_template.service.get_default_sms_sender(),
         normalised_to='601117224412'
     )
-    send_to_providers.send_sms_to_provider(
-        notification_uk
-    )
-
-    firetext_client.send_sms.assert_called_once_with(
-        to="447234123999",
-        content=ANY,
-        reference=str(notification_uk.id),
-        sender=current_app.config['FROM_NUMBER'],
-        international=False
-    )
 
     send_to_providers.send_sms_to_provider(
         notification_international
@@ -696,12 +679,48 @@ def test_should_send_sms_to_international_providers(
         international=True
     )
 
-    assert notification_uk.status == 'sending'
-    assert notification_uk.sent_by == 'firetext'
     assert notification_international.status == 'sent'
     assert notification_international.sent_by == 'mmg'
 
 
+def test_should_send_non_international_sms_to_default_provider(
+    sample_template,
+    sample_user,
+    mocker
+):
+    mocker.patch('app.mmg_client.send_sms')
+    mocker.patch('app.firetext_client.send_sms')
+
+    # set firetext to active
+    get_provider_details_by_identifier('firetext').priority = 100
+    get_provider_details_by_identifier('mmg').priority = 0
+
+    notification_uk = create_notification(
+        template=sample_template,
+        to_field="+447234123999",
+        personalisation={"name": "Jo"},
+        status='created',
+        international=False,
+        reply_to_text=sample_template.service.get_default_sms_sender(),
+        normalised_to="447234123999"
+    )
+
+    send_to_providers.send_sms_to_provider(
+        notification_uk
+    )
+
+    firetext_client.send_sms.assert_called_once_with(
+        to="447234123999",
+        content=ANY,
+        reference=str(notification_uk.id),
+        sender=current_app.config['FROM_NUMBER'],
+        international=False
+    )
+
+    assert notification_uk.status == 'sending'
+    assert notification_uk.sent_by == 'firetext'
+
+
 @pytest.mark.parametrize('sms_sender, expected_sender, prefix_sms, expected_content', [
     ('foo', 'foo', False, 'bar'),
     ('foo', 'foo', True, 'Sample service: bar'),