Launchpad itself

Code review comment for lp:~barry/launchpad/403606-expat

403606-expat
Merge into devel

Revision history for this message

Barry Warsaw (barry) wrote on 2009-10-01:

= Summary =

Bug 403606 is a fun one. For a long while now, we've been getting mysterious
OOPSes in the private XMLRPC server that Mailman talks too. Unfortunately,
Zope swallows all the information that would be useful to diagnose the
problem. OTOH, it never seemed to degrade the service, but it annoyed the
LOSAs and helping them out is always a good thing!

Now that 3.0 is out, it was time to look into this problem, so with the LOSAs
help, I instrumented the private server to gather the additional information.
Specifically, I added some code so that when the XMLRPC request failed, I
logged the entire request body. We ran that code overnight and it was obvious
what the problem was.

It turns out that IMailingListAPIView.holdMessage() is the culprit, and
specifically it's when Mailman tries to hold messages with non-ascii
characters in its headers or body. This makes sense as non-ascii must be
encoded over the wire for XMLRPC.

The downside is that all the messages in the overnight log that I could see
that triggered this were spam. That jives with my experience in Mailman (why
spammers would send at worst illegal or at best often incompatible messages is
beyond me, but there ya go). So the good news is that this should eliminate
or at least largely reduce the OOPSes we're seeing, but the bad news is that
it will probably increase the amount of spam list owners have to moderate.
That is a separate problem though, out of scope for this branch (and we have
ways of mitigating that).

== Proposed fix ==

On the XMLRPC client side in Mailman, wrap the message body texts in an
xmlrpc.Binary for transport over the wire. On the private XMLRPC server side,
unwrap the Binary to get the text bytes of the original message.

== Pre-implementation notes ==

I didn't have any discussions specifically about this fix, but had several
discussions with the LOSAs, Brad, and Francis about how to diagnose the
problem.

== Implementation details ==

A few things to note. On the server side, I do a type-test of the 'bytes'
argument to see whether to unwrap the Binary or not. The reason I don't do
this unconditionally is that it would require a rewrite of all the existing
tests, where most of the message bodies are pure-ascii. So while the client
always sends a Binary, most of the tests will still send strings.

I paid some tech debt along the way. Apologies for cluttering up the meat of
this change.

== Tests ==

% bin/test -vv -t mailinglist-xmlrpc.txt
% bin/test -vv --layer=MailmanLayer -t create-list -t postings

Note that the MailmanLayer tests are sometimes flaky and are not run by 'make
check' by default. Still, you should be able to get through them, though ping
me if you can't.

== Demo and Q/A ==

There are no user visible changes on this branch, but we'll QA it by watching
the daily OOPS reports.

= Launchpad lint =

These lint warnings are bogus.

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/services/mailman/doc/postings.txt
  lib/canonical/launchpad/webapp/publication.py
  lib/canonical/launchpad/mailman/monkeypatches/lpmoderate.py
  lib/lp/registry/doc/message-holds-xmlrpc.txt
  lib/lp/services/mailman/doc/create-lists.txt
  lib/lp/registry/interfaces/mailinglist.py
  lib/canonical/launchpad/xmlrpc/mailinglist.py
  lib/lp/registry/doc/mailinglist-xmlrpc.txt
  lib/canonical/launchpad/webapp/servers.py

== Pyflakes Doctest notices ==

lib/lp/services/mailman/doc/postings.txt
    14: undefined name 'smtpd'
    35: undefined name 'vette_watcher'
    36: undefined name 'smtpd'
    73: undefined name 'vette_watcher'
    74: undefined name 'smtpd'
    88: undefined name 'smtpd'
    89: undefined name 'smtpd'
    99: undefined name 'vette_watcher'
    103: undefined name 'smtpd_watcher'
    104: undefined name 'smtpd'
    156: undefined name 'Browser'
    168: undefined name 'smtpd_watcher'
    169: undefined name 'smtpd'
    212: undefined name 'vette_watcher'
    213: undefined name 'smtpd'
    244: undefined name 'smtpd'
    259: undefined name 'vette_watcher'
    260: undefined name 'smtpd'
    306: undefined name 'smtpd_watcher'
    307: undefined name 'smtpd'
    378: undefined name 'smtpd_watcher'
    382: undefined name 'smtpd'
    405: undefined name 'mhonarc_watcher'
    434: undefined name 'smtpd_watcher'
    435: undefined name 'smtpd_watcher'
    436: undefined name 'smtpd'
    486: undefined name 'mhonarc_watcher'
    501: undefined name 'vette_watcher'
    502: undefined name 'smtpd'
    522: undefined name 'smtpd_watcher'
    523: undefined name 'smtpd_watcher'
    524: undefined name 'smtpd'
    595: undefined name 'vette_watcher'
    596: undefined name 'smtpd'
    639: undefined name 'smtpd_watcher'
    640: undefined name 'smtpd_watcher'
    641: undefined name 'smtpd'
    670: undefined name 'smtpd'
    686: undefined name 'vette_watcher'
    712: undefined name 'smtpd_watcher'
    713: undefined name 'smtpd_watcher'
    740: undefined name 'smtpd_watcher'
    741: undefined name 'smtpd_watcher'
    766: undefined name 'vette_watcher'
    781: undefined name 'smtpd_watcher'
    782: undefined name 'smtpd_watcher'
    786: undefined name 'smtpd'
    795: undefined name 'smtpd_watcher'
    796: undefined name 'smtpd_watcher'
    797: undefined name 'smtpd'
    832: undefined name 'vette_watcher'
    833: undefined name 'smtpd'
    857: undefined name 'vette_watcher'
    871: undefined name 'smtpd_watcher'
    872: undefined name 'smtpd_watcher'
    899: undefined name 'vette_watcher'
    927: undefined name 'vette_watcher'

lib/lp/services/mailman/doc/create-lists.txt
11: undefined name 'Browser'

== Pylint notices ==

lib/canonical/launchpad/webapp/publication.py
551: [E1002, LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super on an old style class

lib/lp/registry/interfaces/mailinglist.py
30: [F0401] Unable to import 'lazr.enum' (No module named enum)

lib/canonical/launchpad/webapp/servers.py
    44: [F0401] Unable to import 'lazr.restful.interfaces' (No module named restful)
    45: [F0401] Unable to import 'lazr.restful.publisher' (No module named restful)
    1087: [E1002, FeedsPublication.traverseName] Use super on an old style class
    1137: [E1002, WebServicePublication.getResource] Use super on an old style class
    1158: [E1002, WebServicePublication.getPrincipal] Use super on an old style class
    1288: [E1002, PrivateXMLRPCPublication.traverseName] Use super on an old style class
    1335: [E1002, ProtocolErrorPublication.__init__] Use super on an old style class

= Summary =

Bug 403606 is a fun one.  For a long while now, we've been getting mysterious
OOPSes in the private XMLRPC server that Mailman talks too.  Unfortunately,
Zope swallows all the information that would be useful to diagnose the
problem.  OTOH, it never seemed to degrade the service, but it annoyed the
LOSAs and helping them out is always a good thing!

Now that 3.0 is out, it was time to look into this problem, so with the LOSAs
help, I instrumented the private server to gather the additional information.
Specifically, I added some code so that when the XMLRPC request failed, I
logged the entire request body.  We ran that code overnight and it was obvious
what the problem was.

It turns out that IMailingListAPIView.holdMessage() is the culprit, and
specifically it's when Mailman tries to hold messages with non-ascii
characters in its headers or body.  This makes sense as non-ascii must be
encoded over the wire for XMLRPC.

The downside is that all the messages in the overnight log that I could see
that triggered this were spam.  That jives with my experience in Mailman (why
spammers would send at worst illegal or at best often incompatible messages is
beyond me, but there ya go).  So the good news is that this should eliminate
or at least largely reduce the OOPSes we're seeing, but the bad news is that
it will probably increase the amount of spam list owners have to moderate.
That is a separate problem though, out of scope for this branch (and we have
ways of mitigating that).

== Proposed fix ==

On the XMLRPC client side in Mailman, wrap the message body texts in an
xmlrpc.Binary for transport over the wire.  On the private XMLRPC server side,
unwrap the Binary to get the text bytes of the original message.

== Pre-implementation notes ==

I didn't have any discussions specifically about this fix, but had several
discussions with the LOSAs, Brad, and Francis about how to diagnose the
problem.

== Implementation details ==

A few things to note.  On the server side, I do a type-test of the 'bytes'
argument to see whether to unwrap the Binary or not.  The reason I don't do
this unconditionally is that it would require a rewrite of all the existing
tests, where most of the message bodies are pure-ascii.  So while the client
always sends a Binary, most of the tests will still send strings.

I paid some tech debt along the way.  Apologies for cluttering up the meat of
this change.

== Tests ==

% bin/test -vv -t mailinglist-xmlrpc.txt
% bin/test -vv --layer=MailmanLayer -t create-list -t postings

Note that the MailmanLayer tests are sometimes flaky and are not run by 'make
check' by default.  Still, you should be able to get through them, though ping
me if you can't.

== Demo and Q/A ==

There are no user visible changes on this branch, but we'll QA it by watching
the daily OOPS reports.

= Launchpad lint =

These lint warnings are bogus.

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

== Pyflakes Doctest notices ==

lib/lp/services/mailman/doc/create-lists.txt
    11: undefined name 'Browser'

== Pylint notices ==

lib/canonical/launchpad/webapp/publication.py
    551: [E1002, LaunchpadBrowserPublication.beginErrorHandlingTransaction] Use super on an old style class

lib/lp/registry/interfaces/mailinglist.py
    30: [F0401] Unable to import 'lazr.enum' (No module named enum)

« Back to merge proposal