launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19310
[Merge] lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail into lp:launchpad with lp:~cjwatson/launchpad/more-pop-notifications as a prerequisite.
Commit message:
Disable Zopeless immediate mail delivery, except for BaseMailer, job OOPS/error notifications, and a few other places that still need it.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #29744 in Launchpad itself: "In Zopeless, mails should be sent only when the transaction is committed"
https://bugs.launchpad.net/launchpad/+bug/29744
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/opt-in-zopeless-immediate-mail/+merge/270383
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/opt-in-zopeless-immediate-mail into lp:launchpad.
=== 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: test@xxxxxxxxxxxxx
=== 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, 'to@xxxxxxxxxxx', 'subject', invalid_body,
bulk=False)
ctrl.send()
+ transaction.commit()
handleMail()
self.assertEqual([], self.oopses)
[notification] = pop_notifications()
@@ -101,6 +102,7 @@
email_address, 'to@xxxxxxxxxxx', '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')
Follow ups
