Clean up session sharing

* Test cases
* Fix premature session.close when caller
  is managing email backend connection
* Ensure session closed correct in exceptions
* Changelog

(Also fixes bug where JSON serialization errors
didn't respect fail_silently.)

djrill/mail/backends/djrill.py

@@ -66,41 +66,60 @@ class DjrillBackend(BaseEmailBackend):
         self.api_send_template = self.api_url + "/messages/send-template.json"
 
     def open(self):
-        if not self.session:
+        """
+        Ensure we have a requests Session to connect to the Mandrill API.
+        Returns True if a new session was created (and the caller must close it).
+        """
+        if self.session:
+            return False  # already exists
+
         try:
             self.session = requests.Session()
-                self.session.headers["User-Agent"] = "Djrill/%s %s" % (
-                    __version__, self.session.headers.get("User-Agent", ""))
-            except:
+        except requests.RequestException:
             if not self.fail_silently:
                 raise
-                return False
+        else:
+            self.session.headers["User-Agent"] = "Djrill/%s %s" % (
+                __version__, self.session.headers.get("User-Agent", ""))
             return True
 
+    def close(self):
+        """
+        Close the Mandrill API Session unconditionally.
 
-    def close(self, error=False):
-        if self.session:
+        (You should call this only if you called open and it returned True;
+        else someone else created the session and will clean it up themselves.)
+        """
+        if self.session is None:
+            return
         try:
             self.session.close()
-            except:
-                if not self.fail_silently and not error:
+        except requests.RequestException:
+            if not self.fail_silently:
                 raise
+        finally:
             self.session = None
 
     def send_messages(self, email_messages):
+        """
+        Sends one or more EmailMessage objects and returns the number of email
+        messages sent.
+        """
         if not email_messages:
             return 0
 
-        if not self.open():
-            return
+        created_session = self.open()
+        if not self.session:
+            return 0  # exception in self.open with fail_silently
 
         num_sent = 0
+        try:
             for message in email_messages:
                 sent = self._send(message)
-
                 if sent:
                     num_sent += 1
-
+        finally:
+            if created_session:
                 self.close()
 
         return num_sent
@@ -134,13 +153,14 @@ class DjrillBackend(BaseEmailBackend):
 
         except NotSupportedByMandrillError:
             if not self.fail_silently:
-                self.close(True)
                 raise
             return False
 
         try:
             api_data = json.dumps(api_params)
         except TypeError as err:
+            if self.fail_silently:
+                return False
             # Add some context to the "not JSON serializable" message
             if not err.args:
                 err.args = ('',)
@@ -148,7 +168,6 @@ class DjrillBackend(BaseEmailBackend):
                 err.args[0] + " in a Djrill message (perhaps it's a merge var?)."
                               " Try converting it to a string or number first.",
             ) + err.args[1:]
-            self.close(True)
             raise err
 
         response = self.session.post(api_url, data=api_data)
@@ -165,7 +184,6 @@ class DjrillBackend(BaseEmailBackend):
                         to['email'] for to in msg_dict.get('to', []) if 'email' in to)
                 if 'from_email' in msg_dict:
                     log_message += " from %s" % msg_dict['from_email']
-                self.close(True)
                 raise MandrillAPIError(
                     status_code=response.status_code,
                     response=response,
@@ -378,6 +396,7 @@ class DjrillBackend(BaseEmailBackend):
 
         # b64encode requires bytes, so let's convert our content.
         try:
+            # noinspection PyUnresolvedReferences
             if isinstance(content, unicode):
                 # Python 2.X unicode string
                 content = content.encode(str_encoding)

djrill/tests/__init__.py

@@ -1,6 +1,7 @@
 from djrill.tests.test_mandrill_send import *
 from djrill.tests.test_mandrill_send_template import *
-from djrill.tests.test_mandrill_webhook import *
+from djrill.tests.test_mandrill_session_sharing import *
 from djrill.tests.test_mandrill_subaccounts import *
+from djrill.tests.test_mandrill_webhook import *
 
 from djrill.tests.test_mandrill_integration import *

djrill/tests/test_mandrill_session_sharing.py

@@ -0,0 +1,72 @@
+from decimal import Decimal
+from mock import patch
+
+from django.core import mail
+
+from .mock_backend import DjrillBackendMockAPITestCase
+
+
+class DjrillSessionSharingTests(DjrillBackendMockAPITestCase):
+    """Test Djrill backend sharing of single Mandrill API connection"""
+
+    @patch('requests.Session.close', autospec=True)
+    def test_connection_sharing(self, mock_close):
+        """Djrill reuses one requests session when sending multiple messages"""
+        datatuple = (
+            ('Subject 1', 'Body 1', 'from@example.com', ['to@example.com']),
+            ('Subject 2', 'Body 2', 'from@example.com', ['to@example.com']),
+        )
+        mail.send_mass_mail(datatuple)
+        self.assertEqual(self.mock_post.call_count, 2)
+        session1 = self.mock_post.call_args_list[0][0]  # arg[0] (self) is session
+        session2 = self.mock_post.call_args_list[1][0]
+        self.assertEqual(session1, session2)
+        self.assertEqual(mock_close.call_count, 1)
+
+    @patch('requests.Session.close', autospec=True)
+    def test_caller_managed_connections(self, mock_close):
+        """Calling code can created long-lived connection that it opens and closes"""
+        connection = mail.get_connection()
+        connection.open()
+        mail.send_mail('Subject 1', 'body', 'from@example.com', ['to@example.com'], connection=connection)
+        session1 = self.mock_post.call_args[0]
+        self.assertEqual(mock_close.call_count, 0)  # shouldn't be closed yet
+
+        mail.send_mail('Subject 2', 'body', 'from@example.com', ['to@example.com'], connection=connection)
+        self.assertEqual(mock_close.call_count, 0)  # still shouldn't be closed
+        session2 = self.mock_post.call_args[0]
+        self.assertEqual(session1, session2)  # should have reused same session
+
+        connection.close()
+        self.assertEqual(mock_close.call_count, 1)
+
+    def test_session_closed_after_exception(self):
+        # fail loud case:
+        msg = mail.EmailMessage('Subject', 'Message', 'from@example.com', ['to1@example.com'],)
+        msg.global_merge_vars = {'PRICE': Decimal('19.99')}  # will cause JSON serialization error
+        with patch('requests.Session.close', autospec=True) as mock_close:
+            with self.assertRaises(TypeError):
+                msg.send()
+        self.assertEqual(mock_close.call_count, 1)
+
+        # fail silently case (EmailMessage caches backend on send, so must create new one):
+        msg = mail.EmailMessage('Subject', 'Message', 'from@example.com', ['to1@example.com'],)
+        msg.global_merge_vars = {'PRICE': Decimal('19.99')}
+        with patch('requests.Session.close', autospec=True) as mock_close:
+            sent = msg.send(fail_silently=True)
+            self.assertEqual(sent, 0)
+        self.assertEqual(mock_close.call_count, 1)
+
+        # caller-supplied connection case:
+        with patch('requests.Session.close', autospec=True) as mock_close:
+            connection = mail.get_connection()
+            connection.open()
+            msg = mail.EmailMessage('Subject', 'Message', 'from@example.com', ['to1@example.com'],
+                                    connection=connection)
+            msg.global_merge_vars = {'PRICE': Decimal('19.99')}
+            with self.assertRaises(TypeError):
+                msg.send()
+            self.assertEqual(mock_close.call_count, 0)  # wait for us to close it
+
+            connection.close()
+            self.assertEqual(mock_close.call_count, 1)

docs/history.rst

@@ -68,6 +68,15 @@ Removed DjrillBackendHTTPError
   with :exc:`djrill.MandrillAPIError`.
 
 
+Other Djrill 2.0 Changes
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+* Use a single HTTP connection to the Mandrill API to improve performance
+  when sending multiple messages at once using :func:`~django.core.mail.send_mass_mail`.
+  (You can also directly manage your own long-lived Djrill connection across multiple sends,
+  by calling open and close on :ref:`Django's email backend <django:topic-email-backends>`.)
+
+
 Older Releases
 --------------