Merge lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail into lp:launchpad

[Colin Watson]

Disable Zopeless immediate mail delivery, except for BaseMailer, job OOPS/error notifications, and a few other places that still need it.

Disable Zopeless immediate mail delivery, except for BaseMailer, job OOPS/error notifications, and a few other places that still need it.

This is a step towards being able to disable it across the board so that we can avoid ever having situations where operation/mail/operation/mail sequences send duplicate mail if a non-first operation fails and has to be retried. There are still several things that need to be fixed, notably archiveuploader; but this branch at least means that it's disabled for tests by default, which will simplify the process of fixing archiveuploader.

[Jes Slow (cluelesscoder)]

This seems pretty stale - is it even relevant at this point?

I accidentally clicked a red "-" button - not sure what it did, sorry if it screwed something up.

[Jes Slow (cluelesscoder)]

OK, looking around at the UI elsewhere I guess I must've removed a related bug - is there a log anywhere where I can find that bug and re-add it?

[Colin Watson (cjwatson)]

It's absolutely still relevant; it's just a fairly invasive change so it's probably scared off reviewers. :-) I've restored the bug link.

[Jes Slow (cluelesscoder)]

Bit off-topic to ask this here, but I suppose there's no log where I could look up my change where I removed the bug report? Also wondering if I could pull up a list of my past comments...? Hunted around for a bug report (33 at https://bugs.launchpad.net/launchpad?field.searchtext=activity+log) and noticed that I can pull up an "Full Activity Log" at a bug report like https://bugs.launchpad.net/launchpad/+bug/294644

Basically would like a full activity log for myself.

Also found https://bugs.launchpad.net/launchpad/+bug/787668 which I suppose could cover this? I can leave a comment there.

[Colin Watson (cjwatson)]

I got it in email about this merge proposal, and it's in the activity log for the bug, but but merge proposals don't themselves have an activity log. You can search for bugs on which you've commented, but there likewise isn't a full activity log for everything a person has done (and that part is pretty much exactly bug 787668).

This is indeed off-topic here though, as you say, and it'd be best not to continue this conversation in this merge proposal because it's likely to get in the way of people reviewing it.

[Colin Watson (cjwatson)]

Superseded by https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373727.

Unmerged revisions

17726. By Colin Watson on 2015-09-08

Drop immediate mail delivery from LaunchpadScript and LaunchpadZopelessLayer.

17725. By Colin Watson on 2015-09-08

Drop explicit immediate mail delivery from buildd-manager; build mail all goes via BaseMailer now, which handles that.

17724. By Colin Watson on 2015-09-08

Drop immediate mail delivery from jobs, except for OOPS and user error notifications.

17723. By Colin Watson on 2015-09-08

Fix CodeHandler to abort before sending error notifications.

17722. By Colin Watson on 2015-09-08

Force immediate mail delivery for MailingListImport.

17721. By Colin Watson on 2015-09-08

Force immediate mail delivery for bug notifications and BaseMailer, which rely on this for SMTP exception handling.

17720. By Colin Watson on 2015-09-08

Adjust some tests to stop assuming immediate mail delivery.

17719. By Colin Watson on 2015-09-08

Introduce an immediate_mail_delivery context manager, allowing more fine-grained control.

=== modified file 'daemons/buildd-manager.tac'
--- daemons/buildd-manager.tac	2011-12-29 05:29:36 +0000
+++ daemons/buildd-manager.tac	2015-09-08 11:52:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # Twisted Application Configuration file.
@@ -6,20 +6,15 @@
 
 from twisted.application import service
 from twisted.scripts.twistd import ServerOptions
-from twisted.web import server
 
+from lp.buildmaster.manager import BuilddManager
 from lp.services.config import dbconfig
 from lp.services.daemons import readyservice
 from lp.services.scripts import execute_zcml_for_scripts
-from lp.buildmaster.manager import BuilddManager
-from lp.services.mail.sendmail import set_immediate_mail_delivery
 from lp.services.twistedsupport.loggingsupport import RotatableFileLogObserver
 
 execute_zcml_for_scripts()
 dbconfig.override(dbuser='buildd_manager', isolation_level='read_committed')
-# XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
-# Should be removed from callsites verified to not need it.
-set_immediate_mail_delivery(True)
 
 options = ServerOptions()
 options.parseOptions()
@@ -34,4 +29,3 @@
 # Service for scanning buildd slaves.
 service = BuilddManager()
 service.setServiceParent(application)
-
 
=== modified file 'lib/lp/bugs/scripts/bugnotification.py'
--- lib/lp/bugs/scripts/bugnotification.py	2015-08-06 12:09:28 +0000
+++ lib/lp/bugs/scripts/bugnotification.py	2015-09-08 11:52:24 +0000
@@ -35,7 +35,10 @@
 from lp.services.database.constants import UTC_NOW
 from lp.services.mail.helpers import get_email_template
 from lp.services.mail.mailwrapper import MailWrapper
-from lp.services.mail.sendmail import sendmail
+from lp.services.mail.sendmail import (
+    immediate_mail_delivery,
+    sendmail,
+    )
 from lp.services.scripts.base import LaunchpadCronScript
 from lp.services.scripts.logger import log
 from lp.services.webapp import canonical_url
@@ -339,7 +342,7 @@
 
 
 class SendBugNotifications(LaunchpadCronScript):
-    def main(self):
+    def send_notifications(self):
         notifications_sent = False
         bug_notification_set = getUtility(IBugNotificationSet)
         deferred_notifications = \
@@ -392,3 +395,9 @@
 
         if not notifications_sent:
             self.logger.debug("No notifications are pending to be sent.")
+
+    def main(self):
+        # XXX cjwatson 2015-09-05 bug=29744: SMTP exception handling here
+        # currently relies on this.
+        with immediate_mail_delivery(True):
+            self.send_notifications()
 
=== modified file 'lib/lp/bugs/scripts/tests/test_bugnotification.py'
--- lib/lp/bugs/scripts/tests/test_bugnotification.py	2015-08-06 12:09:28 +0000
+++ lib/lp/bugs/scripts/tests/test_bugnotification.py	2015-09-08 11:52:24 +0000
@@ -1368,7 +1368,7 @@
                 "send-bug-notifications", config.malone.bugnotification_dbuser,
                 ["-q"])
             script.txn = self.layer.txn
-            script.main()
+            script.send_notifications()
 
         self.assertEqual(1, len(self.oopses))
         self.assertIn(
 
=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt	2015-07-21 09:04:01 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt	2015-09-08 11:52:24 +0000
@@ -81,8 +81,8 @@
 
     >>> def process_email(raw_mail):
     ...     msg = construct_email(raw_mail)
-    ...     handler.process(msg, msg['To'],
-    ...         )
+    ...     handler.process(msg, msg['To'])
+    ...     transaction.commit()
 
     >>> process_email(submit_mail)
 
@@ -454,9 +454,7 @@
     ...     signature = None
     >>> msg = email.message_from_string(
     ...     comment_mail, _class=MockUnsignedMessage)
-    >>> handler.process(
-    ...     msg, msg['To'],
-    ...     )
+    >>> handler.process(msg, msg['To'])
     True
     >>> transaction.commit()
 
@@ -514,9 +512,7 @@
     ...     return construct_email(edit_mail)
 
     >>> def submit_command_email(msg):
-    ...     handler.process(
-    ...         msg, msg['To'],
-    ...         )
+    ...     handler.process(msg, msg['To'])
     ...     transaction.commit()
 
     >>> def submit_commands(bug, *commands):
@@ -1805,10 +1801,9 @@
     >>> msg = signed_message_from_string(submit_mail)
     >>> import email.utils
     >>> msg['Message-Id'] = email.utils.make_msgid()
-    >>> handler.process(
-    ...     msg, msg['To'],
-    ...     )
+    >>> handler.process(msg, msg['To'])
     True
+    >>> transaction.commit()
     >>> print_latest_email()
     Subject: Submit Request Failure
     To: [e-mail]
 
=== modified file 'lib/lp/code/mail/codehandler.py'
--- lib/lp/code/mail/codehandler.py	2015-07-08 16:05:11 +0000
+++ lib/lp/code/mail/codehandler.py	2015-09-08 11:52:24 +0000
@@ -316,11 +316,11 @@
                 message, context.vote, context.vote_tags, mail)
 
         except IncomingEmailError as error:
+            transaction.abort()
             send_process_error_notification(
                 str(user.preferredemail.email),
                 'Submit Request Failure',
                 error.message, mail, error.failing_command)
-            transaction.abort()
         return True
 
     @staticmethod
 
=== modified file 'lib/lp/services/job/celeryjob.py'
--- lib/lp/services/job/celeryjob.py	2015-08-03 07:06:45 +0000
+++ lib/lp/services/job/celeryjob.py	2015-09-08 11:52:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Celery-specific Job code.
@@ -34,7 +34,6 @@
     install_feature_controller,
     make_script_feature_controller,
     )
-from lp.services.mail.sendmail import set_immediate_mail_delivery
 from lp.services.job.model.job import (
     Job,
     UniversalJobSource,
@@ -139,7 +138,6 @@
         return
     transaction.abort()
     scripts.execute_zcml_for_scripts(use_web_security=False)
-    set_immediate_mail_delivery(True)
     needs_zcml = False
 
 
 
=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2015-08-04 00:19:36 +0000
+++ lib/lp/services/job/runner.py	2015-09-08 11:52:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Facilities for running Jobs."""
@@ -72,8 +72,8 @@
     IRunnableJob,
     )
 from lp.services.mail.sendmail import (
+    immediate_mail_delivery,
     MailController,
-    set_immediate_mail_delivery,
     )
 from lp.services.twistedsupport import run_reactor
 from lp.services.webapp import errorlog
@@ -177,7 +177,9 @@
         """Report this oops."""
         ctrl = self.getOopsMailController(oops['id'])
         if ctrl is not None:
-            ctrl.send()
+            # XXX cjwatson 2015-09-06 bug=29744: Can this be removed?
+            with immediate_mail_delivery(True):
+                ctrl.send()
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
@@ -187,7 +189,9 @@
         """See `IRunnableJob`."""
         ctrl = self.getUserErrorMailController(e)
         if ctrl is not None:
-            ctrl.send()
+            # XXX cjwatson 2015-09-06 bug=29744: Can this be removed?
+            with immediate_mail_delivery(True):
+                ctrl.send()
 
     def makeOopsReport(self, oops_config, info):
         """Generate an OOPS report using the given OOPS configuration."""
@@ -406,9 +410,6 @@
         scripts.execute_zcml_for_scripts(use_web_security=False)
         signal(SIGHUP, handler)
         dbconfig.override(dbuser=cls.dbuser, isolation_level='read_committed')
-        # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
-        # Should be removed from callsites verified to not need it.
-        set_immediate_mail_delivery(True)
 
     @staticmethod
     def __exit__(exc_type, exc_val, exc_tb):
 
=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py	2015-08-25 14:05:24 +0000
+++ lib/lp/services/mail/basemailer.py	2015-09-08 11:52:24 +0000
@@ -20,6 +20,7 @@
 from lp.services.mail.sendmail import (
     append_footer,
     format_address,
+    immediate_mail_delivery,
     MailController,
     )
 from lp.services.utils import text_delta
@@ -204,8 +205,12 @@
 
     def sendAll(self):
         """Send notifications to all recipients."""
-        for email, recipient in sorted(self._recipients.getRecipientPersons()):
-            self.sendOne(email, recipient)
+        # XXX cjwatson 2015-09-05 bug=29744: We currently depend on this to
+        # handle SMTPExceptions in the correct place.
+        with immediate_mail_delivery(True):
+            for email, recipient in sorted(
+                    self._recipients.getRecipientPersons()):
+                self.sendOne(email, recipient)
 
 
 class RecipientReason:
 
=== modified file 'lib/lp/services/mail/sendmail.py'
--- lib/lp/services/mail/sendmail.py	2015-07-21 09:04:01 +0000
+++ lib/lp/services/mail/sendmail.py	2015-09-08 11:52:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The One True Way to send mail from the Launchpad application.
@@ -19,6 +19,7 @@
     'format_address',
     'format_address_for_person',
     'get_msgid',
+    'immediate_mail_delivery',
     'MailController',
     'sendmail',
     'set_immediate_mail_delivery',
@@ -29,6 +30,7 @@
 
 
 from binascii import b2a_qp
+from contextlib import contextmanager
 from email import charset
 from email.encoders import encode_base64
 from email.header import Header
@@ -178,6 +180,16 @@
     _immediate_mail_delivery = enabled
 
 
+@contextmanager
+def immediate_mail_delivery(enabled):
+    """Context manager to temporarily change immediate mail delivery."""
+    global _immediate_mail_delivery
+    previous = _immediate_mail_delivery
+    set_immediate_mail_delivery(enabled)
+    yield
+    set_immediate_mail_delivery(previous)
+
+
 def simple_sendmail(from_addr, to_addrs, subject, body, headers=None,
                     bulk=True):
     """Send an email from from_addr to to_addrs with the subject and body
 
=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
--- lib/lp/services/mail/tests/incomingmail.txt	2015-09-08 11:52:24 +0000
+++ lib/lp/services/mail/tests/incomingmail.txt	2015-09-08 11:52:24 +0000
@@ -45,16 +45,20 @@
 
 Now we send a few test mails to foo.com, bar.com, and baz.com:
 
+    >>> import transaction
     >>> from lp.services.mail.tests.helpers import read_test_message
     >>> from lp.services.mail.sendmail import sendmail as original_sendmail
     >>> from lp.testing.dbuser import switch_dbuser
 
 For these examples, we don't want the Precedence header added. Domains
 are treated without regard to case: for incoming mail, foo.com and
-FOO.COM are treated equivalently.
+FOO.COM are treated equivalently. We also commit to ensure that mail is
+sent immediately.
 
     >>> def sendmail(msg, to_addrs=None):
-    ...     return original_sendmail(msg, to_addrs=to_addrs, bulk=False)
+    ...     msgid = original_sendmail(msg, to_addrs=to_addrs, bulk=False)
+    ...     transaction.commit()
+    ...     return msgid
 
     >>> switch_dbuser('launchpad')
     >>> msgids = {'foo.com': [], 'bar.com': [], 'baz.com': []}
 
=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2015-07-08 16:05:11 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2015-09-08 11:52:24 +0000
@@ -75,6 +75,7 @@
             email_address, '[e-mail]', 'subject', invalid_body,
             bulk=False)
         ctrl.send()
+        transaction.commit()
         handleMail()
         self.assertEqual([], self.oopses)
         [notification] = pop_notifications()
@@ -101,6 +102,7 @@
             email_address, '[e-mail]', 'subject', fat_body,
             bulk=False)
         ctrl.send()
+        transaction.commit()
         handleMail()
         self.assertEqual([], self.oopses)
         [notification] = pop_notifications()
 
=== modified file 'lib/lp/services/scripts/base.py'
--- lib/lp/services/scripts/base.py	2014-08-29 01:34:04 +0000
+++ lib/lp/services/scripts/base.py	2015-09-08 11:52:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -44,7 +44,6 @@
     install_feature_controller,
     make_script_feature_controller,
     )
-from lp.services.mail.sendmail import set_immediate_mail_delivery
 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
 from lp.services.scripts.logger import OopsHandler
 from lp.services.webapp.errorlog import globalErrorUtility
@@ -307,10 +306,6 @@
         self._init_zca(use_web_security=use_web_security)
         self._init_db(isolation=isolation)
 
-        # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
-        # Should be called directly by scripts that actually need it.
-        set_immediate_mail_delivery(True)
-
         date_started = datetime.datetime.now(UTC)
         profiler = None
         if self.options.profile:
 
=== modified file 'lib/lp/testing/layers.py'
--- lib/lp/testing/layers.py	2013-08-14 11:15:51 +0000
+++ lib/lp/testing/layers.py	2015-09-08 11:52:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Layers used by Launchpad tests.
@@ -1498,10 +1498,6 @@
     @profiled
     def testSetUp(cls):
         dbconfig.override(isolation_level='read_committed')
-        # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
-        # Tests that still need it should eventually set this directly,
-        # so the whole layer is not polluted.
-        set_immediate_mail_delivery(True)
 
         # Connect Storm
         reconnect_stores()
 
=== modified file 'scripts/mlist-import.py'
--- scripts/mlist-import.py	2013-01-07 02:40:55 +0000
+++ scripts/mlist-import.py	2015-09-08 11:52:24 +0000
@@ -24,6 +24,7 @@
 
 from lp.registry.scripts.mlistimport import Importer
 from lp.services.config import config
+from lp.services.mail.sendmail import set_immediate_mail_delivery
 from lp.services.scripts.base import LaunchpadScript
 
 
@@ -56,6 +57,12 @@
 
     def main(self):
         """See `LaunchpadScript`."""
+        # XXX cjwatson 2015-09-06 bug=29744: We need immediate mail delivery
+        # for the current implementation of the --notifications switch, and
+        # because the tests expect notifications to end up in
+        # LayerProcessController.smtp_controller.
+        set_immediate_mail_delivery(True)
+
         team_name = None
         if len(self.args) == 0:
             self.parser.error('Missing team name')