launchpad-reviewers team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/launchpad/bug791492 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #791492 in Launchpad itself: "mailman crashing in production"
  https://bugs.launchpad.net/launchpad/+bug/791492

For more details, see:
https://code.launchpad.net/~gary/launchpad/bug791492/+merge/69469

This branch adds a timeout for mailman's xmlrpc serverproxy of Launchpad.  It's fairly straightforward.  The only trick is that, while httplib's HTTPConnection provides a timeout argument, the legacy HTTP class, essentially a proxy for HTTPConnection, does not.

All my tests are pure surface-level unit tests: I'm assuming that the lower levels handle timeouts appropriately.

lint is happy.

Thanks!

Gary
-- 
https://code.launchpad.net/~gary/launchpad/bug791492/+merge/69469
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug791492 into lp:launchpad.

=== modified file 'lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py'
--- lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py	2011-01-18 22:37:35 +0000
+++ lib/lp/services/mailman/monkeypatches/xmlrpcrunner.py	2011-07-27 14:06:45 +0000
@@ -39,6 +39,7 @@
 # XXX sinzui 2008-08-15 bug=258423:
 # We should be importing from lazr.errorlog.
 from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
+from lp.services.xmlrpc import Transport
 
 
 COMMASPACE = ', '
@@ -52,7 +53,8 @@
 
 
 def get_mailing_list_api_proxy():
-    return xmlrpclib.ServerProxy(mm_cfg.XMLRPC_URL)
+    return xmlrpclib.ServerProxy(
+        mm_cfg.XMLRPC_URL, transport=Transport(timeout=5))
 
 
 class MailmanErrorUtility(ErrorReportingUtility):
@@ -313,20 +315,17 @@
                 # See if the case is changing.
                 current_address = mlist.getMemberCPAddress(address)
                 future_address = key_to_case_preserved[address]
-                # pylint: disable-msg=W0331
-                if current_address <> future_address:
+                if current_address != future_address:
                     mlist.changeMemberAddress(address, future_address)
                     found_updates.append('%s -> %s' %
                                          (address, future_address))
                 # flags are ignored for now.
                 realname, flags, status = member_map[future_address]
-                # pylint: disable-msg=W0331
-                if realname <> mlist.getMemberName(address):
+                if realname != mlist.getMemberName(address):
                     mlist.setMemberName(address, realname)
                     found_updates.append('%s new name: %s' %
                                          (address, realname))
-                # pylint: disable-msg=W0331
-                if status <> mlist.getDeliveryStatus(address):
+                if status != mlist.getDeliveryStatus(address):
                     mlist.setDeliveryStatus(address, status)
                     found_updates.append('%s new status: %s' %
                                          (address, status))
@@ -341,10 +340,9 @@
     def _get_subscriptions(self):
         """Get the latest subscription information."""
         # First, calculate the names of the active mailing lists.
-        # pylint: disable-msg=W0331
         lists = sorted(list_name
                        for list_name in Utils.list_names()
-                       if list_name <> mm_cfg.MAILMAN_SITE_LIST)
+                       if list_name != mm_cfg.MAILMAN_SITE_LIST)
         # Batch the subscription requests in order to reduce the possibility
         # of timeouts in the XMLRPC server.  Note that we cannot eliminate
         # timeouts, which will cause an entire batch to fail.  To reduce the

=== modified file 'lib/lp/services/mailman/tests/test_xmlrpcrunner.py'
--- lib/lp/services/mailman/tests/test_xmlrpcrunner.py	2011-01-18 02:42:08 +0000
+++ lib/lp/services/mailman/tests/test_xmlrpcrunner.py	2011-07-27 14:06:45 +0000
@@ -16,11 +16,19 @@
     XMLRPCRunner,
     )
 
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+    BaseLayer,
+    DatabaseFunctionalLayer,
+    )
+from lp.services.mailman.monkeypatches.xmlrpcrunner import (
+    get_mailing_list_api_proxy,
+    )
 from lp.services.mailman.testing import (
      get_mailing_list_api_test_proxy,
      MailmanTestCase,
      )
+from lp.services.xmlrpc import Transport
+from lp.testing import TestCase
 
 
 @contextmanager
@@ -39,7 +47,21 @@
     try:
         yield
     finally:
-        runner._check_list_actions= original__check_list_actions
+        runner._check_list_actions = original__check_list_actions
+
+
+class TestXMLRPCRunnerTimeout(TestCase):
+    """Make sure that we set a timeout on our xmlrpc connections."""
+
+    layer = BaseLayer
+
+    def test_timeout_used(self):
+        proxy = get_mailing_list_api_proxy()
+        # We don't want to trigger the proxy if we misspell something, so we
+        # look in the dict.
+        transport = proxy.__dict__['_ServerProxy__transport']
+        self.assertTrue(isinstance(transport, Transport))
+        self.assertEqual(5, transport.timeout)
 
 
 class TestXMLRPCRunnerHeatBeat(MailmanTestCase):

=== added file 'lib/lp/services/tests/test_xmlrpc.py'
--- lib/lp/services/tests/test_xmlrpc.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/tests/test_xmlrpc.py	2011-07-27 14:06:45 +0000
@@ -0,0 +1,51 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the generic and/or shared xmlrpc code that Launchpad provides."""
+
+__metaclass__ = type
+
+import httplib
+
+from canonical.testing.layers import BaseLayer
+from lp.services.xmlrpc import (
+    HTTP,
+    Transport,
+    )
+from lp.testing import TestCase
+
+
+class DummyConnectionClass:
+    def __init__(self, *args, **kwargs):
+        self.args = args
+        self.kwargs = kwargs
+
+    def __getattr__(self, name):
+        return name
+
+
+class TestTransport(TestCase):
+    """Test code that allows xmlrpclib.ServerProxy to have a socket timeout"""
+
+    layer = BaseLayer
+
+    def test_default_initialization(self):
+        transport = Transport()
+        conn = httplib.HTTPConnection('localhost')
+        self.assertEqual(conn.timeout, transport.timeout)
+
+    def test_custom_initialization(self):
+        transport = Transport(timeout=25)
+        self.assertEqual(25, transport.timeout)
+
+    def test_timeout_passed_to_connection(self):
+        # The _connection_class is actually set on a parent class.  We verify
+        # this, so we can just delete it from the class at the end.
+        self.assertEqual(self, HTTP.__dict__.get('_connection_class', self))
+        HTTP._connection_class = DummyConnectionClass
+        try:
+            transport = Transport(timeout=25)
+            http = transport.make_connection('localhost')
+            self.assertEqual(25, http._conn.kwargs['timeout'])
+        finally:
+            del HTTP.__dict__['_connection_class']

=== modified file 'lib/lp/services/xmlrpc.py'
--- lib/lp/services/xmlrpc.py	2010-04-09 12:58:01 +0000
+++ lib/lp/services/xmlrpc.py	2011-07-27 14:06:45 +0000
@@ -6,8 +6,11 @@
 __metaclass__ = type
 __all__ = [
     'LaunchpadFault',
+    'Transport',
     ]
 
+import httplib
+import socket
 import xmlrpclib
 
 
@@ -38,3 +41,28 @@
 
     def __ne__(self, other):
         return not (self == other)
+
+
+class HTTP(httplib.HTTP):
+    """A version of httplib.HTTP with a timeout argument."""
+
+    def __init__(self, host='', port=None, strict=None,
+                 timeout=socket._GLOBAL_DEFAULT_TIMEOUT):
+        self._setup(
+            self._connection_class(host, port, strict, timeout=timeout))
+
+
+class Transport(xmlrpclib.Transport):
+    """An xmlrpclib transport that supports a timeout argument.
+
+    Use by passing into the "transport" argument of the xmlrpclib.ServerProxy
+    initialization.
+    """
+
+    def __init__(self,
+                 use_datetime=0, timeout=socket._GLOBAL_DEFAULT_TIMEOUT):
+        xmlrpclib.Transport.__init__(self, use_datetime)
+        self.timeout = timeout
+
+    def make_connection(self, host):
+        return HTTP(host, timeout=self.timeout)