launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24009
[Merge] ~cjwatson/launchpad:opt-in-zopeless-immediate-mail into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:opt-in-zopeless-immediate-mail into launchpad:master.
Commit message:
Mostly disable Zopeless immediate mail delivery
BaseMailer, job OOPS/error notifications, and a few other places 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.
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/+git/launchpad/+merge/373727
This is essentially the same as https://code.launchpad.net/~cjwatson/launchpad/opt-in-zopeless-immediate-mail/+merge/270383, converted to git and rebased on master.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:opt-in-zopeless-immediate-mail into launchpad:master.
diff --git a/daemons/buildd-manager.tac b/daemons/buildd-manager.tac
index 06f99e5..c263ed4 100644
--- a/daemons/buildd-manager.tac
+++ b/daemons/buildd-manager.tac
@@ -9,22 +9,18 @@ import resource
from twisted.application import service
from twisted.scripts.twistd import ServerOptions
+from lp.buildmaster.manager import BuilddManager
from lp.services.config import (
config,
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.features import setup_feature_controller
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)
# Allow generous slack for database connections, idle download connections,
# etc.
diff --git a/lib/lp/bugs/scripts/bugnotification.py b/lib/lp/bugs/scripts/bugnotification.py
index fbcdbb6..c6ac115 100644
--- a/lib/lp/bugs/scripts/bugnotification.py
+++ b/lib/lp/bugs/scripts/bugnotification.py
@@ -35,7 +35,10 @@ from lp.registry.model.person import get_recipients
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
@@ -338,7 +341,7 @@ def process_deferred_notifications(bug_notifications):
class SendBugNotifications(LaunchpadCronScript):
- def main(self):
+ def send_notifications(self):
notifications_sent = False
bug_notification_set = getUtility(IBugNotificationSet)
deferred_notifications = \
@@ -391,3 +394,9 @@ class SendBugNotifications(LaunchpadCronScript):
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()
diff --git a/lib/lp/bugs/scripts/tests/test_bugnotification.py b/lib/lp/bugs/scripts/tests/test_bugnotification.py
index 41b7ba4..4200fc4 100644
--- a/lib/lp/bugs/scripts/tests/test_bugnotification.py
+++ b/lib/lp/bugs/scripts/tests/test_bugnotification.py
@@ -1368,7 +1368,7 @@ class TestSendBugNotifications(TestCaseWithFactory):
"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(
diff --git a/lib/lp/bugs/tests/bugs-emailinterface.txt b/lib/lp/bugs/tests/bugs-emailinterface.txt
index ab8fd0d..22d80d3 100644
--- a/lib/lp/bugs/tests/bugs-emailinterface.txt
+++ b/lib/lp/bugs/tests/bugs-emailinterface.txt
@@ -81,8 +81,8 @@ bug got submitted correctly:
>>> 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 @@ The same will happen if we send the same email without signing it:
... 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 @@ to edit bug 4:
... 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):
@@ -1813,10 +1809,9 @@ signing the mail:
>>> 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
diff --git a/lib/lp/code/mail/codehandler.py b/lib/lp/code/mail/codehandler.py
index 80eceff..bd589b4 100644
--- a/lib/lp/code/mail/codehandler.py
+++ b/lib/lp/code/mail/codehandler.py
@@ -314,11 +314,11 @@ class CodeHandler:
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
diff --git a/lib/lp/services/job/celeryjob.py b/lib/lp/services/job/celeryjob.py
index 95e5519..40dc0a8 100644
--- a/lib/lp/services/job/celeryjob.py
+++ b/lib/lp/services/job/celeryjob.py
@@ -40,7 +40,6 @@ from lp.services.features import (
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,
@@ -157,7 +156,6 @@ def ensure_zcml():
return
transaction.abort()
scripts.execute_zcml_for_scripts(use_web_security=False)
- set_immediate_mail_delivery(True)
needs_zcml = False
diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py
index 635e075..7c0fe96 100644
--- a/lib/lp/services/job/runner.py
+++ b/lib/lp/services/job/runner.py
@@ -72,8 +72,8 @@ from lp.services.job.interfaces.job import (
IRunnableJob,
)
from lp.services.mail.sendmail import (
+ immediate_mail_delivery,
MailController,
- set_immediate_mail_delivery,
)
from lp.services.timeout import (
get_default_timeout_function,
@@ -187,7 +187,9 @@ class BaseRunnableJob(BaseRunnableJobSource):
"""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`."""
@@ -197,7 +199,9 @@ class BaseRunnableJob(BaseRunnableJobSource):
"""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."""
@@ -434,9 +438,6 @@ class JobRunnerProcess(child.AMPChild):
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):
diff --git a/lib/lp/services/mail/basemailer.py b/lib/lp/services/mail/basemailer.py
index a2f8a40..36380f6 100644
--- a/lib/lp/services/mail/basemailer.py
+++ b/lib/lp/services/mail/basemailer.py
@@ -22,6 +22,7 @@ from lp.services.mail.notificationrecipientset import NotificationRecipientSet
from lp.services.mail.sendmail import (
append_footer,
format_address,
+ immediate_mail_delivery,
MailController,
)
from lp.services.utils import text_delta
@@ -231,8 +232,12 @@ class BaseMailer:
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:
diff --git a/lib/lp/services/mail/sendmail.py b/lib/lp/services/mail/sendmail.py
index b344b12..ec17b93 100644
--- a/lib/lp/services/mail/sendmail.py
+++ b/lib/lp/services/mail/sendmail.py
@@ -19,6 +19,7 @@ __all__ = [
'format_address',
'format_address_for_person',
'get_msgid',
+ 'immediate_mail_delivery',
'MailController',
'sendmail',
'set_immediate_mail_delivery',
@@ -29,6 +30,7 @@ __all__ = [
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 @@ def set_immediate_mail_delivery(enabled):
_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
diff --git a/lib/lp/services/mail/tests/incomingmail.txt b/lib/lp/services/mail/tests/incomingmail.txt
index 996e23d..dccaeb2 100644
--- a/lib/lp/services/mail/tests/incomingmail.txt
+++ b/lib/lp/services/mail/tests/incomingmail.txt
@@ -45,16 +45,20 @@ handle, and register them for some domains:
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': []}
diff --git a/lib/lp/services/mail/tests/test_incoming.py b/lib/lp/services/mail/tests/test_incoming.py
index 28bac00..32ce838 100644
--- a/lib/lp/services/mail/tests/test_incoming.py
+++ b/lib/lp/services/mail/tests/test_incoming.py
@@ -105,6 +105,7 @@ class IncomingTestCase(TestCaseWithFactory):
email_address, 'to@xxxxxxxxxxx', 'subject', invalid_body,
bulk=False)
ctrl.send()
+ transaction.commit()
handleMail()
self.assertEqual([], self.oopses)
[notification] = pop_notifications()
@@ -138,6 +139,7 @@ class IncomingTestCase(TestCaseWithFactory):
email_address, 'to@xxxxxxxxxxx', 'subject', invalid_body,
bulk=False)
ctrl.send()
+ transaction.commit()
handleMail()
self.assertEqual([], self.oopses)
[notification] = pop_notifications()
@@ -171,6 +173,7 @@ class IncomingTestCase(TestCaseWithFactory):
email_address, 'to@xxxxxxxxxxx', 'subject', invalid_body,
bulk=False)
ctrl.send()
+ transaction.commit()
handleMail()
self.assertEqual([], self.oopses)
[notification] = pop_notifications()
@@ -197,6 +200,7 @@ class IncomingTestCase(TestCaseWithFactory):
email_address, 'to@xxxxxxxxxxx', 'subject', fat_body,
bulk=False)
ctrl.send()
+ transaction.commit()
handleMail()
self.assertEqual([], self.oopses)
[notification] = pop_notifications()
diff --git a/lib/lp/services/scripts/base.py b/lib/lp/services/scripts/base.py
index 2a19eff..772d4de 100644
--- a/lib/lp/services/scripts/base.py
+++ b/lib/lp/services/scripts/base.py
@@ -44,7 +44,6 @@ from lp.services.features import (
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 @@ class LaunchpadScript:
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:
diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
index ffac753..bed3545 100644
--- a/lib/lp/testing/layers.py
+++ b/lib/lp/testing/layers.py
@@ -1486,10 +1486,6 @@ class LaunchpadZopelessLayer(LaunchpadScriptLayer):
@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()
diff --git a/scripts/mlist-import.py b/scripts/mlist-import.py
index 6aefe78..0d9b4f1 100755
--- a/scripts/mlist-import.py
+++ b/scripts/mlist-import.py
@@ -24,6 +24,7 @@ import textwrap
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 @@ class MailingListImport(LaunchpadScript):
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')
