launchpad-reviewers team mailing list archive

[Martin Packman <martin.packman@xxxxxxxxxxxxx>]

Martin Packman has proposed merging lp:~gz/launchpad/py27_xmlrpc_transport_timeout into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~gz/launchpad/py27_xmlrpc_transport_timeout/+merge/112528

In Python 2.7 xmlrpclib started using HTTPConnection directly rather than the legacy compat HTTP wrapper around it. The custom Transport subclass that passes down a socket timeout therefore needed fixing to return the right thing from make_connection which varies by version. Using the existing make_connection method and poking the timeout attribute onto the correct class reduces the complexity and avoids needing to care (as much) about which version of xmlrpclib is used.
-- 
https://code.launchpad.net/~gz/launchpad/py27_xmlrpc_transport_timeout/+merge/112528
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gz/launchpad/py27_xmlrpc_transport_timeout into lp:launchpad.

=== modified file 'lib/lp/services/tests/test_xmlrpc.py'
--- lib/lp/services/tests/test_xmlrpc.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/tests/test_xmlrpc.py	2012-06-28 09:59:24 +0000
@@ -8,22 +8,12 @@
 import httplib
 
 from lp.services.xmlrpc import (
-    HTTP,
     Transport,
     )
 from lp.testing import TestCase
 from lp.testing.layers import BaseLayer
 
 
-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"""
 
@@ -39,13 +29,8 @@
         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']
+        transport = Transport(timeout=25)
+        http = transport.make_connection('localhost')
+        # See logic in lp.services.xmlrpc.Transport.make_connection
+        http = getattr(http, "_conn", http)
+        self.assertEqual(25, http.timeout)

=== modified file 'lib/lp/services/xmlrpc.py'
--- lib/lp/services/xmlrpc.py	2011-07-27 14:01:17 +0000
+++ lib/lp/services/xmlrpc.py	2012-06-28 09:59:24 +0000
@@ -9,7 +9,6 @@
     'Transport',
     ]
 
-import httplib
 import socket
 import xmlrpclib
 
@@ -43,15 +42,6 @@
         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.
 
@@ -65,4 +55,10 @@
         self.timeout = timeout
 
     def make_connection(self, host):
-        return HTTP(host, timeout=self.timeout)
+        conn = xmlrpclib.Transport.make_connection(self, host)
+        # In Python 2.6 make_connection returns a legacy HTTP wrapper object
+        # around the HTTPConnection, 2.7 returns the HTTPConnection directly.
+        # No connection is yet opened so it's okay to set timeout after init.
+        real_conn = getattr(conn, "_conn", conn)
+        real_conn.timeout = self.timeout
+        return conn