launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03860
[Merge] lp:~bac/launchpad/bug-788874 into lp:launchpad
Brad Crittenden has proposed merging lp:~bac/launchpad/bug-788874 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #788874 in Launchpad itself: "mail handling stalls when a large message is received and not deliverable"
https://bugs.launchpad.net/launchpad/+bug/788874
For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-788874/+merge/63750
= Summary =
Large messages cause bug mail (and others) to OOPS, and then get wedged
when trying to send a non-delivery report which includes the supersized
incoming message.
== Proposed fix ==
1) Don't return the whole original message but just a truncated version,
2) Remove the bad message from the mailbox before attempting to send the
OOPS mail. This prevents it from wedging.
== Pre-implementation notes ==
Chat with Gary.
== Implementation details ==
As above.
== Tests ==
bin/test -vvt bugs-emailinterface.txt
== Demo and Q/A ==
None
= Launchpad lint =
Checking for conflicts and issues in changed files.
Will fix lint after the review to avoid polluting the diff.
Linting changed files:
lib/canonical/config/schema-lazr.conf
lib/canonical/launchpad/mailnotification.py
lib/lp/services/mail/incoming.py
lib/lp/bugs/tests/bugs-emailinterface.txt
./lib/canonical/config/schema-lazr.conf
540: Line exceeds 78 characters.
623: Line exceeds 78 characters.
999: Line exceeds 78 characters.
1092: Line exceeds 78 characters.
./lib/canonical/launchpad/mailnotification.py
52: Line exceeds 78 characters.
157: E225 missing whitespace around operator
./lib/lp/bugs/tests/bugs-emailinterface.txt
400: source exceeds 78 characters.
1712: source exceeds 78 characters.
1716: source exceeds 78 characters.
2105: source exceeds 78 characters.
--
https://code.launchpad.net/~bac/launchpad/bug-788874/+merge/63750
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-788874 into lp:launchpad.
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2011-06-07 05:02:53 +0000
+++ lib/canonical/config/schema-lazr.conf 2011-06-07 19:25:59 +0000
@@ -1732,7 +1732,7 @@
dbuser: processmail
storm_cache: generational
storm_cache_size: 500
-
+max_error_message_return_size: 65536
[process_apport_blobs]
# The database user which will be used by this process.
=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py 2011-03-10 19:11:04 +0000
+++ lib/canonical/launchpad/mailnotification.py 2011-06-07 19:25:59 +0000
@@ -9,6 +9,7 @@
__metaclass__ = type
from difflib import unified_diff
+from email import message_from_string
from email.MIMEMessage import MIMEMessage
from email.MIMEMultipart import MIMEMultipart
from email.MIMEText import MIMEText
@@ -43,21 +44,25 @@
CC = "CC"
+MAX_RETURNED_MESSAGE_SIZE = config.processmail.max_error_message_return_size
def send_process_error_notification(to_address, subject, error_msg,
- original_msg, failing_command=None):
+ original_msg, failing_command=None,
+ max_return_size=MAX_RETURNED_MESSAGE_SIZE):
"""Send a mail about an error occurring while using the email interface.
Tells the user that an error was encountered while processing his
request and attaches the original email which caused the error to
- happen.
+ happen. The original message will be truncated to
+ max_return_size bytes.
:to_address: The address to send the notification to.
:subject: The subject of the notification.
:error_msg: The error message that explains the error.
:original_msg: The original message sent by the user.
:failing_command: The command that caused the error to happen.
+ :max_return_size: The maximum size returned for the original message.
"""
if isinstance(failing_command, list):
failing_commands = failing_command
@@ -83,6 +88,10 @@
msg['From'] = get_bugmail_error_address()
msg['Subject'] = subject
msg.attach(error_part)
+ original_msg_str = str(original_msg)
+ if len(original_msg_str) > max_return_size:
+ truncated_msg = original_msg_str[:max_return_size]
+ original_msg = message_from_string(truncated_msg)
msg.attach(MIMEMessage(original_msg))
sendmail(msg)
=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt 2011-05-27 19:53:20 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2011-06-07 19:25:59 +0000
@@ -340,7 +340,7 @@
... Date: Fri Jun 17 10:10:23 BST 2005
... Subject: Not important
...
- ... summary "Even nicer summary"
+ ... summary "Even nicer summary"
... """
>>> process_email(edit_mail)
@@ -2026,7 +2026,7 @@
used to send the error messages. It needs the message that caused the
error, so let's create one.
- >>> msg = email.message_from_string("""From: foo.bar@xxxxxxxxxxxxx
+ >>> test_msg = email.message_from_string("""From: foo.bar@xxxxxxxxxxxxx
... To: bugs@xxxxxxxxxxxxx
... Message-Id: <original@msg>
... Subject: Original Message Subject
@@ -2043,7 +2043,7 @@
... send_process_error_notification)
>>> send_process_error_notification(
... sampledata.USER_EMAIL, 'Some subject', 'Some error message.',
- ... msg, failing_command=['foo bar'])
+ ... test_msg, failing_command=['foo bar'])
The To and Subject headers got set to the values we provided:
@@ -2095,6 +2095,23 @@
>>> print msg.get_payload(decode=True)
Original message body.
+Sometimes the original error was caused by the original message being
+too large. In that case we cannot really return the entire original
+message as our outgoing message will be too big. So, we can truncate
+the original message.
+
+ >>> max_return_size = len(str(test_msg)) / 2
+ >>> send_process_error_notification(
+ ... sampledata.USER_EMAIL, 'Some subject', 'Some error message.',
+ ... test_msg, failing_command=['foo bar'], max_return_size=max_return_size)
+ >>> commit()
+ >>> from_addr, to_addrs, raw_message = stub.test_emails[-1]
+ >>> sent_msg = email.message_from_string(raw_message)
+ >>> failure_msg, original_msg = sent_msg.get_payload()
+ >>> msg = original_msg.get_payload()[0]
+ >>> # Fudge due to new line added to the payload.
+ >>> len(str(msg)) <= (max_return_size + 1)
+ True
Error handling
--------------
=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py 2011-05-30 23:57:16 +0000
+++ lib/lp/services/mail/incoming.py 2011-06-07 19:25:59 +0000
@@ -110,7 +110,7 @@
def _authenticateDkim(signed_message):
- """"Attempt DKIM authentication of email; return True if known authentic
+ """Attempt DKIM authentication of email; return True if known authentic
:param signed_message: ISignedMessage
"""
@@ -368,9 +368,12 @@
log.exception(
"An exception was raised inside the handler:\n%s"
% (file_alias_url,))
+ # Delete the troublesome email before attempting to send the
+ # OOPS in case something goes wrong. Retrying probably
+ # wouldn't work and we'd get stuck on the bad message.
+ mailbox.delete(mail_id)
_send_email_oops(trans, log, mail,
"Unhandled exception", file_alias_url)
- mailbox.delete(mail_id)
finally:
log.info("Closing the mail box.")
mailbox.close()
@@ -414,7 +417,7 @@
def handle_one_mail(log, mail, file_alias, file_alias_url,
- signature_timestamp_checker):
+ signature_timestamp_checker):
"""Process one message.
Returns None when the message has either been successfully processed, or
