Merge lp:~free.ekanayaka/landscape-client/amp-cleanup-9 into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

Alright, one of the last episodes of the AMP cleanup saga. In this branch:

- Drop landscape.lib.amp.RemoteObjectConnector, as most of its functionality has no been pushed down to MethodCallClientFactory which is a more standard Twisted primitive more importantly and is transport-agnostic, making landscape.lib.amp really generic.

- Convert landscape.lib.tests.test_amp.RemoteObjectConnectorTest to MethodCallFunctionalTest. The RemoteObjectConnectorTest class was exercising the (now gone) RemoteObjectConnector in a functional-like way, that means spinning the real twisted reactor and listening/connecting to real Unix sockets. Even if from a strict unit-test point of view all the functionality in landscape.lib.amp is already tested, I felt it was nice to still have some functional-like coverage mainly to protect us against regressions or API changes in the Twisted machinery that we depend on. I've used @inlineCallbacks to make tests a bit more readable.

Alright, one of the last episodes of the AMP cleanup saga. In this branch:

- Drop landscape.lib.amp.RemoteObjectConnector, as most of its functionality has no been pushed down to MethodCallClientFactory which is a more standard Twisted primitive more importantly and is transport-agnostic, making landscape.lib.amp really generic.

- Convert landscape.lib.tests.test_amp.RemoteObjectConnectorTest to MethodCallFunctionalTest. The RemoteObjectConnectorTest class was exercising the (now gone) RemoteObjectConnector in a functional-like way, that means spinning the real twisted reactor and listening/connecting to real Unix sockets. Even if from strict unit-test point of view all the functionality in landscape.lib.amp is already tested, I felt it was nice to still have some functional-like coverage mainly to protect us against regressions or API changes in the Twisted machinery that we depend on. I've used @inlineCallbacks to make tests a bit more readable.

[Chris Glass (tribaal)]

Great branch! Thanks a lot for all this refactoring, the code is much, much cleaner and easier to read now. +1
Man, inline callbacks + yield is so much nicer :)

[1]
While not directly related to your branch, could you please rename the "object" variable to "obj" or something in MethodCallReceiver's __init__()? It's not hugely important in this particular case, but overriding builtins even for a limited scope is a bit ugly :)

[2]
Same comment in MethodCallServerProtocol.

[3]
Same comment in MethodCallServerFactory's __init__

review: Approve

[Christopher Armstrong (radix)]

Nothing at all offensive about this branch. I like that almost all of the points are "remove foo" :-) +1

review: Approve

[lp:~free.ekanayaka/landscape-client/amp-cleanup-9]

674. By Free Ekanayaka on 2013-05-07

Rename object to obj

[Free Ekanayaka (free.ekanayaka)]

Thanks! [1] to [3] fixed.

=== modified file 'landscape/lib/amp.py'
--- landscape/lib/amp.py	2013-05-06 18:15:21 +0000
+++ landscape/lib/amp.py	2013-05-07 15:57:26 +0000
@@ -3,7 +3,7 @@
 This module implements an AMP-based protocol for performing remote procedure
 calls in a convenient and easy way. It's conceptually similar to DBus in that
 it supports exposing a Python object to a remote process, with communication
-happening over plain Unix domain sockets.
+happening over any Twisted-supported transport, e.g. Unix domain sockets.
 
 For example let's say we have a Python process "A" that creates an instance of
 this class::
@@ -35,8 +35,8 @@
     factory.getRemoteObject().addCallback(got_remote)
 
 Note that when invoking a method via the remote proxy, the parameters
-are required to be serializable with bpickle, so that they can be sent
-over the wire.
+are required to be serializable with bpickle, so they can be sent over
+the wire.
 
 See also::
 
@@ -130,14 +130,14 @@
 class MethodCallReceiver(CommandLocator):
     """Expose methods of a local object over AMP.
 
-    @param object: The Python object to be exposed.
+    @param obj: The Python object to be exposed.
     @param methods: The list of the object's methods that can be called
          remotely.
     """
 
-    def __init__(self, object, methods):
+    def __init__(self, obj, methods):
         CommandLocator.__init__(self)
-        self._object = object
+        self._object = obj
         self._methods = methods
         self._pending_chunks = {}
 
@@ -293,8 +293,8 @@
 class MethodCallServerProtocol(AMP):
     """Receive L{MethodCall} commands over the wire and send back results."""
 
-    def __init__(self, object, methods):
-        AMP.__init__(self, locator=MethodCallReceiver(object, methods))
+    def __init__(self, obj, methods):
+        AMP.__init__(self, locator=MethodCallReceiver(obj, methods))
 
 
 class MethodCallClientProtocol(AMP):
@@ -419,7 +419,7 @@
         @param protocol: The newly connected protocol instance.
         """
         self._sender = MethodCallSender(protocol, self._factory.clock)
-        if self._retry_on_reconnect:
+        if self._factory.retryOnReconnect:
             self._retry()
 
     def _retry(self):
@@ -442,14 +442,14 @@
 
     protocol = MethodCallServerProtocol
 
-    def __init__(self, object, methods):
+    def __init__(self, obj, methods):
         """
         @param object: The object exposed by the L{MethodCallProtocol}s
             instances created by this factory.
         @param methods: A list of the names of the methods that remote peers
             are allowed to call on the C{object} that we publish.
         """
-        self.object = object
+        self.object = obj
         self.methods = methods
 
     def buildProtocol(self, addr):
@@ -560,52 +560,3 @@
 
         for deferred in requests:
             deferred.callback(result)
-
-
-class RemoteObjectConnector(object):
-    """Connect to remote objects exposed by a L{MethodCallProtocol}."""
-
-    factory = MethodCallClientFactory
-    remote = RemoteObject
-
-    def __init__(self, reactor, socket_path):
-        """
-        @param reactor: A reactor able to connect to Unix sockets.
-        @param socket: The path to the socket we want to connect to.
-        @param args: Arguments to be passed to the created L{RemoteObject}.
-        @param kwargs: Keyword arguments for the created L{RemoteObject}.
-        """
-        self._socket_path = socket_path
-        self._reactor = reactor
-        self._factory = None
-        self._connector = None
-
-    def connect(self, max_retries=None, factor=None):
-        """Connect to a remote object exposed by a L{MethodCallProtocol}.
-
-        This method will connect to the socket provided in the constructor
-        and return a L{Deferred} resulting in a connected L{RemoteObject}.
-
-        @param max_retries: If not C{None} give up try to connect after this
-            amount of times, otherwise keep trying to connect forever.
-        @param factor: Optionally a float indicating by which factor the
-            delay between subsequent retries should increase. Smaller values
-            result in a faster reconnection attempts pace.
-        """
-        factory = self.factory(self._reactor)
-        factory.maxRetries = max_retries
-        if factor:
-            factory.factor = factor
-        self._connector = self._reactor.connectUNIX(self._socket_path, factory)
-        return factory.getRemoteObject()
-
-    def disconnect(self):
-        """Disconnect the L{RemoteObject} that we have created."""
-        if self._connector is not None:
-            factory = self._connector.factory
-            factory.stopTrying()
-            # XXX we should be using self._connector.disconnect() here
-            remote = factory._remote
-            if remote:
-                if remote._sender.protocol.transport:
-                    remote._sender.protocol.transport.loseConnection()

=== modified file 'landscape/lib/tests/test_amp.py'
--- landscape/lib/tests/test_amp.py	2013-05-06 18:15:21 +0000
+++ landscape/lib/tests/test_amp.py	2013-05-07 15:57:26 +0000
@@ -1,13 +1,14 @@
 from twisted.internet import reactor
 from twisted.internet.error import ConnectError, ConnectionDone
 from twisted.internet.task import Clock
-from twisted.internet.defer import Deferred, DeferredList
+from twisted.internet.defer import Deferred, inlineCallbacks
 from twisted.python.failure import Failure
+from twisted.trial.unittest import TestCase
 
 from landscape.lib.amp import (
     MethodCallError, MethodCallServerProtocol, MethodCallClientProtocol,
     MethodCallServerFactory, MethodCallClientFactory, RemoteObject,
-    RemoteObjectConnector, MethodCallSender)
+    MethodCallSender)
 from landscape.tests.helpers import LandscapeTest
 
 
@@ -178,32 +179,6 @@
            "google"]
 
 
-class WordsFactory(MethodCallClientFactory):
-
-    factor = 0.19
-
-    retryOnReconnect = True
-    retryTimeout = 0.7
-
-
-class RemoteWordsConnector(RemoteObjectConnector):
-
-    factory = WordsFactory
-
-
-METHODS = ["empty",
-           "motd",
-           "capitalize",
-           "is_short",
-           "concatenate",
-           "lower_case",
-           "multiply_alphabetically",
-           "translate",
-           "meaning_of_life",
-           "guess",
-           "google"]
-
-
 class MethodCallTest(LandscapeTest):
 
     def setUp(self):
@@ -368,7 +343,7 @@
     def setUp(self):
         super(RemoteObjectTest, self).setUp()
         self.clock = Clock()
-        self.factory = WordsFactory(self.clock)
+        self.factory = MethodCallClientFactory(self.clock)
         server_factory = MethodCallServerFactory(Words(self.clock), METHODS)
         self.connector = FakeConnector(self.factory, server_factory)
         self.connector.connect()
@@ -564,6 +539,8 @@
         factory, the L{RemoteObject} will transparently retry to perform
         the L{MethodCall} requests that failed due to the broken connections.
         """
+        self.factory.factor = 0.19
+        self.factory.retryOnReconnect = True
         self.connector.disconnect()
         deferred = self.remote.capitalize("john")
 
@@ -583,6 +560,8 @@
         the L{RemoteObject} will properly propagate the error to the original
         caller.
         """
+        self.factory.factor = 0.19
+        self.factory.retryOnReconnect = True
         self.connector.disconnect()
         deferred = self.remote.secret()
 
@@ -689,151 +668,217 @@
         self.assertIsNone(self.successResultOf(deferred))
 
 
-class RemoteObjectConnectorTest(LandscapeTest):
+class MethodCallFunctionalTest(TestCase):
 
     def setUp(self):
-        super(RemoteObjectConnectorTest, self).setUp()
+        super(MethodCallFunctionalTest, self).setUp()
         self.socket = self.mktemp()
-        self.server_factory = MethodCallServerFactory(Words(reactor), METHODS)
-        self.port = reactor.listenUNIX(self.socket, self.server_factory)
-        self.connector = RemoteWordsConnector(reactor, self.socket)
-
-        def set_remote(words):
-            self.words = words
-
-        connected = self.connector.connect()
-        return connected.addCallback(set_remote)
+        self.server = MethodCallServerFactory(Words(reactor), METHODS)
+        self.client = MethodCallClientFactory(reactor)
+        self.port = reactor.listenUNIX(self.socket, self.server)
+        #self.client.maxRetries = 0  # By default, don't try to reconnect
 
     def tearDown(self):
-        self.connector.disconnect()
+        super(MethodCallFunctionalTest, self).tearDown()
         self.port.stopListening()
-        super(RemoteObjectConnectorTest, self).tearDown()
 
+    @inlineCallbacks
     def test_connect(self):
         """
         The L{RemoteObject} resulting form the deferred returned by
-        L{RemoteObjectConnector.connect} is properly connected to the
-        remote peer.
-        """
-        return self.assertSuccess(self.words.empty())
-
+        L{MethodCallClientFactory.getRemoteObject} is properly connected
+        to the remote peer.
+        """
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+        result = yield remote.capitalize("john")
+        self.assertEqual(result, "John")
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
+    def test_connect_with_max_retries(self):
+        """
+        If L{MethodCallClientFactory.maxRetries} is set, then the factory
+        will give up trying to connect after that amout of times.
+        """
+        self.port.stopListening()
+        self.client.maxRetries = 0
+        reactor.connectUNIX(self.socket, self.client)
+        yield self.assertFailure(self.client.getRemoteObject(), ConnectError)
+
+    @inlineCallbacks
+    def test_reconnect(self):
+        """
+        If the connection is lost, the L{RemoteObject} created by the factory
+        will transparently handle the reconnection.
+        """
+        self.client.factor = 0.01  # Try reconnecting very quickly
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect and wait till we connect again
+        deferred = Deferred()
+        self.client.notifyOnConnect(deferred.callback)
+        connector.disconnect()
+        yield deferred
+
+        # The remote object is still working
+        result = yield remote.capitalize("john")
+        self.assertEqual(result, "John")
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
+    def test_retry(self):
+        """
+        If the connection is lost, the L{RemoteObject} created by the creator
+        will transparently retry to perform the L{MethodCall} requests that
+        failed due to the broken connection.
+        """
+        self.client.factor = 0.01  # Try reconnecting very quickly
+        self.client.retryOnReconnect = True
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect
+        connector.disconnect()
+
+        # This call will fail but it's transparently retried
+        result = yield remote.capitalize("john")
+        self.assertEqual(result, "John")
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
+    def test_retry_with_method_call_error(self):
+        """
+        If a retried L{MethodCall} request fails due to a L{MethodCallError},
+        the L{RemoteObject} will properly propagate the error to the original
+        caller.
+        """
+        self.client.factor = 0.01  # Try reconnecting very quickly
+        self.client.retryOnReconnect = True
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect
+        connector.disconnect()
+
+        # A method call error is not retried
+        yield self.assertFailure(remote.secret(), MethodCallError)
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
     def test_wb_retry_with_while_still_disconnected(self):
         """
         The L{RemoteObject._retry} method gets called as soon as a new
         connection is ready. If for whatever reason the connection drops
         again very quickly, the C{_retry} method will behave as expected.
         """
-        self.words._sender.protocol.transport.loseConnection()
-        self.port.stopListening()
-        real_handle_connect = self.words._handle_connect
-
-        def handle_connect(protocol):
+        self.client.factor = 0.01  # Try reconnecting very quickly
+        self.client.retryOnReconnect = True
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect
+        connector.disconnect()
+
+        def handle_reconnect(protocol):
             # In this precise moment we have a newly connected protocol
-            self.words._sender.protocol = protocol
+            remote._sender.protocol = protocol
 
             # Pretend that the connection is lost again very quickly
             protocol.transport.loseConnection()
-            self.port.stopListening()
 
             # Force RemoteObject._retry to run using a disconnected protocol
-            reactor.callLater(0, self.words._retry)
+            reactor.callLater(0, remote._retry)
 
             # Restore the real handler and start listening again very soon
-            factory.dontNotifyOnConnect(handle_connect)
-            factory.notifyOnConnect(real_handle_connect)
-            reactor.callLater(0.2, restart_listening)
-
-        def restart_listening():
-            self.port = reactor.listenUNIX(self.socket, self.server_factory)
+            self.client.dontNotifyOnConnect(handle_reconnect)
+            self.client.notifyOnConnect(remote._handle_connect)
 
         def assert_failure(error):
             self.assertEqual(str(error), "Forbidden method 'secret'")
 
         # Use our own reconnect handler
-        factory = self.connector._connector.factory
-        factory.dontNotifyOnConnect(real_handle_connect)
-        factory.notifyOnConnect(handle_connect)
-
-        reactor.callLater(0.2, restart_listening)
-        result = self.words.secret()
-        self.assertFailure(result, MethodCallError)
-        return result.addCallback(assert_failure)
-
+        self.client.dontNotifyOnConnect(remote._handle_connect)
+        self.client.notifyOnConnect(handle_reconnect)
+
+        error = yield self.assertFailure(remote.secret(), MethodCallError)
+        self.assertEqual(str(error), "Forbidden method 'secret'")
+
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
     def test_retry_with_many_method_calls(self):
         """
         If several L{MethodCall} requests were issued while disconnected, they
         will be all eventually completed when the connection gets established
         again.
         """
-        self.words._sender.protocol.transport.loseConnection()
-        self.port.stopListening()
-
-        def restart_listening():
-            self.port = reactor.listenUNIX(self.socket, self.server_factory)
-
-        def assert_guess(response):
-            self.assertEqual(response, "Guessed!")
-
-        def assert_secret(failure):
-            self.assertEqual(str(failure.value), "Forbidden method 'secret'")
-
-        def assert_motd(response):
-            self.assertEqual(response, "Words are cool")
-
-        reactor.callLater(0.1, restart_listening)
-
-        results = [self.words.guess("word", "cool", value=4),
-                   self.words.secret(),
-                   self.words.motd()]
-        results[0].addCallback(assert_guess)
-        results[1].addErrback(assert_secret)
-        results[2].addCallback(assert_motd)
-        return DeferredList(results)
-
+        self.client.factor = 0.01  # Try reconnecting very quickly
+        self.client.retryOnReconnect = True
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect
+        connector.disconnect()
+
+        result1 = yield remote.guess("word", "cool", value=4)
+        result2 = yield remote.motd()
+
+        self.assertEqual(result1, "Guessed!")
+        self.assertEqual(result2, "Words are cool")
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
     def test_retry_without_retry_on_reconnect(self):
         """
-        If C{retry_on_reconnect} is C{False}, the L{RemoteObject} object won't
+        If C{retryOnReconnect} is C{False}, the L{RemoteObject} object won't
         retry to perform requests which failed because the connection was
         lost, however requests made after a reconnection will still succeed.
         """
-        self.words._sender.protocol.transport.loseConnection()
-        self.port.stopListening()
-
-        def restart_listening():
-            self.port = reactor.listenUNIX(self.socket, self.server_factory)
-            reactor.callLater(0.3, assert_reconnected)
-
-        def assert_reconnected():
-            result = self.words.empty()
-            result.addCallback(lambda x: reconnected.callback(None))
-            return result
-
-        reactor.callLater(0.1, restart_listening)
-        self.words._factory.retryOnReconnect = False
-        result = self.words.empty()
-        self.assertFailure(result, ConnectionDone)
-        reconnected = Deferred()
-        return result.addCallback(lambda x: reconnected)
-
+        self.client.factor = 0.01  # Try reconnecting very quickly
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect
+        deferred = Deferred()
+        self.client.notifyOnConnect(deferred.callback)
+        connector.disconnect()
+
+        yield self.assertFailure(remote.modt(), ConnectionDone)
+
+        # Wait for reconnection and peform another call
+        yield deferred
+        result = yield remote.motd()
+        self.assertEqual(result, "Words are cool")
+
+        self.client.stopTrying()
+        connector.disconnect()
+
+    @inlineCallbacks
     def test_retry_with_timeout(self):
         """
-        If a C{timeout} is set, the L{RemoteObject} object will errback failed
-        L{MethodCall}s after that amount of seconds, without retrying them when
-        the connection established again.
+        If a C{retryTimeout} is set, the L{RemoteObject} object will errback
+        failed L{MethodCall}s after that amount of seconds, without retrying
+        them when the connection established again.
         """
-        self.words._sender.protocol.transport.loseConnection()
-        self.port.stopListening()
-
-        def restart_listening():
-            self.port = reactor.listenUNIX(self.socket, self.server_factory)
-            reactor.callLater(0.1, reconnected.callback, None)
-
-        def assert_failure(error):
-            self.assertEqual(str(error), "timeout")
-            return reconnected
-
-        reactor.callLater(0.9, restart_listening)
-        result = self.words.empty()
-        self.assertFailure(result, MethodCallError)
-        reconnected = Deferred()
-        return result.addCallback(assert_failure)
+        self.client.retryOnReconnect = True
+        self.client.retryTimeout = 0.1
+        self.client.factor = 1  # Reconnect slower than timeout
+        connector = reactor.connectUNIX(self.socket, self.client)
+        remote = yield self.client.getRemoteObject()
+
+        # Disconnect
+        connector.disconnect()
+
+        error = yield self.assertFailure(remote.modt(), MethodCallError)
+        self.assertEqual("timeout", str(error))
+
+        self.client.stopTrying()
+        connector.disconnect()