Merge lp:~thumper/launchpad/catch-email-send-failures into lp:launchpad

[Tim Penhey]

[Tim Penhey (thumper)]

= Summary =

Found partly due to tracking down why a MySQL branch subscriber failed to get
an email about a new merge proposal. One of the subscribers was going to be
getting a very large diff, which caused the SMTP server to barf. This caused
no emails to be sent out, and the proposal wasn't created.

Also fixes: https://bugs.edge.launchpad.net/launchpad-code/+bug/139071

== Proposed fix ==

Check in the BaseMailer.sendAll method to make sure we catch and handle
STMPExceptions.

One of the main reasons we get this message is due to a size problem with the
attachments, so we add a plain text attachment that informs the user that we
didn't attach a large one and try again.

== Tests ==

TestBaseMailer

In order to simulate SMTP send failures, I had to create a new test mail
controller, and instead of messing with metaclasses, used a factory that is
passed in to override the default MailController class used by the BaseMailer.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/services/mail/tests/test_basemailer.py
  lib/lp/services/mail/basemailer.py

[Abel Deuring (adeuring)]

Hi Tim,

a nice branch. Just two nitpicks.

Abel

+ def test_generateEmail_force_no_attachments(self):
+ # If BaseMailer.getnerateEmail is called with

s/getnerateEmail/generateEmail

+ def test_sendall_single_failure_doesnt_kill_all(self):
+ # A failure to send to a particular email address doesn't stop ending
+ # to others.

s/ending/sending/

review: Approve

=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py	2009-07-17 00:26:05 +0000
+++ lib/lp/services/mail/basemailer.py	2009-09-04 05:02:48 +0000
@@ -7,12 +7,15 @@
 
 __all__ = ['BaseMailer']
 
+import logging
+from smtplib import SMTPException
 
 from canonical.launchpad.helpers import get_email_template
-from canonical.launchpad.mail import format_address, MailController
 from canonical.launchpad.mailout import text_delta
+
 from lp.services.mail.notificationrecipientset import (
     NotificationRecipientSet)
+from lp.services.mail.sendmail import format_address, MailController
 
 
 class BaseMailer:
@@ -26,7 +29,8 @@
     """
 
     def __init__(self, subject, template_name, recipients, from_address,
-                 delta=None, message_id=None, notification_type=None):
+                 delta=None, message_id=None, notification_type=None,
+                 mail_controller_class=None):
         """Constructor.
 
         :param subject: A Python dict-replacement template for the subject
@@ -38,6 +42,8 @@
             and "new_values", such as BranchMergeProposalDelta.
         :param message_id: The Message-Id to use for generated emails.  If
             not supplied, random message-ids will be used.
+        :param mail_controller_class: The class of the mail controller to
+            use to send the mails.  Defaults to `MailController`.
         """
         self._subject_template = subject
         self._template_name = template_name
@@ -48,11 +54,15 @@
         self.delta = delta
         self.message_id = message_id
         self.notification_type = notification_type
+        self.logger = logging.getLogger('lp.services.mail.basemailer')
+        if mail_controller_class is None:
+            mail_controller_class = MailController
+        self._mail_controller_class = mail_controller_class
 
     def _getToAddresses(self, recipient, email):
         return [format_address(recipient.displayname, email)]
 
-    def generateEmail(self, email, recipient):
+    def generateEmail(self, email, recipient, force_no_attachments=False):
         """Generate the email for this recipient.
 
         :param email: Email address of the recipient to send to.
@@ -63,10 +73,15 @@
         headers = self._getHeaders(email)
         subject = self._getSubject(email)
         body = self._getBody(email)
-        ctrl = MailController(
+        ctrl = self._mail_controller_class(
             self.from_address, to_addresses, subject, body, headers,
             envelope_to=[email])
-        self._addAttachments(ctrl, email)
+        if force_no_attachments:
+            ctrl.addAttachment(
+                'Excessively large attachments removed.',
+                content_type='text/plain', inline=True)
+        else:
+            self._addAttachments(ctrl, email)
         return ctrl
 
     def _getSubject(self, email):
@@ -119,6 +134,19 @@
 
     def sendAll(self):
         """Send notifications to all recipients."""
+        # We never want SMTP errors to propagate from this function.
         for email, recipient in self._recipients.getRecipientPersons():
-            ctrl = self.generateEmail(email, recipient)
-            ctrl.send()
+            try:
+                ctrl = self.generateEmail(email, recipient)
+                ctrl.send()
+            except SMTPException, e:
+                # If the initial sending failed, try again without
+                # attachments.
+                try:
+                    ctrl = self.generateEmail(
+                        email, recipient, force_no_attachments=True)
+                    ctrl.send()
+                except SMTPException, e:
+                    # Don't want an entire stack trace, just some details.
+                    self.logger.warning(
+                        'send failed for %s, %s' % (email, e))

=== modified file 'lib/lp/services/mail/tests/test_basemailer.py'
--- lib/lp/services/mail/tests/test_basemailer.py	2009-07-17 02:25:09 +0000
+++ lib/lp/services/mail/tests/test_basemailer.py	2009-09-04 05:02:48 +0000
@@ -1,15 +1,19 @@
 # Copyright 2009 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+"""Tests for the BaseMailer class."""
+
 
 __metaclass__ = type
 
-
+from smtplib import SMTPException
 import unittest
 
 from canonical.testing import LaunchpadZopelessLayer
 
 from lp.testing import TestCaseWithFactory
+from lp.testing.mail_helpers import pop_notifications
 from lp.services.mail.basemailer import BaseMailer
+from lp.services.mail.sendmail import MailController
 
 
 class FakeSubscription:
@@ -35,6 +39,44 @@
         return email.upper()
 
 
+class AttachmentMailer(BaseMailerSubclass):
+    """Subclass the test mailer to add an attachment."""
+
+    def _addAttachments(self, ctrl, email):
+        ctrl.addAttachment('attachment1')
+        ctrl.addAttachment('attachment2')
+
+
+class RaisingMailController(MailController):
+    """A mail controller that can raise errors."""
+
+    def raiseOnSend(self):
+        """Make send fail for the specified email address."""
+        self.raise_on_send = True
+
+    def send(self, bulk=True):
+        if getattr(self, 'raise_on_send', False):
+            raise SMTPException('boom')
+        else:
+            super(RaisingMailController, self).send(bulk)
+
+
+class RaisingMailControllerFactory:
+    """Pretends to be a class to make raising mail controllers."""
+
+    def __init__(self, bad_email_addr, raise_count):
+        self.bad_email_addr = bad_email_addr
+        self.raise_count = raise_count
+
+    def __call__(self, *args, **kwargs):
+        ctrl = RaisingMailController(*args, **kwargs)
+        if ((self.bad_email_addr in kwargs['envelope_to'])
+            and self.raise_count):
+            self.raise_count -= 1
+            ctrl.raiseOnSend()
+        return ctrl
+
+
 class TestBaseMailer(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
@@ -47,8 +89,8 @@
         fake_to = self.factory.makePerson(email='[e-mail]',
             displayname='Example To')
         recipients = {fake_to: FakeSubscription()}
-        mailer = BaseMailerSubclass('subject', None, recipients,
-                                    '[e-mail]')
+        mailer = BaseMailerSubclass(
+            'subject', None, recipients, '[e-mail]')
         ctrl = mailer.generateEmail('[e-mail]', fake_to)
         self.assertEqual(['[e-mail]'], ctrl.envelope_to)
         self.assertEqual(['Example To <[e-mail]>'], ctrl.to_addrs)
@@ -61,11 +103,90 @@
         """
         fake_to = self.factory.makePerson(email='[e-mail]')
         recipients = {fake_to: FakeSubscription()}
-        mailer = ToAddressesUpper('subject', None, recipients,
-                                  '[e-mail]')
+        mailer = ToAddressesUpper(
+            'subject', None, recipients, '[e-mail]')
         ctrl = mailer.generateEmail('[e-mail]', fake_to)
         self.assertEqual(['[e-mail]'], ctrl.to_addrs)
 
+    def test_generateEmail_adds_attachments(self):
+        # BaseMailer.generateEmail calls _addAttachments.
+        fake_to = self.factory.makePerson(email='[e-mail]')
+        recipients = {fake_to: FakeSubscription()}
+        mailer = AttachmentMailer(
+            'subject', None, recipients, '[e-mail]')
+        ctrl = mailer.generateEmail('[e-mail]', fake_to)
+        self.assertEqual(2, len(ctrl.attachments))
+
+    def test_generateEmail_force_no_attachments(self):
+        # If BaseMailer.getnerateEmail is called with
+        # force_no_attachments=True then attachments are not added.
+        fake_to = self.factory.makePerson(email='[e-mail]')
+        recipients = {fake_to: FakeSubscription()}
+        mailer = AttachmentMailer(
+            'subject', None, recipients, '[e-mail]')
+        ctrl = mailer.generateEmail(
+            '[e-mail]', fake_to, force_no_attachments=True)
+        self.assertEqual(1, len(ctrl.attachments))
+        attachment = ctrl.attachments[0]
+        self.assertEqual(
+            'Excessively large attachments removed.',
+            attachment.get_payload())
+        self.assertEqual('text/plain', attachment['Content-Type'])
+        self.assertEqual('inline', attachment['Content-Disposition'])
+
+    def test_sendall_single_failure_doesnt_kill_all(self):
+        # A failure to send to a particular email address doesn't stop ending
+        # to others.
+        recipients = {
+            self.factory.makePerson(name='good', email='[e-mail]'):
+                FakeSubscription(),
+            self.factory.makePerson(name='bad', email='[e-mail]'):
+                FakeSubscription()}
+        controller_factory = RaisingMailControllerFactory(
+            '[e-mail]', 2)
+        mailer = BaseMailerSubclass(
+            'subject', None, recipients, '[e-mail]',
+            mail_controller_class=controller_factory)
+        mailer.sendAll()
+        # One email is still sent.
+        notifications = pop_notifications()
+        self.assertEqual(1, len(notifications))
+        self.assertEqual('Good <[e-mail]>', notifications[0]['To'])
+
+    def test_sendall_first_failure_strips_attachments(self):
+        # If sending an email fails, we try again without the (almost
+        # certainly) large attachment.
+        recipients = {
+            self.factory.makePerson(name='good', email='[e-mail]'):
+                FakeSubscription(),
+            self.factory.makePerson(name='bad', email='[e-mail]'):
+                FakeSubscription()}
+        # Only raise the first time for bob.
+        controller_factory = RaisingMailControllerFactory(
+            '[e-mail]', 1)
+        mailer = AttachmentMailer(
+            'subject', None, recipients, '[e-mail]',
+            mail_controller_class=controller_factory)
+        mailer.sendAll()
+        # Both emails are sent.
+        notifications = pop_notifications()
+        self.assertEqual(2, len(notifications))
+        bad, good = notifications
+        # The good email as the expected attachments.
+        good_parts = good.get_payload()
+        self.assertEqual(3, len(good_parts))
+        self.assertEqual(
+            'attachment1', good_parts[1].get_payload(decode=True))
+        self.assertEqual(
+            'attachment2', good_parts[2].get_payload(decode=True))
+        # The bad email has the normal attachments stripped off and replaced
+        # with the text.
+        bad_parts = bad.get_payload()
+        self.assertEqual(2, len(bad_parts))
+        self.assertEqual(
+            'Excessively large attachments removed.',
+            bad_parts[1].get_payload(decode=True))
+
 
 def test_suite():
     suite = unittest.TestSuite()