Launchpad itself

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

  1. Git
  2. lp:~cjwatson/launchpad
  3. opt-in-zopeless-immediate-mail
  4. Merge into master
Proposed by Colin Watson
Status: Needs review
Proposed branch: ~cjwatson/launchpad:opt-in-zopeless-immediate-mail
Merge into: launchpad:master
Diff against target: 399 lines (+63/-41)
14 files modified
daemons/buildd-manager.tac (+1/-5)
lib/lp/bugs/scripts/bugnotification.py (+11/-2)
lib/lp/bugs/scripts/tests/test_bugnotification.py (+1/-1)
lib/lp/bugs/tests/bugs-emailinterface.txt (+6/-11)
lib/lp/code/mail/codehandler.py (+1/-1)
lib/lp/services/job/celeryjob.py (+0/-2)
lib/lp/services/job/runner.py (+7/-6)
lib/lp/services/mail/basemailer.py (+7/-2)
lib/lp/services/mail/sendmail.py (+12/-0)
lib/lp/services/mail/tests/incomingmail.txt (+6/-2)
lib/lp/services/mail/tests/test_incoming.py (+4/-0)
lib/lp/services/scripts/base.py (+0/-5)
lib/lp/testing/layers.py (+0/-4)
scripts/mlist-import.py (+7/-0)
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+373727@code.launchpad.net

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.

Description of the change

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.

To post a comment you must log in.

Unmerged commits

6be7bfc... by Colin Watson

Drop immediate mail delivery from LaunchpadScript and LaunchpadZopelessLayer.

6289675... by Colin Watson

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

8a36053... by Colin Watson

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

eab63ee... by Colin Watson

Fix CodeHandler to abort before sending error notifications.

1339654... by Colin Watson

Force immediate mail delivery for MailingListImport.

71cba20... by Colin Watson

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

64798df... by Colin Watson

Adjust some tests to stop assuming immediate mail delivery.

8412138... by Colin Watson

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

LP: #29744

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/daemons/buildd-manager.tac b/daemons/buildd-manager.tac
2index 06f99e5..c263ed4 100644
3--- a/daemons/buildd-manager.tac
4+++ b/daemons/buildd-manager.tac
5@@ -9,22 +9,18 @@ import resource
6 from twisted.application import service
7 from twisted.scripts.twistd import ServerOptions
8
9+from lp.buildmaster.manager import BuilddManager
10 from lp.services.config import (
11 config,
12 dbconfig,
13 )
14 from lp.services.daemons import readyservice
15 from lp.services.scripts import execute_zcml_for_scripts
16-from lp.buildmaster.manager import BuilddManager
17-from lp.services.mail.sendmail import set_immediate_mail_delivery
18 from lp.services.twistedsupport.features import setup_feature_controller
19 from lp.services.twistedsupport.loggingsupport import RotatableFileLogObserver
20
21 execute_zcml_for_scripts()
22 dbconfig.override(dbuser='buildd_manager', isolation_level='read_committed')
23-# XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
24-# Should be removed from callsites verified to not need it.
25-set_immediate_mail_delivery(True)
26
27 # Allow generous slack for database connections, idle download connections,
28 # etc.
29diff --git a/lib/lp/bugs/scripts/bugnotification.py b/lib/lp/bugs/scripts/bugnotification.py
30index fbcdbb6..c6ac115 100644
31--- a/lib/lp/bugs/scripts/bugnotification.py
32+++ b/lib/lp/bugs/scripts/bugnotification.py
33@@ -35,7 +35,10 @@ from lp.registry.model.person import get_recipients
34 from lp.services.database.constants import UTC_NOW
35 from lp.services.mail.helpers import get_email_template
36 from lp.services.mail.mailwrapper import MailWrapper
37-from lp.services.mail.sendmail import sendmail
38+from lp.services.mail.sendmail import (
39+ immediate_mail_delivery,
40+ sendmail,
41+ )
42 from lp.services.scripts.base import LaunchpadCronScript
43 from lp.services.scripts.logger import log
44 from lp.services.webapp import canonical_url
45@@ -338,7 +341,7 @@ def process_deferred_notifications(bug_notifications):
46
47
48 class SendBugNotifications(LaunchpadCronScript):
49- def main(self):
50+ def send_notifications(self):
51 notifications_sent = False
52 bug_notification_set = getUtility(IBugNotificationSet)
53 deferred_notifications = \
54@@ -391,3 +394,9 @@ class SendBugNotifications(LaunchpadCronScript):
55
56 if not notifications_sent:
57 self.logger.debug("No notifications are pending to be sent.")
58+
59+ def main(self):
60+ # XXX cjwatson 2015-09-05 bug=29744: SMTP exception handling here
61+ # currently relies on this.
62+ with immediate_mail_delivery(True):
63+ self.send_notifications()
64diff --git a/lib/lp/bugs/scripts/tests/test_bugnotification.py b/lib/lp/bugs/scripts/tests/test_bugnotification.py
65index 41b7ba4..4200fc4 100644
66--- a/lib/lp/bugs/scripts/tests/test_bugnotification.py
67+++ b/lib/lp/bugs/scripts/tests/test_bugnotification.py
68@@ -1368,7 +1368,7 @@ class TestSendBugNotifications(TestCaseWithFactory):
69 "send-bug-notifications", config.malone.bugnotification_dbuser,
70 ["-q"])
71 script.txn = self.layer.txn
72- script.main()
73+ script.send_notifications()
74
75 self.assertEqual(1, len(self.oopses))
76 self.assertIn(
77diff --git a/lib/lp/bugs/tests/bugs-emailinterface.txt b/lib/lp/bugs/tests/bugs-emailinterface.txt
78index ab8fd0d..22d80d3 100644
79--- a/lib/lp/bugs/tests/bugs-emailinterface.txt
80+++ b/lib/lp/bugs/tests/bugs-emailinterface.txt
81@@ -81,8 +81,8 @@ bug got submitted correctly:
82
83 >>> def process_email(raw_mail):
84 ... msg = construct_email(raw_mail)
85- ... handler.process(msg, msg['To'],
86- ... )
87+ ... handler.process(msg, msg['To'])
88+ ... transaction.commit()
89
90 >>> process_email(submit_mail)
91
92@@ -454,9 +454,7 @@ The same will happen if we send the same email without signing it:
93 ... signature = None
94 >>> msg = email.message_from_string(
95 ... comment_mail, _class=MockUnsignedMessage)
96- >>> handler.process(
97- ... msg, msg['To'],
98- ... )
99+ >>> handler.process(msg, msg['To'])
100 True
101 >>> transaction.commit()
102
103@@ -514,9 +512,7 @@ to edit bug 4:
104 ... return construct_email(edit_mail)
105
106 >>> def submit_command_email(msg):
107- ... handler.process(
108- ... msg, msg['To'],
109- ... )
110+ ... handler.process(msg, msg['To'])
111 ... transaction.commit()
112
113 >>> def submit_commands(bug, *commands):
114@@ -1813,10 +1809,9 @@ signing the mail:
115 >>> msg = signed_message_from_string(submit_mail)
116 >>> import email.utils
117 >>> msg['Message-Id'] = email.utils.make_msgid()
118- >>> handler.process(
119- ... msg, msg['To'],
120- ... )
121+ >>> handler.process(msg, msg['To'])
122 True
123+ >>> transaction.commit()
124 >>> print_latest_email()
125 Subject: Submit Request Failure
126 To: test@canonical.com
127diff --git a/lib/lp/code/mail/codehandler.py b/lib/lp/code/mail/codehandler.py
128index 80eceff..bd589b4 100644
129--- a/lib/lp/code/mail/codehandler.py
130+++ b/lib/lp/code/mail/codehandler.py
131@@ -314,11 +314,11 @@ class CodeHandler:
132 message, context.vote, context.vote_tags, mail)
133
134 except IncomingEmailError as error:
135+ transaction.abort()
136 send_process_error_notification(
137 str(user.preferredemail.email),
138 'Submit Request Failure',
139 error.message, mail, error.failing_command)
140- transaction.abort()
141 return True
142
143 @staticmethod
144diff --git a/lib/lp/services/job/celeryjob.py b/lib/lp/services/job/celeryjob.py
145index 95e5519..40dc0a8 100644
146--- a/lib/lp/services/job/celeryjob.py
147+++ b/lib/lp/services/job/celeryjob.py
148@@ -40,7 +40,6 @@ from lp.services.features import (
149 install_feature_controller,
150 make_script_feature_controller,
151 )
152-from lp.services.mail.sendmail import set_immediate_mail_delivery
153 from lp.services.job.model.job import (
154 Job,
155 UniversalJobSource,
156@@ -157,7 +156,6 @@ def ensure_zcml():
157 return
158 transaction.abort()
159 scripts.execute_zcml_for_scripts(use_web_security=False)
160- set_immediate_mail_delivery(True)
161 needs_zcml = False
162
163
164diff --git a/lib/lp/services/job/runner.py b/lib/lp/services/job/runner.py
165index 635e075..7c0fe96 100644
166--- a/lib/lp/services/job/runner.py
167+++ b/lib/lp/services/job/runner.py
168@@ -72,8 +72,8 @@ from lp.services.job.interfaces.job import (
169 IRunnableJob,
170 )
171 from lp.services.mail.sendmail import (
172+ immediate_mail_delivery,
173 MailController,
174- set_immediate_mail_delivery,
175 )
176 from lp.services.timeout import (
177 get_default_timeout_function,
178@@ -187,7 +187,9 @@ class BaseRunnableJob(BaseRunnableJobSource):
179 """Report this oops."""
180 ctrl = self.getOopsMailController(oops['id'])
181 if ctrl is not None:
182- ctrl.send()
183+ # XXX cjwatson 2015-09-06 bug=29744: Can this be removed?
184+ with immediate_mail_delivery(True):
185+ ctrl.send()
186
187 def getOopsVars(self):
188 """See `IRunnableJob`."""
189@@ -197,7 +199,9 @@ class BaseRunnableJob(BaseRunnableJobSource):
190 """See `IRunnableJob`."""
191 ctrl = self.getUserErrorMailController(e)
192 if ctrl is not None:
193- ctrl.send()
194+ # XXX cjwatson 2015-09-06 bug=29744: Can this be removed?
195+ with immediate_mail_delivery(True):
196+ ctrl.send()
197
198 def makeOopsReport(self, oops_config, info):
199 """Generate an OOPS report using the given OOPS configuration."""
200@@ -434,9 +438,6 @@ class JobRunnerProcess(child.AMPChild):
201 scripts.execute_zcml_for_scripts(use_web_security=False)
202 signal(SIGHUP, handler)
203 dbconfig.override(dbuser=cls.dbuser, isolation_level='read_committed')
204- # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
205- # Should be removed from callsites verified to not need it.
206- set_immediate_mail_delivery(True)
207
208 @staticmethod
209 def __exit__(exc_type, exc_val, exc_tb):
210diff --git a/lib/lp/services/mail/basemailer.py b/lib/lp/services/mail/basemailer.py
211index a2f8a40..36380f6 100644
212--- a/lib/lp/services/mail/basemailer.py
213+++ b/lib/lp/services/mail/basemailer.py
214@@ -22,6 +22,7 @@ from lp.services.mail.notificationrecipientset import NotificationRecipientSet
215 from lp.services.mail.sendmail import (
216 append_footer,
217 format_address,
218+ immediate_mail_delivery,
219 MailController,
220 )
221 from lp.services.utils import text_delta
222@@ -231,8 +232,12 @@ class BaseMailer:
223
224 def sendAll(self):
225 """Send notifications to all recipients."""
226- for email, recipient in sorted(self._recipients.getRecipientPersons()):
227- self.sendOne(email, recipient)
228+ # XXX cjwatson 2015-09-05 bug=29744: We currently depend on this to
229+ # handle SMTPExceptions in the correct place.
230+ with immediate_mail_delivery(True):
231+ for email, recipient in sorted(
232+ self._recipients.getRecipientPersons()):
233+ self.sendOne(email, recipient)
234
235
236 class RecipientReason:
237diff --git a/lib/lp/services/mail/sendmail.py b/lib/lp/services/mail/sendmail.py
238index b344b12..ec17b93 100644
239--- a/lib/lp/services/mail/sendmail.py
240+++ b/lib/lp/services/mail/sendmail.py
241@@ -19,6 +19,7 @@ __all__ = [
242 'format_address',
243 'format_address_for_person',
244 'get_msgid',
245+ 'immediate_mail_delivery',
246 'MailController',
247 'sendmail',
248 'set_immediate_mail_delivery',
249@@ -29,6 +30,7 @@ __all__ = [
250
251
252 from binascii import b2a_qp
253+from contextlib import contextmanager
254 from email import charset
255 from email.encoders import encode_base64
256 from email.header import Header
257@@ -178,6 +180,16 @@ def set_immediate_mail_delivery(enabled):
258 _immediate_mail_delivery = enabled
259
260
261+@contextmanager
262+def immediate_mail_delivery(enabled):
263+ """Context manager to temporarily change immediate mail delivery."""
264+ global _immediate_mail_delivery
265+ previous = _immediate_mail_delivery
266+ set_immediate_mail_delivery(enabled)
267+ yield
268+ set_immediate_mail_delivery(previous)
269+
270+
271 def simple_sendmail(from_addr, to_addrs, subject, body, headers=None,
272 bulk=True):
273 """Send an email from from_addr to to_addrs with the subject and body
274diff --git a/lib/lp/services/mail/tests/incomingmail.txt b/lib/lp/services/mail/tests/incomingmail.txt
275index 996e23d..dccaeb2 100644
276--- a/lib/lp/services/mail/tests/incomingmail.txt
277+++ b/lib/lp/services/mail/tests/incomingmail.txt
278@@ -45,16 +45,20 @@ handle, and register them for some domains:
279
280 Now we send a few test mails to foo.com, bar.com, and baz.com:
281
282+ >>> import transaction
283 >>> from lp.services.mail.tests.helpers import read_test_message
284 >>> from lp.services.mail.sendmail import sendmail as original_sendmail
285 >>> from lp.testing.dbuser import switch_dbuser
286
287 For these examples, we don't want the Precedence header added. Domains
288 are treated without regard to case: for incoming mail, foo.com and
289-FOO.COM are treated equivalently.
290+FOO.COM are treated equivalently. We also commit to ensure that mail is
291+sent immediately.
292
293 >>> def sendmail(msg, to_addrs=None):
294- ... return original_sendmail(msg, to_addrs=to_addrs, bulk=False)
295+ ... msgid = original_sendmail(msg, to_addrs=to_addrs, bulk=False)
296+ ... transaction.commit()
297+ ... return msgid
298
299 >>> switch_dbuser('launchpad')
300 >>> msgids = {'foo.com': [], 'bar.com': [], 'baz.com': []}
301diff --git a/lib/lp/services/mail/tests/test_incoming.py b/lib/lp/services/mail/tests/test_incoming.py
302index 28bac00..32ce838 100644
303--- a/lib/lp/services/mail/tests/test_incoming.py
304+++ b/lib/lp/services/mail/tests/test_incoming.py
305@@ -105,6 +105,7 @@ class IncomingTestCase(TestCaseWithFactory):
306 email_address, 'to@example.com', 'subject', invalid_body,
307 bulk=False)
308 ctrl.send()
309+ transaction.commit()
310 handleMail()
311 self.assertEqual([], self.oopses)
312 [notification] = pop_notifications()
313@@ -138,6 +139,7 @@ class IncomingTestCase(TestCaseWithFactory):
314 email_address, 'to@example.com', 'subject', invalid_body,
315 bulk=False)
316 ctrl.send()
317+ transaction.commit()
318 handleMail()
319 self.assertEqual([], self.oopses)
320 [notification] = pop_notifications()
321@@ -171,6 +173,7 @@ class IncomingTestCase(TestCaseWithFactory):
322 email_address, 'to@example.com', 'subject', invalid_body,
323 bulk=False)
324 ctrl.send()
325+ transaction.commit()
326 handleMail()
327 self.assertEqual([], self.oopses)
328 [notification] = pop_notifications()
329@@ -197,6 +200,7 @@ class IncomingTestCase(TestCaseWithFactory):
330 email_address, 'to@example.com', 'subject', fat_body,
331 bulk=False)
332 ctrl.send()
333+ transaction.commit()
334 handleMail()
335 self.assertEqual([], self.oopses)
336 [notification] = pop_notifications()
337diff --git a/lib/lp/services/scripts/base.py b/lib/lp/services/scripts/base.py
338index 2a19eff..772d4de 100644
339--- a/lib/lp/services/scripts/base.py
340+++ b/lib/lp/services/scripts/base.py
341@@ -44,7 +44,6 @@ from lp.services.features import (
342 install_feature_controller,
343 make_script_feature_controller,
344 )
345-from lp.services.mail.sendmail import set_immediate_mail_delivery
346 from lp.services.scripts.interfaces.scriptactivity import IScriptActivitySet
347 from lp.services.scripts.logger import OopsHandler
348 from lp.services.webapp.errorlog import globalErrorUtility
349@@ -307,10 +306,6 @@ class LaunchpadScript:
350 self._init_zca(use_web_security=use_web_security)
351 self._init_db(isolation=isolation)
352
353- # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
354- # Should be called directly by scripts that actually need it.
355- set_immediate_mail_delivery(True)
356-
357 date_started = datetime.datetime.now(UTC)
358 profiler = None
359 if self.options.profile:
360diff --git a/lib/lp/testing/layers.py b/lib/lp/testing/layers.py
361index ffac753..bed3545 100644
362--- a/lib/lp/testing/layers.py
363+++ b/lib/lp/testing/layers.py
364@@ -1486,10 +1486,6 @@ class LaunchpadZopelessLayer(LaunchpadScriptLayer):
365 @profiled
366 def testSetUp(cls):
367 dbconfig.override(isolation_level='read_committed')
368- # XXX wgrant 2011-09-24 bug=29744: initZopeless used to do this.
369- # Tests that still need it should eventually set this directly,
370- # so the whole layer is not polluted.
371- set_immediate_mail_delivery(True)
372
373 # Connect Storm
374 reconnect_stores()
375diff --git a/scripts/mlist-import.py b/scripts/mlist-import.py
376index 6aefe78..0d9b4f1 100755
377--- a/scripts/mlist-import.py
378+++ b/scripts/mlist-import.py
379@@ -24,6 +24,7 @@ import textwrap
380
381 from lp.registry.scripts.mlistimport import Importer
382 from lp.services.config import config
383+from lp.services.mail.sendmail import set_immediate_mail_delivery
384 from lp.services.scripts.base import LaunchpadScript
385
386
387@@ -56,6 +57,12 @@ class MailingListImport(LaunchpadScript):
388
389 def main(self):
390 """See `LaunchpadScript`."""
391+ # XXX cjwatson 2015-09-06 bug=29744: We need immediate mail delivery
392+ # for the current implementation of the --notifications switch, and
393+ # because the tests expect notifications to end up in
394+ # LayerProcessController.smtp_controller.
395+ set_immediate_mail_delivery(True)
396+
397 team_name = None
398 if len(self.args) == 0:
399 self.parser.error('Missing team name')

Subscribers

People subscribed via source and target branches

to status/vote changes: