Merge lp:~mandel/ubuntuone-client/stale-broker into lp:ubuntuone-client

[Manuel de la Peña]

- Fix stale broker problems by reconnecting to the server and requesting new valid instances for the reference objects in the client side (LP: #1040915).

- Fix stale broker problems by reconnecting to the server and requesting new valid instances for the reference objects in the client side (LP: #1040915).

[Mike McCracken (mikemc)]

As discussed on IRC, I'd like to understand a bit more about why we're disconnecting and getting invalid refs before we just auto-reconnect on failures.

The bug seems to come up when switching between tabs, and we might (didn't confirm) have one PB connection per tab to the syncdaemon - is something locally getting GC'd when a tab isn't visible?
If so, why does that cause a remote ref to be invalid? I haven't seen any evidence that syncdaemon itself is dying.

review: Needs Information

[Roberto Alsina (ralsina)]

Works ok for me. While we do want to know why we are reconnecting, reconnection in itself is useful (for example, because of sd crashes)

review: Approve

[Mike McCracken (mikemc)]

OK, the branch works for me, so I'll approve.

Filed bug #1042789 to look into the root cause

review: Approve

=== modified file 'tests/platform/tools/test_tools.py'
--- tests/platform/tools/test_tools.py	2012-05-24 15:09:34 +0000
+++ tests/platform/tools/test_tools.py	2012-08-27 16:04:01 +0000
@@ -32,6 +32,7 @@
 import operator
 
 from twisted.internet import defer
+from twisted.spread import pb
 from twisted.trial.unittest import TestCase
 
 from ubuntuone.platform.tools import perspective_broker
@@ -125,3 +126,66 @@
                 attr = getattr(self.sdtool, attr_name)
                 func_name = getattr(attr, "__name__", None)
                 self.assertNotEqual(func_name, "call_after_connection_inner")
+
+
+class FakeRemoteObject(object):
+    """Fake a remote object."""
+
+    def __init__(self):
+        """Create a new instance."""
+        self.number_calls = 0
+        self.called = []
+
+    def method_call(self, *args, **kwargs):
+        """Fake a remote method call."""
+        if self.number_calls == 0:
+           self.number_calls += 1
+           raise pb.DeadReferenceError()
+        else:
+           self.called.append((args, kwargs))
+           return defer.succeed(self.number_calls)
+
+
+class PerspectiveBrokerReconnect(TestCase):
+    """Test when the ipc is reconnected."""
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        """Set the tests."""
+        yield super(PerspectiveBrokerReconnect, self).setUp()
+        self.sdtool = perspective_broker.SyncDaemonToolProxy()
+        self.sdtool.client.fake_remote = FakeRemoteObject()
+        self.connected_signals = []
+        self.reconnected = False
+
+        def connect_signal(my_self, *args, **kwargs):
+            """Fake connect_signal call."""
+            self.connected_signals.append(('connect_signal', args, kwargs))
+
+        self.patch(perspective_broker.SyncDaemonToolProxy, 'connect_signal',
+                connect_signal)
+
+        def fake_reconnect(_):
+            """Fake the reconnection of the client."""
+            self.reconnected = True
+
+        self.patch(perspective_broker.UbuntuOneClient, 'reconnect',
+                fake_reconnect)
+        self.patch(perspective_broker.UbuntuOneClient, 'connect',
+                lambda _: defer.succeed(True))
+
+    @defer.inlineCallbacks
+    def test_reconnect_no_signals(self):
+        """Test reconnection with no signals."""
+        yield self.sdtool.call_method('fake_remote', 'method_call')
+        self.assertTrue(self.reconnected)
+        self.assertEqual(0, len(self.connected_signals))
+
+    @defer.inlineCallbacks
+    def test_reconnect_signals(self):
+        """Test reconnection with signals."""
+        self.sdtool.connected_signals = dict(create_signal=lambda:None,
+                                             delete_signal=lambda:None)
+        yield self.sdtool.call_method('fake_remote', 'method_call')
+        self.assertTrue(self.reconnected)
+        self.assertEqual(2, len(self.connected_signals))

=== modified file 'ubuntuone/platform/ipc/ipc_client.py'
--- ubuntuone/platform/ipc/ipc_client.py	2012-08-16 12:00:12 +0000
+++ ubuntuone/platform/ipc/ipc_client.py	2012-08-27 16:04:01 +0000
@@ -778,6 +778,18 @@
                             'Could not connect to the syncdaemon ipc.', e)
         # pylint: disable=W0702
 
+    @defer.inlineCallbacks
+    def reconnect(self):
+        """Reconnect and get the new remote objects."""
+        try:
+            root = yield self.factory.getRootObject()
+            yield self._request_remote_objects(root)
+            yield self.register_to_signals()
+            defer.returnValue(self)
+        except Exception, e:
+            raise SyncDaemonClientConnectionError(
+                            'Could not reconnect to the syncdaemon ipc.', e)
+
     def is_connected(self):
         """Return if the client is connected."""
         return (self.client is not None)

=== modified file 'ubuntuone/platform/tools/perspective_broker.py'
--- ubuntuone/platform/tools/perspective_broker.py	2012-07-13 16:06:27 +0000
+++ ubuntuone/platform/tools/perspective_broker.py	2012-08-27 16:04:01 +0000
@@ -28,9 +28,11 @@
 # files in the program, then also delete it here.
 """SyncDaemon Tools."""
 
+import logging
 import subprocess
 
 from twisted.internet import defer
+from twisted.spread.pb import DeadReferenceError
 
 from ubuntuone.platform.ipc.perspective_broker import is_already_running
 from ubuntuone.platform.ipc.ipc_client import UbuntuOneClient
@@ -99,6 +101,7 @@
     _DONT_VERIFY_CONNECTED = [
         "wait_connected",
         "client", "last_event", "delayed_call", "log", "connected",
+        "connected_signals"
     ]
 
     def _should_wrap(self, attr_name):
@@ -115,8 +118,11 @@
             return attr
 
     def __init__(self, bus=None):
+        self.log = logging.getLogger('ubuntuone.platform.tools.' +
+            'perspective_broker')
         self.client = UbuntuOneClient()
         self.connected = None
+        self.connected_signals = {}
 
     def _call_after_connection(self, method):
         """Make sure Perspective Broker is connected before calling."""
@@ -127,17 +133,41 @@
         def call_after_connection_inner(*args, **kwargs):
             """Call the given method after the connection to pb is made."""
             yield self.connected
-            retval = yield method(*args, **kwargs)
+            try:
+                retval = yield method(*args, **kwargs)
+            except DeadReferenceError:
+                self.log.debug('Got stale broker, atempting reconnect.')
+                # might be the case where we have a stale broker
+                yield self._reconnect_client()
+                retval = yield method(*args, **kwargs)
             defer.returnValue(retval)
 
         return call_after_connection_inner
 
+    @defer.inlineCallbacks
+    def _reconnect_client(self):
+        """Reconnect the client."""
+        self.connected = False
+        yield self.client.reconnect()
+        # do connect all the signals again
+        for signal_name, handler in self.connected_signals.items():
+            self.connect_signal(signal_name, handler)
+
+    @defer.inlineCallbacks
     def call_method(self, client_kind, method_name, *args, **kwargs):
         """Call the 'method_name' passing 'args' and 'kwargs'."""
         client = getattr(self.client, client_kind)
         method = getattr(client, method_name)
-        result = method(*args, **kwargs)
-        return result
+        try:
+            result = yield method(*args, **kwargs)
+        except DeadReferenceError:
+            self.log.debug('Got stale broker, atempting reconnect.')
+            # may happen in the case we reconnected and the server side objects
+            # for gc
+            yield self._reconnect_client()
+            result = yield self.call_method(client_kind, method_name,
+                    *args, **kwargs)
+        defer.returnValue(result)
 
     def shutdown(self):
         """Close connections."""
@@ -148,6 +178,8 @@
         client_kind, callback = self._SIGNAL_MAPPING[signal_name]
         client = getattr(self.client, client_kind)
         setattr(client, callback, handler)
+        # do remember the connected signal in case we need to reconnect
+        self.connected_signals[signal_name] = handler
         return handler
 
     def disconnect_signal(self, signal_name, handler_or_match):
@@ -155,6 +187,8 @@
         client_kind, callback = self._SIGNAL_MAPPING[signal_name]
         client = getattr(self.client, client_kind)
         setattr(client, callback, None)
+        # forget that the connection was made in case we need to reconnect
+        del self.connected_signals[signal_name]
         return handler_or_match
 
     def wait_connected(self):