Merge lp:~nataliabidart/ubuntuone-client/is-running-needs-yield into lp:ubuntuone-client

[Natalia Bidart]

- Make callers of is_already_running to handle the returned deferred
  (LP: #885885).

To test, please stoip your syncdaemon and confirm that both ubuntuone-launch and u1sdtool -s start syncdaemon properly. That would be:

PYTHONPATH=. bin/u1sdtool -s
PYTHONPATH=. bin/u1sdtool -q
PYTHONPATH=. bin/ubuntuone-launch

[Natalia Bidart (nataliabidart)]

Running the tests in windows will show this failing test:

twisted.trial.unittest.FailTest: not equal:
a = ''
b = 'x\x9cKT\xc8\xc9/Q\xc8OSHI,IT(\xc9WH\xce\xcf-(J-.\x06\x00o\xb2\t\x0f'

tests.syncdaemon.test_action_queue.TestZipQueue.test_compress_gets_compressed_data

This is reported in bug #887150 and will be addressed in another branch.

[Roman Yepishev (rye)]

Worked for me.

review: Approve (fieldtest)

[John Lenton (chipaca)]

Looks good.

review: Approve

=== modified file 'bin/u1sdtool'
--- bin/u1sdtool	2011-10-21 21:01:56 +0000
+++ bin/u1sdtool	2011-11-07 15:48:24 +0000
@@ -42,7 +42,7 @@
 
 from ubuntuone.platform.tools import (
     SyncDaemonTool,
-    is_running,
+    is_already_running,
     show_dirty_nodes,
     show_downloads,
     show_error,
@@ -61,8 +61,9 @@
 from ubuntuone.clientdefs import VERSION
 
 
[e-mail]
 def main(options, args, stdout):
-    """entry point"""
+    """Entry point."""
     sync_daemon_tool = SyncDaemonTool(bus)
 
     # get the encoding of the output stream, defaults to UTF-8
@@ -71,24 +72,18 @@
         out_encoding = 'utf-8'
     out = codecs.getwriter(out_encoding)(stdout, errors='replace')
     # start syncdaemon if it's required
-    if not is_running() and not options.quit and not options.start:
-        d = sync_daemon_tool.start()
-        d.addBoth(lambda _: run(options, sync_daemon_tool, out))
-    else:
-        d = run(options, sync_daemon_tool, out)
-
-    def default_errback(error):
-        """ default error handler. """
-        out.write("\nOops, an error ocurred:\n")
-        error.printTraceback()
-
-    def stop_reactor(result):
-        """ stop the reactor. """
+    running = yield is_already_running()
+    should_start = not running and not options.quit and not options.start
+
+    try:
+        if should_start:
+            yield sync_daemon_tool.start()
+        yield run(options, sync_daemon_tool, out)
+    except Exception, e:
+        out.write("\nOops, an error ocurred:\n%s\n" % e)
+    finally:
         if reactor.running:
             reactor.stop()
-    d.addErrback(default_errback)
-    d.addCallback(stop_reactor)
-    return d
 
 
 def run(options, sync_daemon_tool, out):
@@ -171,11 +166,16 @@
         d.addCallback(lambda r: show_downloads(r, out))
     elif options.quit:
         d = sync_daemon_tool.quit()
+
+        @defer.inlineCallbacks
         def shutdown_check(result):
-            if result is None and not is_running():
+            """Shutdown and check if really stopped."""
+            running = yield is_already_running()
+            if result is None and not running:
                 out.write("ubuntuone-syncdaemon stopped.\n")
             else:
                 out.write("ubuntuone-syncdaemon still running.\n")
+
         d.addBoth(shutdown_check)
     elif options.connect:
         d = sync_daemon_tool.connect()
@@ -233,7 +233,7 @@
                       action="store_true",
                       help="Request a refresh of the list of shares to"
                       " the server")
-    parser.add_option("", "--offer-share", dest="offer_share", type="string", 
+    parser.add_option("", "--offer-share", dest="offer_share", type="string",
                       nargs=4, metavar="PATH USER SHARE_NAME ACCESS_LEVEL",
                       help="Share PATH to USER. ")
     parser.add_option("", "--list-shared", dest="list_shared",
@@ -268,8 +268,8 @@
     parser.add_option("", "--info", dest="path_info",
                       metavar="PATH", help="Request the metadata of PATH")
 
-    parser.add_option("", "--list-dirty-nodes", dest="dirty_nodes", 
-                      action="store_true", 
+    parser.add_option("", "--list-dirty-nodes", dest="dirty_nodes",
+                      action="store_true",
                       help="Show the list of nodes marked as 'dirty'")
     parser.add_option("", "--current-transfers", dest="current_transfers",
                       action="store_true",
@@ -278,25 +278,25 @@
                       help="Shutdown the syncdaemon")
     parser.add_option("-c", "--connect", dest="connect", action='store_true',
                       help="Connect the syncdaemon")
-    parser.add_option("-d", "--disconnect", dest="disconnect", 
+    parser.add_option("-d", "--disconnect", dest="disconnect",
                       action='store_true', help="Disconnect the syncdaemon")
-    parser.add_option("-s", "--status", dest="status", action='store_true', 
+    parser.add_option("-s", "--status", dest="status", action='store_true',
                       help="Get the current status of syncdaemon")
-    parser.add_option("", "--waiting", dest="waiting", 
-                      action='store_true', 
+    parser.add_option("", "--waiting", dest="waiting",
+                      action='store_true',
                       help="Get the list of operations being executed")
-    parser.add_option("", "--waiting-content", dest="waiting_content", 
-                      action='store_true', 
+    parser.add_option("", "--waiting-content", dest="waiting_content",
+                      action='store_true',
                       help="Get the waiting content list - Warning: this "
                       "option is deprecated, use '--waiting' instead")
-    parser.add_option("", "--waiting-metadata", dest="waiting_metadata", 
+    parser.add_option("", "--waiting-metadata", dest="waiting_metadata",
                       action='store_true',
                       help="Get the waiting metadata list - Warning: this "
                       "option is deprecated, use '--waiting' instead")
-    parser.add_option("", "--free-space", dest="free_space", 
+    parser.add_option("", "--free-space", dest="free_space",
                       metavar="VOLUME_ID",
                       help="Get the free space for the volume")
-    parser.add_option("", "--start", dest="start", action='store_true', 
+    parser.add_option("", "--start", dest="start", action='store_true',
                       help="Start syncdaemon if it's not running")
     parser.add_option("", "--version", dest="version", action='store_true',
                       help="Print the version number and exit")
@@ -304,4 +304,3 @@
     (options, args) = parser.parse_args(sys.argv)
     reactor.callWhenRunning(main, options, args, sys.stdout)
     reactor.run()
-

=== modified file 'bin/ubuntuone-launch'
--- bin/ubuntuone-launch	2011-03-22 15:18:50 +0000
+++ bin/ubuntuone-launch	2011-11-07 15:48:24 +0000
@@ -52,66 +52,39 @@
 import dbus
 import glib
 import gobject
+
 from dbus.mainloop.glib import DBusGMainLoop
-from ubuntuone.platform.credentials import CredentialsManagementTool
-from ubuntuone.platform.tools import SyncDaemonTool, is_running
 from twisted.internet import defer
 
-
-def stage_two(node, sync_daemon_tool):
-    """Do the last few checks and ask for a connect if all is ok."""
-    # if the node_id of the root node is None, the user has never
-    # connected. Do not connect in that case.
-    if node.get('node_id', '') is None:
-        d = defer.fail(RuntimeError("never connected?"))
-    else:
-        # one last check: avoid having sso pop up asking for creds if
-        # the user deleted them
-        creds_tools = CredentialsManagementTool()
-        d = creds_tools.find_credentials()
-
-    return d
-
-
-def wait_for_ready(_, sync_daemon_tool):
-    """Wait for syncdaemon to be READY."""
-    d = sync_daemon_tool.wait_for_signal(
-        'StatusChanged',
-        lambda a: a.get('name', '') == 'READY')
-    return d
-
-
+from ubuntuone.syncdaemon.config import get_user_config
+from ubuntuone.platform.tools import SyncDaemonTool, is_already_running
+
+
[e-mail]
 def main():
     """Start syncdaemon and ask it to connect, if possible."""
     gobject.set_application_name("ubuntuone-launch")
+
+    files_sync_enabled = get_user_config().get_files_sync_enabled()
+    if not files_sync_enabled:
+        # SD will not start (has been disabled by user)
+        raise RuntimeError("File sync is disabled")
+
     loop = DBusGMainLoop(set_as_default=True)
     bus = dbus.SessionBus(mainloop=loop)
     sync_daemon_tool = SyncDaemonTool(bus)
-
-    if sync_daemon_tool.is_files_sync_enabled():
-        if not is_running():
-            try:
-                # have SD start
-                d = sync_daemon_tool.start()
-                d.addBoth(wait_for_ready, sync_daemon_tool)
-            except dbus.exception.DBusException, e:
-                # some dbus error, shouldn't happen, bail out
-                d = defer.fail(e)
-        else:
-            # SD is already running
-            d = defer.succeed(True)
-    else:
-        # SD will not start (has been disabled by user)
-        d = defer.fail(RuntimeError("File sync is disabled"))
-
-    d.addCallback(lambda _: sync_daemon_tool.get_metadata(U1ROOT))
-    d.addCallback(stage_two, sync_daemon_tool)
+    running = yield is_already_running(bus=bus)
+    if not running:
+        # have SD start
+        yield sync_daemon_tool.start()
+        yield sync_daemon_tool.wait_for_signal('StatusChanged',
+                lambda a: a.get('name', '') == 'READY')
+
+
+if __name__ == '__main__':
+    d = main()
     # os._exit feels like it's cheating, but it's simple and fast
     d.addCallbacks(lambda _: os._exit(0), lambda _: os._exit(1))
 
     mainloop = glib.MainLoop()
     mainloop.run()
-
-
-if __name__ == '__main__':
-    main()

=== modified file 'tests/platform/test_tools.py'
--- tests/platform/test_tools.py	2011-10-28 15:30:54 +0000
+++ tests/platform/test_tools.py	2011-11-07 15:48:24 +0000
@@ -60,7 +60,8 @@
         self.patch(self.main, 'quit',
                    lambda: self.called.append('quit') or defer.succeed(None))
 
-        self.patch(tools, 'is_already_running', lambda bus: True)
+        self.patch(tools, 'is_already_running',
+                   lambda bus: defer.succeed(True))
         yield self.main.wait_for_nirvana()
 
     def _create_share(self, accepted=True, subscribed=True):
@@ -189,14 +190,16 @@
     @defer.inlineCallbacks
     def test_quit_when_running(self):
         """Test the quit method when the daemon is running."""
-        self.patch(tools, 'is_already_running', lambda bus: True)
+        self.patch(tools, 'is_already_running',
+                   lambda bus: defer.succeed(True))
         yield self.tool.quit()
         self.assertEqual(self.called, ['quit'])
 
     @defer.inlineCallbacks
     def test_quit_not_running(self):
         """Test the quit method when the daemon is not running."""
-        self.patch(tools, 'is_already_running', lambda bus: False)
+        self.patch(tools, 'is_already_running',
+                   lambda bus: defer.succeed(False))
         yield self.tool.quit()
         self.assertEqual(self.called, [])
 
@@ -497,15 +500,33 @@
     @defer.inlineCallbacks
     def test_start_when_running(self):
         """Test the start method when the daemon is running."""
-        self.patch(tools, 'is_already_running', lambda bus: True)
+        self.patch(tools, 'is_already_running',
+                   lambda bus: defer.succeed(True))
+        self.patch(self.tool, 'wait_for_signals',
+                   lambda *a: defer.fail(AssertionError(a)))
         yield self.tool.start()
         self.assertEqual(self.called, [])
 
     @defer.inlineCallbacks
     def test_start_not_running(self):
         """Test the start method when the daemon is not running."""
-        self.patch(tools, 'is_already_running', lambda bus: False)
+        d = defer.Deferred()
+
+        def succed_status_changed(signal_name, *a):
+            """Returned a fired deferred if signal_name is StatusChanged."""
+            if signal_name == 'StatusChanged':
+                return d.callback(True)
+            else:
+                return d.errback(AssertionError(signal_name))
+
+        self.patch(tools, 'is_already_running',
+                   lambda bus: defer.succeed(False))
+        self.patch(self.tool, 'wait_for_signals', succed_status_changed)
+        assert not d.called
+
         yield self.tool.start()
+        yield d  # d is fired when wait_for_signals on StatusChanged was called
+
         self.assertEqual(self.called, ['start'])
 
     @defer.inlineCallbacks
@@ -710,7 +731,10 @@
     @defer.inlineCallbacks
     def test_enable_files_sync_when_disabled(self):
         """Test for enable_files_sync."""
-        self.patch(tools, 'is_already_running', lambda bus: False)
+        self.patch(self.tool, 'start',
+                   lambda: self.called.append('start') or defer.succeed(None))
+        self.patch(tools, 'is_already_running',
+                   lambda bus: defer.succeed(False))
         # be sure file sync is disabled and clear called
         yield self.tool.enable_files_sync(False)
         self.called = []

=== modified file 'ubuntuone/platform/tools/__init__.py'
--- ubuntuone/platform/tools/__init__.py	2011-11-02 18:14:42 +0000
+++ ubuntuone/platform/tools/__init__.py	2011-11-07 15:48:24 +0000
@@ -474,14 +474,15 @@
         (file_info,) = yield d
         defer.returnValue(file_info)
 
+    @defer.inlineCallbacks
     @log_call(logger.debug)
     def quit(self):
         """Quit the syncdaemon."""
-        if is_already_running(bus=self.bus):
-            result = self.proxy.call_method('sync_daemon', 'quit')
-        else:
-            result = defer.succeed(None)
-        return result
+        result = None
+        running = yield is_already_running(bus=self.bus)
+        if running:
+            result = yield self.proxy.call_method('sync_daemon', 'quit')
+        defer.returnValue(result)
 
     @log_call(logger.debug)
     def connect(self):
@@ -524,14 +525,15 @@
         """Return the waiting content queue elements."""
         return self.proxy.call_method('status', 'waiting_content')
 
+    @defer.inlineCallbacks
     @log_call(logger.debug)
     def start(self):
         """Start syncdaemon if it's not running."""
-        if is_already_running(bus=self.bus):
-            result = defer.succeed(None)
-        else:
-            result = self.proxy.start()
-        return result
+        running = yield is_already_running(bus=self.bus)
+        if not running:
+            wait_d = self.wait_for_signals('StatusChanged')
+            yield self.proxy.start()
+            yield wait_d
 
     @log_call(logger.debug)
     def get_throttling_limits(self):
@@ -664,7 +666,7 @@
 # callbacks used by u1sdtool script
 
 def show_shared(shares, out):
-    """Callback that prints the list of shared shares."""
+    """Print the list of shared shares."""
     if len(shares) == 0:
         out.write("No shared\n")
     else:
@@ -680,7 +682,7 @@
 
 
 def show_folders(folders, out):
-    """Callback that prints the list of user defined folders."""
+    """Print the list of user defined folders."""
     if len(folders) == 0:
         out.write("No folders\n")
     else:
@@ -704,7 +706,7 @@
 
 
 def show_shares(shares, out):
-    """Callback that prints the list of shares."""
+    """Print the list of shares."""
     if len(shares) == 0:
         out.write("No shares\n")
     else:

=== modified file 'ubuntuone/platform/tools/linux.py'
--- ubuntuone/platform/tools/linux.py	2011-10-21 17:06:24 +0000
+++ ubuntuone/platform/tools/linux.py	2011-11-07 15:48:24 +0000
@@ -155,13 +155,8 @@
                             follow_name_owner_changes=True)
 
     def start(self):
-        """Start syncdaemon if it's not running."""
-        wait_d = self.wait_for_signal('StatusChanged', lambda x: x)
-        bus_client = DBusClient(self.bus, dbus.bus.BUS_DAEMON_PATH,
-                                dbus.bus.BUS_DAEMON_IFACE,
-                                dbus.bus.BUS_DAEMON_NAME)
-        d = bus_client.call_method('StartServiceByName',
-                                   DBUS_IFACE_NAME, 0,
-                                   signature='su')
-        d.addCallback(lambda _: wait_d)
-        return d
+        """Start syncdaemon, should *not* be running."""
+        if DBUS_IFACE_NAME in self.bus.list_names():
+            self.bus.release_name(DBUS_IFACE_NAME)
+        _, result = self.bus.start_service_by_name(DBUS_IFACE_NAME, 0)
+        return defer.succeed(result == dbus.bus.DBUS_START_REPLY_SUCCESS)

=== modified file 'ubuntuone/platform/tools/windows.py'
--- ubuntuone/platform/tools/windows.py	2011-11-02 19:28:33 +0000
+++ ubuntuone/platform/tools/windows.py	2011-11-07 15:48:24 +0000
@@ -149,7 +149,7 @@
         return self.connected
 
     def start(self):
-        """Start syncdaemon if it's not running."""
+        """Start syncdaemon, should *not* be running."""
         # look in the reg to find the path of the .exe to be executed
         # to launch the sd on windows
         key = OpenKey(HKEY_LOCAL_MACHINE, U1_REG_PATH)