Merge lp:~jml/launchpad/better-slave-mock into lp:launchpad

[Jonathan Lange]

Move BlockingProxy to lp.services.twistedsupport.xmlrpc, make BuilderSlave use it.

This branch moves BlockingProxy from lp.codehosting.vfs and into lp.services.twistedsupport.xmlrpc. It then rejiggers the BuilderSlave class to use a callRemote style proxy rather than a getattr style one.

The branch also adds DeferredBlockingProxy, which is much like BlockingProxy, except its guaranteed to return Deferreds. It then changes codehosting to use this DeferredBlockingProxy. The net effect is to move generic code deeper down the stack, and make the codehosting vfs more focused on being a codehosting vfs.

I was going to actually improve the mocks, but I figured that would be easier to do when driven by actual tests.

[Michael Nelson (michael.nelson)]

Thanks Jonathan - a few notes below, but nothing that needs attention.

It was a good chance for me to learn a little more about twisted :)

> === modified file 'lib/lp/buildmaster/model/builder.py'
> --- lib/lp/buildmaster/model/builder.py 2010-09-21 18:43:27 +0000
> +++ lib/lp/buildmaster/model/builder.py 2010-09-23 08:14:12 +0000
> @@ -137,43 +138,55 @@
> # should make a test double that doesn't do any XML-RPC and can be used to
> # make testing easier & tests faster.
>
> - def __init__(self, urlbase, vm_host):
> - """Initialise a Server with specific parameter to our buildfarm."""
> - self.vm_host = vm_host
> - self.urlbase = urlbase
> - rpc_url = urlappend(urlbase, "rpc")
> - self._server = xmlrpclib.Server(
> + def __init__(self, proxy, file_cache_url, vm_host):
> + """Initialise a Server with specific parameter to our buildfarm.

Nothing of yours, but s/parameter/parameters/

> +
> + :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
> + support passing and returning None objects.
> + :param file_cache_url: The URL of the file cache.
> + :param vm_host: The VM host to use when resuming.
> + """
> + self._vm_host = vm_host
> + self._file_cache_url = file_cache_url
> + self._server = proxy
> +
> + @classmethod
> + def makeBlockingSlave(cls, url_base, vm_host):
> + rpc_url = urlappend(url_base, 'rpc')
> + server_proxy = xmlrpclib.ServerProxy(
> rpc_url, transport=TimeoutTransport(), allow_none=True)
> + file_cache_url = urlappend(url_base, 'filecache')
> + return cls(BlockingProxy(server_proxy), file_cache_url, vm_host)

I don't have a preference, but have noticed the code guys using static
methods unless you need to use the class for something other than
constructing (eg. lp.code.model.sourcepackagerecipe.SourcePackageRecipe.new())

> @@ -203,9 +216,10 @@
> :param libraryfilealias: An `ILibraryFileAlias`.
> """
> url = libraryfilealias.http_url
> - logger.debug("Asking builder on %s to ensure it has file %s "
> - "(%s, %s)" % (self.urlbase, libraryfilealias.filename,
> - url, libraryfilealias.content.sha1))
> + logger.debug(
> + "Asking builder on %s to ensure it has file %s (%s, %s)" % (

Ignore for now, but I wonder if we should start removing interpolation
operators from logger calls (to avoid interpolation errors). Ie.
logger.debug("...", arg1, arg2, arg3) (Although if our tests exercise all of
our logging calls, then we don't need to worry ;))

> === modified file 'lib/lp/services/twistedsupport/tests/test_xmlrpc.py'
> --- lib/lp/services/twistedsupport/tests/test_xmlrpc.py 2010-08-20 20:31:18 +0000
> +++ lib/lp/services/twistedsupport/tests/test_xmlrpc.py 2010-09-23 08:14:12 +0000
> @@ -86,5 +91,47 @@
> self.assertEqual(TestFaultOne.msg_template, fault.faultString)
>
>
> +class TestBlockingProxy(TestCase):
> +
> + def test_callRemote_calls_attribute(self):
> + class ExampleProxy:
> + def ok(self, a, b, c=None):
> + ...

review: Approve (code)

[Jonathan Lange (jml)]

On Thu, Sep 23, 2010 at 10:06 AM, Michael Nelson
<email address hidden> wrote:
> Review: Approve code
> Thanks Jonathan - a few notes below, but nothing that needs attention.
>
> It was a good chance for me to learn a little more about twisted :)
>
>> === modified file 'lib/lp/buildmaster/model/builder.py'
>> --- lib/lp/buildmaster/model/builder.py       2010-09-21 18:43:27 +0000
>> +++ lib/lp/buildmaster/model/builder.py       2010-09-23 08:14:12 +0000
>> @@ -137,43 +138,55 @@
>>      # should make a test double that doesn't do any XML-RPC and can be used to
>>      # make testing easier & tests faster.
>>
>> -    def __init__(self, urlbase, vm_host):
>> -        """Initialise a Server with specific parameter to our buildfarm."""
>> -        self.vm_host = vm_host
>> -        self.urlbase = urlbase
>> -        rpc_url = urlappend(urlbase, "rpc")
>> -        self._server = xmlrpclib.Server(
>> +    def __init__(self, proxy, file_cache_url, vm_host):
>> +        """Initialise a Server with specific parameter to our buildfarm.
>
> Nothing of yours, but s/parameter/parameters/
>

Fixed.

>> +
>> +        :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
>> +            support passing and returning None objects.
>> +        :param file_cache_url: The URL of the file cache.
>> +        :param vm_host: The VM host to use when resuming.
>> +        """
>> +        self._vm_host = vm_host
>> +        self._file_cache_url = file_cache_url
>> +        self._server = proxy
>> +
>> +    @classmethod
>> +    def makeBlockingSlave(cls, url_base, vm_host):
>> +        rpc_url = urlappend(url_base, 'rpc')
>> +        server_proxy = xmlrpclib.ServerProxy(
>>              rpc_url, transport=TimeoutTransport(), allow_none=True)
>> +        file_cache_url = urlappend(url_base, 'filecache')
>> +        return cls(BlockingProxy(server_proxy), file_cache_url, vm_host)
>
> I don't have a preference, but have noticed the code guys using static
> methods unless you need to use the class for something other than
> constructing (eg. lp.code.model.sourcepackagerecipe.SourcePackageRecipe.new())
>

I'm not sure why that is. My preference is for the classmethod
approach, which is often more suitable for subclassing.

>> @@ -203,9 +216,10 @@
>>          :param libraryfilealias: An `ILibraryFileAlias`.
>>          """
>>          url = libraryfilealias.http_url
>> -        logger.debug("Asking builder on %s to ensure it has file %s "
>> -                     "(%s, %s)" % (self.urlbase, libraryfilealias.filename,
>> -                                   url, libraryfilealias.content.sha1))
>> +        logger.debug(
>> +            "Asking builder on %s to ensure it has file %s (%s, %s)" % (
>
> Ignore for now, but I wonder if we should start removing interpolation
> operators from logger calls (to avoid interpolation errors).  Ie.
> logger.debug("...", arg1, arg2, arg3) (Although if our tests exercise all of
> our logging calls, then we don't need to worry ;))
>

Interesting thought.

>> === modified file 'lib/lp/services/twistedsupport/tests/test_xmlrpc.py'
>> --- lib/lp/services/twistedsupport/tests/test_xmlrpc.py       2010-08-20 20:31:18 +0000
>> ++...

=== modified file 'lib/lp/buildmaster/doc/builder.txt'
--- lib/lp/buildmaster/doc/builder.txt	2010-09-22 11:26:19 +0000
+++ lib/lp/buildmaster/doc/builder.txt	2010-09-23 12:39:48 +0000
@@ -43,16 +43,6 @@
     ...     builder.current_build_behavior, BinaryPackageBuildBehavior)
     True
 
-A builder has an XML-RPC proxy in the 'slave' attribute, which allows
-us to easily call methods on the slave machines.
-
-    >>> s = builder.slave
-
-The base URL of the proxy matches the builder's URL.
-
-    >>> s.urlbase == builder.url
-    True
-
 
 BuilderSet
 ==========

=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py	2010-09-21 18:43:27 +0000
+++ lib/lp/buildmaster/model/builder.py	2010-09-23 12:39:48 +0000
@@ -80,6 +80,7 @@
 from lp.services.job.model.job import Job
 from lp.services.osutils import until_no_eintr
 from lp.services.propertycache import cachedproperty
+from lp.services.twistedsupport.xmlrpc import BlockingProxy
 # XXX Michael Nelson 2010-01-13 bug=491330
 # These dependencies on soyuz will be removed when getBuildRecords()
 # is moved.
@@ -113,7 +114,11 @@
 
 
 class BuilderSlave(object):
-    """Add in a few useful methods for the XMLRPC slave."""
+    """Add in a few useful methods for the XMLRPC slave.
+
+    :ivar url: The URL of the actual builder. The XML-RPC resource and
+        the filecache live beneath this.
+    """
 
     # WARNING: If you change the API for this, you should also change the APIs
     # of the mocks in soyuzbuilderhelpers to match. Otherwise, you will have
@@ -137,43 +142,55 @@
     # should make a test double that doesn't do any XML-RPC and can be used to
     # make testing easier & tests faster.
 
-    def __init__(self, urlbase, vm_host):
-        """Initialise a Server with specific parameter to our buildfarm."""
-        self.vm_host = vm_host
-        self.urlbase = urlbase
-        rpc_url = urlappend(urlbase, "rpc")
-        self._server = xmlrpclib.Server(
+    def __init__(self, proxy, builder_url, vm_host):
+        """Initialize a BuilderSlave.
+
+        :param proxy: An XML-RPC proxy, implementing 'callRemote'. It must
+            support passing and returning None objects.
+        :param builder_url: The URL of the builder.
+        :param vm_host: The VM host to use when resuming.
+        """
+        self.url = builder_url
+        self._vm_host = vm_host
+        self._file_cache_url = urlappend(builder_url, 'filecache')
+        self._server = proxy
+
+    @classmethod
+    def makeBlockingSlave(cls, builder_url, vm_host):
+        rpc_url = urlappend(builder_url, 'rpc')
+        server_proxy = xmlrpclib.ServerProxy(
             rpc_url, transport=TimeoutTransport(), allow_none=True)
+        return cls(BlockingProxy(server_proxy), builder_url, vm_host)
 
     def abort(self):
         """Abort the current build."""
-        return self._server.abort()
+        return self._server.callRemote('abort')
 
     def clean(self):
         """Clean up the waiting files and reset the slave's internal state."""
-        return self._server.clean()
+        return self._server.callRemote('clean')
 
     def echo(self, *args):
         """Echo the arguments back."""
-        return self._server.echo(*args)
+        return self._server.callRemote('echo', *args)
 
     def info(self):
         """Return the protocol version and the builder methods supported."""
-        return self._server.info()
+        return self._server.callRemote('info')
 
     def status(self):
         """Return the status of the build daemon."""
-        return self._server.status()
+        return self._server.callRemote('status')
 
     def ensurepresent(self, sha1sum, url, username, password):
         """Attempt to ensure the given file is present."""
-        return self._server.ensurepresent(sha1sum, url, username, password)
+        return self._server.callRemote(
+            'ensurepresent', sha1sum, url, username, password)
 
     def getFile(self, sha_sum):
         """Construct a file-like object to return the named file."""
-        filelocation = "filecache/%s" % sha_sum
-        fileurl = urlappend(self.urlbase, filelocation)
-        return urllib2.urlopen(fileurl)
+        file_url = urlappend(self._file_cache_url, sha_sum)
+        return urllib2.urlopen(file_url)
 
     def resume(self):
         """Resume a virtual builder.
@@ -188,7 +205,7 @@
         # always want to do this asynchronously, there's no need for the
         # duplication.
         resume_command = config.builddmaster.vm_resume_command % {
-            'vm_host': self.vm_host}
+            'vm_host': self._vm_host}
         resume_argv = resume_command.split()
         resume_process = subprocess.Popen(
             resume_argv, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
@@ -203,9 +220,10 @@
         :param libraryfilealias: An `ILibraryFileAlias`.
         """
         url = libraryfilealias.http_url
-        logger.debug("Asking builder on %s to ensure it has file %s "
-                     "(%s, %s)" % (self.urlbase, libraryfilealias.filename,
-                                   url, libraryfilealias.content.sha1))
+        logger.debug(
+            "Asking builder on %s to ensure it has file %s (%s, %s)" % (
+                self._file_cache_url, libraryfilealias.filename, url,
+                libraryfilealias.content.sha1))
         self.sendFileToSlave(libraryfilealias.content.sha1, url)
 
     def sendFileToSlave(self, sha1, url, username="", password=""):
@@ -226,8 +244,8 @@
             the build job type.
         """
         try:
-            return self._server.build(
-                buildid, builder_type, chroot_sha1, filemap, args)
+            return self._server.callRemote(
+                'build', buildid, builder_type, chroot_sha1, filemap, args)
         except xmlrpclib.Fault, info:
             raise BuildSlaveFailure(info)
 
@@ -444,7 +462,7 @@
         # the slave object, which is usually an XMLRPC client, with a
         # stub object that removes the need to actually create a buildd
         # slave in various states - which can be hard to create.
-        return BuilderSlave(self.url, self.vm_host)
+        return BuilderSlave.makeBlockingSlave(self.url, self.vm_host)
 
     def setSlaveForTesting(self, proxy):
         """See IBuilder."""

=== modified file 'lib/lp/buildmaster/tests/mock_slaves.py'
--- lib/lp/buildmaster/tests/mock_slaves.py	2010-09-22 14:21:18 +0000
+++ lib/lp/buildmaster/tests/mock_slaves.py	2010-09-23 12:39:48 +0000
@@ -43,7 +43,7 @@
         self.builderok = True
         self.manual = False
         self.url = 'http://fake:0000'
-        slave.urlbase = self.url
+        slave.url = self.url
         self.name = name
         self.virtualized = True
 

=== modified file 'lib/lp/buildmaster/tests/test_builder.py'
--- lib/lp/buildmaster/tests/test_builder.py	2010-09-22 11:26:19 +0000
+++ lib/lp/buildmaster/tests/test_builder.py	2010-09-23 12:39:48 +0000
@@ -129,6 +129,13 @@
         # single EINTR.
         self.assertEqual(0, builder.handleTimeout.call_count)
 
+    def test_slave(self):
+        # Builder.slave is a BuilderSlave that points at the actual Builder.
+        # The Builder is only ever used in scripts that run outside of the
+        # security context.
+        builder = removeSecurityProxy(self.factory.makeBuilder())
+        self.assertEqual(builder.url, builder.slave.url)
+
 
 class Test_rescueBuilderIfLost(TestCaseWithFactory):
     """Tests for lp.buildmaster.model.builder.rescueBuilderIfLost."""
@@ -501,7 +508,7 @@
 
         Points to a fixed URL that is also used by `BuilddSlaveTestSetup`.
         """
-        return BuilderSlave(self.TEST_URL, 'vmhost')
+        return BuilderSlave.makeBlockingSlave(self.TEST_URL, 'vmhost')
 
     def makeCacheFile(self, tachandler, filename):
         """Make a cache file available on the remote slave.

=== modified file 'lib/lp/codehosting/inmemory.py'
--- lib/lp/codehosting/inmemory.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/inmemory.py	2010-09-23 12:39:48 +0000
@@ -16,6 +16,7 @@
     escape,
     unescape,
     )
+from twisted.internet import defer
 from zope.component import (
     adapter,
     getSiteManager,
@@ -913,8 +914,11 @@
     def __init__(self, endpoint):
         self.endpoint = endpoint
 
-    def callRemote(self, method_name, *args):
+    def _callRemote(self, method_name, *args):
         result = getattr(self.endpoint, method_name)(*args)
         if isinstance(result, Fault):
             raise result
         return result
+
+    def callRemote(self, method_name, *args):
+        return defer.maybeDeferred(self._callRemote, method_name, *args)

=== modified file 'lib/lp/codehosting/vfs/__init__.py'
--- lib/lp/codehosting/vfs/__init__.py	2010-08-21 07:49:13 +0000
+++ lib/lp/codehosting/vfs/__init__.py	2010-09-23 12:39:48 +0000
@@ -5,7 +5,6 @@
 
 __all__ = [
     'AsyncLaunchpadTransport',
-    'BlockingProxy',
     'branch_id_to_path',
     'BranchFileSystemClient',
     'get_lp_server',
@@ -25,7 +24,6 @@
     make_branch_mirrorer,
     )
 from lp.codehosting.vfs.branchfsclient import (
-    BlockingProxy,
     BranchFileSystemClient,
     )
 

=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2010-09-23 12:39:48 +0000
@@ -102,7 +102,6 @@
     )
 from lp.codehosting.bzrutils import get_stacked_on_url
 from lp.codehosting.vfs.branchfsclient import (
-    BlockingProxy,
     BranchFileSystemClient,
     )
 from lp.codehosting.vfs.transport import (
@@ -112,7 +111,10 @@
     get_readonly_transport,
     TranslationError,
     )
-from lp.services.twistedsupport.xmlrpc import trap_fault
+from lp.services.twistedsupport.xmlrpc import (
+    DeferredBlockingProxy,
+    trap_fault,
+    )
 
 
 class BadUrl(Exception):
@@ -189,7 +191,7 @@
 def get_ro_server():
     """Get a Launchpad internal server for scanning branches."""
     proxy = xmlrpclib.ServerProxy(config.codehosting.codehosting_endpoint)
-    codehosting_endpoint = BlockingProxy(proxy)
+    codehosting_endpoint = DeferredBlockingProxy(proxy)
     branch_transport = get_readonly_transport(
         get_transport(config.codehosting.internal_branch_by_id_root))
     return LaunchpadInternalServer(
@@ -212,7 +214,7 @@
         return DirectDatabaseLaunchpadServer('lp-internal:///', transport)
     else:
         proxy = xmlrpclib.ServerProxy(config.codehosting.codehosting_endpoint)
-        codehosting_endpoint = BlockingProxy(proxy)
+        codehosting_endpoint = DeferredBlockingProxy(proxy)
         return LaunchpadInternalServer(
             'lp-internal:///', codehosting_endpoint, transport)
 
@@ -719,7 +721,7 @@
 
     codehosting_client = xmlrpclib.ServerProxy(codehosting_endpoint_url)
     lp_server = LaunchpadServer(
-        BlockingProxy(codehosting_client), user_id, branch_transport,
+        DeferredBlockingProxy(codehosting_client), user_id, branch_transport,
         seen_new_branch_hook)
     return lp_server
 

=== modified file 'lib/lp/codehosting/vfs/branchfsclient.py'
--- lib/lp/codehosting/vfs/branchfsclient.py	2010-08-10 03:24:22 +0000
+++ lib/lp/codehosting/vfs/branchfsclient.py	2010-09-23 12:39:48 +0000
@@ -8,7 +8,6 @@
 
 __metaclass__ = type
 __all__ = [
-    'BlockingProxy',
     'BranchFileSystemClient',
     'NotInCache',
     ]
@@ -20,15 +19,6 @@
 from lp.code.interfaces.codehosting import BRANCH_TRANSPORT
 
 
-class BlockingProxy:
-
-    def __init__(self, proxy):
-        self._proxy = proxy
-
-    def callRemote(self, method_name, *args):
-        return getattr(self._proxy, method_name)(*args)
-
-
 class NotInCache(Exception):
     """Raised when we try to get a path from the cache that's not present."""
 
@@ -50,7 +40,8 @@
                  seen_new_branch_hook=None, _now=time.time):
         """Construct a caching codehosting_endpoint.
 
-        :param codehosting_endpoint: An XML-RPC proxy that implements callRemote.
+        :param codehosting_endpoint: An XML-RPC proxy that implements
+            callRemote and returns Deferreds.
         :param user_id: The database ID of the user who will be making these
             requests. An integer.
         :param expiry_time: If supplied, only cache the results of
@@ -116,9 +107,8 @@
         :param branch_path: The path to the branch to create.
         :return: A `Deferred` that fires the ID of the created branch.
         """
-        return defer.maybeDeferred(
-            self._codehosting_endpoint.callRemote, 'createBranch',
-            self._user_id, branch_path)
+        return self._codehosting_endpoint.callRemote(
+            'createBranch', self._user_id, branch_path)
 
     def branchChanged(self, branch_id, stacked_on_url, last_revision_id,
                       control_string, branch_string, repository_string):
@@ -126,8 +116,7 @@
 
         :param branch_id: The database ID of the branch.
         """
-        return defer.maybeDeferred(
-            self._codehosting_endpoint.callRemote,
+        return self._codehosting_endpoint.callRemote(
             'branchChanged', self._user_id, branch_id, stacked_on_url,
             last_revision_id, control_string, branch_string,
             repository_string)
@@ -137,8 +126,7 @@
         try:
             return defer.succeed(self._getFromCache(path))
         except NotInCache:
-            deferred = defer.maybeDeferred(
-                self._codehosting_endpoint.callRemote,
+            deferred = self._codehosting_endpoint.callRemote(
                 'translatePath', self._user_id, path)
             deferred.addCallback(self._addToCache, path)
             return deferred

=== modified file 'lib/lp/codehosting/vfs/tests/test_transport.py'
--- lib/lp/codehosting/vfs/tests/test_transport.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/vfs/tests/test_transport.py	2010-09-23 12:39:48 +0000
@@ -19,7 +19,7 @@
 from lp.codehosting.inmemory import InMemoryFrontend
 from lp.codehosting.tests.helpers import TestResultWrapper
 from lp.codehosting.vfs.branchfs import LaunchpadInternalServer
-from lp.codehosting.vfs.branchfsclient import BlockingProxy
+from lp.services.twistedsupport.xmlrpc import DeferredBlockingProxy
 
 
 class TestingServer(LaunchpadInternalServer):
@@ -44,7 +44,8 @@
         # unreliable for tests that involve particular errors.
         LaunchpadInternalServer.__init__(
             self, 'lp-testing-%s:///' % id(self),
-            BlockingProxy(branchfs), LocalTransport(local_path_to_url('.')))
+            DeferredBlockingProxy(branchfs),
+            LocalTransport(local_path_to_url('.')))
         self._chroot_servers = []
 
     def get_bogus_url(self):

=== modified file 'lib/lp/services/twistedsupport/tests/test_xmlrpc.py'
--- lib/lp/services/twistedsupport/tests/test_xmlrpc.py	2010-08-20 20:31:18 +0000
+++ lib/lp/services/twistedsupport/tests/test_xmlrpc.py	2010-09-23 12:39:48 +0000
@@ -10,7 +10,12 @@
 from twisted.python.failure import Failure
 from twisted.trial.unittest import TestCase
 
-from lp.services.twistedsupport.xmlrpc import trap_fault
+from lp.services.twistedsupport import extract_result
+from lp.services.twistedsupport.xmlrpc import (
+    BlockingProxy,
+    DeferredBlockingProxy,
+    trap_fault,
+    )
 from lp.services.xmlrpc import LaunchpadFault
 
 
@@ -86,5 +91,47 @@
         self.assertEqual(TestFaultOne.msg_template, fault.faultString)
 
 
+class TestBlockingProxy(TestCase):
+
+    def test_callRemote_calls_attribute(self):
+        class ExampleProxy:
+            def ok(self, a, b, c=None):
+                return (c, b, a)
+        proxy = BlockingProxy(ExampleProxy())
+        result = proxy.callRemote('ok', 2, 3, c=8)
+        self.assertEqual((8, 3, 2), result)
+
+    def test_callRemote_reraises_attribute(self):
+        class ExampleProxy:
+            def not_ok(self, a, b, c=None):
+                raise RuntimeError((c, b, a))
+        proxy = BlockingProxy(ExampleProxy())
+        error = self.assertRaises(
+            RuntimeError, proxy.callRemote, 'not_ok', 2, 3, c=8)
+        self.assertEqual(str(error), str((8, 3, 2)))
+
+
+class TestDeferredBlockingProxy(TestCase):
+
+    def test_callRemote_calls_attribute(self):
+        class ExampleProxy:
+            def ok(self, a, b, c=None):
+                return (c, b, a)
+        proxy = DeferredBlockingProxy(ExampleProxy())
+        d = proxy.callRemote('ok', 2, 3, c=8)
+        result = extract_result(d)
+        self.assertEqual((8, 3, 2), result)
+
+    def test_callRemote_traps_failure(self):
+        class ExampleProxy:
+            def not_ok(self, a, b, c=None):
+                raise RuntimeError((c, b, a))
+        proxy = DeferredBlockingProxy(ExampleProxy())
+        d = proxy.callRemote('not_ok', 2, 3, c=8)
+        error = self.assertRaises(RuntimeError, extract_result, d)
+        self.assertEqual(str(error), str((8, 3, 2)))
+
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/services/twistedsupport/xmlrpc.py'
--- lib/lp/services/twistedsupport/xmlrpc.py	2010-04-09 12:39:23 +0000
+++ lib/lp/services/twistedsupport/xmlrpc.py	2010-09-23 12:39:48 +0000
@@ -5,12 +5,52 @@
 
 __metaclass__ = type
 __all__ = [
+    'BlockingProxy',
+    'DeferredBlockingProxy',
     'trap_fault',
     ]
 
+from twisted.internet import defer
 from twisted.web.xmlrpc import Fault
 
 
+class BlockingProxy:
+    """Make an xmlrpclib.ServerProxy behave like a Twisted XML-RPC proxy.
+
+    This is useful for writing code that needs to work in both a synchronous
+    and asynchronous fashion.
+
+    Also, some people prefer the callRemote style of invocation, which is more
+    explicit.
+    """
+
+    def __init__(self, proxy):
+        """Construct a `BlockingProxy`.
+
+        :param proxy: An xmlrpclib.ServerProxy.
+        """
+        self._proxy = proxy
+
+    def callRemote(self, method_name, *args, **kwargs):
+        return getattr(self._proxy, method_name)(*args, **kwargs)
+
+
+class DeferredBlockingProxy(BlockingProxy):
+    """Make an xmlrpclib.ServerProxy behave more like a Twisted XML-RPC proxy.
+
+    This is almost exactly like 'BlockingProxy', except that this returns
+    Deferreds. It is guaranteed to be exactly as synchronous as the passed-in
+    proxy. That means if you pass in a normal xmlrpclib proxy you ought to be
+    able to use `lp.services.twistedsupport.extract_result` to get the result.
+    """
+
+    def callRemote(self, method_name, *args, **kwargs):
+        return defer.maybeDeferred(
+            super(DeferredBlockingProxy, self).callRemote,
+            method_name, *args, **kwargs)
+
+
+
 def trap_fault(failure, *fault_classes):
     """Trap a fault, based on fault code.