Merge lp:~diegosarmentero/ubuntuone-client/syncdaemon-q into lp:ubuntuone-client

[Diego Sarmentero]

- Avoid creating SyncDaemonTool if is not already running and the user wants to close it. (LP: #907479).

[dobey (dobey)]

Do we really need to print the "not running" message here? I think just exiting cleanly would be fine, and it would change this block of code to just be "if running and options.quit:" which I think is better.

review: Needs Information

[Diego Sarmentero (diegosarmentero)]

We can't change the if to: "if running and options.quit:" because you will be leaving out a lot of options.
I can avoid printing the message and just exit if you think it's best, but i added the message to show some kind of feedback, so the user actually knows that something was executed.

[dobey (dobey)]

Approving, but we should probably fix that code to be more modular, so that all the options actually get handled separately in parsing. Right now, the logic is hard to separate when reading the diff/code.

review: Approve

[Natalia Bidart (nataliabidart)]

The creation of the sync_daemon_tool instance should not start the service, only the call to .start() should do it.

The changes applied to this branch are not incorrect, but I'm not comfortable with landing this, since we will be masquerading the real issue which is that the simple fact of creating an instance of SyncDaemonTool starts the service in windows (and it shouldn't).

review: Needs Fixing

[Diego Sarmentero (diegosarmentero)]

> The creation of the sync_daemon_tool instance should not start the service,
> only the call to .start() should do it.
>
> The changes applied to this branch are not incorrect, but I'm not comfortable
> with landing this, since we will be masquerading the real issue which is that
> the simple fact of creating an instance of SyncDaemonTool starts the service
> in windows (and it shouldn't).

The problem seems to be that in ubuntu_sso tcpactivation to get the active port, if sd is not running self._spawn_server() is called, and this opens sd but never close it.

    def _spawn_server(self):
        """Start running the server process."""
        # Without using close_fds=True, strange things happen
        # with logging on windows. More information at
        # http://bugs.python.org/issue4749
        with open(r"c:\Users\gatox\Desktop\output.txt", 'w') as f:
            f.write(repr(self.config.command_line))
        subprocess.Popen(self.config.command_line, close_fds=True)

    @defer.inlineCallbacks
    def _do_get_active_port(self):
        """Get the port for the running instance, starting it if needed."""
        is_running = yield self.is_already_running()
        if not is_running:
            self._spawn_server()
            yield self._wait_server_active()
        defer.returnValue(self.config.port)

I assume that this code (starting sd just to ask the port if running) is here for another reason, because it doesn't make sense with this issue, we wouldn't need to start sd to ask for the port, if we want to close it.

So, i'm not sure if the proper fix is the one already proposed, or we should close sd after asking for the port (which it doesn't have much sense to me).

[Diego Sarmentero (diegosarmentero)]

http://paste.ubuntu.com/921687/

These are the series of call that end up openning syncdaemon in the "_spawn_server" method.

----------------------------------------------------------
(u1sdtool)

@defer.inlineCallbacks
def main(options, args, stdout):
    """Entry point."""
    sync_daemon_tool = SyncDaemonTool(bus)

----------------------------------------------------------
(ubuntuone-client/ubuntuone/platform/tools/__init__.py)

class SyncDaemonTool(object):
    """Various utility methods to test/play with the SyncDaemon."""

    def __init__(self, bus=None):
        self.bus = bus
        self.last_event = 0
        self.delayed_call = None
        self.log = logger
        self.proxy = source.SyncDaemonToolProxy(bus=bus)

----------------------------------------------------------
(ubuntuone-client/ubuntuone/platform/tools/windows.py)

class SyncDaemonToolProxy(object):
...
    def __init__(self, bus=None):
        self.client = UbuntuOneClient()
        self.connected = self.client.connect()

----------------------------------------------------------
(ubuntuone-client/ubuntuone/platform/windows/ipc_client.py)

class UbuntuOneClient(object):
    """Root object that provides access to all the remote objects."""

    def __init__(self):
        """Create a new instance."""
        ...

    @defer.inlineCallbacks
    def connect(self):
        """Connect to the syncdaemon service."""
        # pylint: disable=W0702
        try:
            # connect to the remote objects
            self.factory = PBClientFactory()
            self.client = yield ipc_client_connect(self.factory)
            ...

----------------------------------------------------------
(ubuntuone-client/ubuntuone/platform/windows/ipc.py)

@defer.inlineCallbacks
def ipc_client_connect(client_factory):
    """Connect the IPC client factory."""
    ac = ActivationClient(get_activation_config())
    port = yield ac.get_active_port()

----------------------------------------------------------
(ubuntu-sso-client/ubuntu_sso/utils/tcpactivation.py)

class ActivationClient(ActivationDetector):
    """A client for tcp activation."""

    # a classwide lock, so the server is started only once
    lock = defer.DeferredLock()

    @defer.inlineCallbacks
    def _wait_server_active(self):
        """Wait till the server is active."""
        for _ in xrange(NUMBER_OF_CHECKS):
            is_running = yield self.is_already_running()
            if is_running:
                defer.returnValue(None)
            yield async_sleep(DELAY_BETWEEN_CHECKS)
        raise ActivationTimeoutError()

    def _spawn_server(self):
        """Start running the server process."""
        # Without using close_fds=True, strange things happen
        # with logging on windows. More information at
        # http://bugs.python.org/issue4749
        subprocess.Popen(self.config.command_line, close_fds=True)

    @defer.inlineCallbacks
    def _do_get_active_port(self):
        """Get the port for the running instance, starting it if needed."""
        is_running = yield self.is_already_running()
        if not is_running:
            self._spawn_server()
            yield self._wait_server_active()
        de...

[Diego Sarmentero (diegosarmentero)]

> The creation of the sync_daemon_tool instance should not start the service,
> only the call to .start() should do it.
>
> The changes applied to this branch are not incorrect, but I'm not comfortable
> with landing this, since we will be masquerading the real issue which is that
> the simple fact of creating an instance of SyncDaemonTool starts the service
> in windows (and it shouldn't).

Done!

[Alejandro J. Cura (alecu)]

self.wait_connected() returns a deferred that is callbacked when the connection is done, so this branch won't work as expected.

review: Needs Fixing

[Diego Sarmentero (diegosarmentero)]

> self.wait_connected() returns a deferred that is callbacked when the
> connection is done, so this branch won't work as expected.

Fixed.
The call to wait_connection(), was wrong, and unnecessary, because inside of the proper function call "call_after_connection_inner", we are already doing: yield self.connected

[Alejandro J. Cura (alecu)]

Looks great now.

review: Approve

[Alejandro J. Cura (alecu)]

After some chat on IRC with nessita, it became clear that these areas of the branch need improvement:

<nessita> alecu: so, I'm not that happy with the definition of record_calls, since it also dismissed the arguments passed to the patched functions
<alecu> nessita, the point about record_calls is very valid: we should also be checking the arguments for those calls.
<nessita> alecu: also, I would advice using functools.partial for the pacth, and not defining a new record_calls

review: Needs Fixing

[Manuel de la Peña (mandel)]

+1

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0, Needs Fixing == 0, Needs Information == 0, Resubmit == 0, Pending == 0. Got: 2 Approve, 2 Needs Fixing.

[Alejandro J. Cura (alecu)]

Looks good now.

review: Approve

[Natalia Bidart (nataliabidart)]

review: Abstain

=== modified file 'tests/platform/windows/test_tools.py'
--- tests/platform/windows/test_tools.py	2012-04-09 20:07:05 +0000
+++ tests/platform/windows/test_tools.py	2012-04-16 12:24:17 +0000
@@ -29,6 +29,7 @@
 """Tests for some tools for talking to the syncdaemon."""
 
 import sys
+import operator
 
 from twisted.internet import defer
 from twisted.trial.unittest import TestCase
@@ -50,6 +51,7 @@
         yield super(TestSyncDaemonTool, self).setUp()
         self.patch(windows.UbuntuOneClient, "connect", lambda _: defer.Deferred())
         self.sdtool = windows.SyncDaemonToolProxy()
+        self.calls = {}
 
     def test_call_after_connection(self):
         """Test the _call_after_connection method."""
@@ -61,6 +63,31 @@
         self.sdtool.connected.callback("got connected!")
         self.assertEqual(len(collected_calls), 1)
 
+    def test_call_after_connection_connect(self):
+        """Test execute connect in _call_after_connection method."""
+        self.patch(self.sdtool.client, "is_connected", lambda: False)
+        my_connect = lambda *args, **kwargs: operator.setitem(
+                self.calls, "connect", (args, kwargs))
+        self.patch(self.sdtool.client, "connect", my_connect)
+        collected_calls = []
+        test_method = lambda *args: collected_calls.append(args)
+        test_method = self.sdtool._call_after_connection(test_method)
+        test_method(123)
+        self.assertIn("connect", self.calls)
+        self.assertEqual(self.calls["connect"], ((), {}))
+
+    def test_call_after_connection_not_connect(self):
+        """Test execute connect in _call_after_connection method."""
+        self.patch(self.sdtool.client, "is_connected", lambda: True)
+        my_connect = lambda *args, **kwargs: operator.setitem(
+                self.calls, "connect", (args, kwargs))
+        self.patch(self.sdtool.client, "connect", my_connect)
+        collected_calls = []
+        test_method = lambda *args: collected_calls.append(args)
+        test_method = self.sdtool._call_after_connection(test_method)
+        test_method(123)
+        self.assertNotIn("connect", self.calls)
+
     def test_should_wrap(self):
         """Only some attributes should be wrapped."""
         test_function = "sample_function"

=== modified file 'tests/syncdaemon/test_action_queue.py'
--- tests/syncdaemon/test_action_queue.py	2012-04-09 20:07:05 +0000
+++ tests/syncdaemon/test_action_queue.py	2012-04-16 12:24:17 +0000
@@ -1288,6 +1288,7 @@
         orig = self.action_queue.clientConnectionLost
 
         d = defer.Deferred()
+
         def faked_connectionLost(*args, **kwargs):
             """Receive connection lost and check."""
             orig(*args, **kwargs)

=== modified file 'ubuntuone/platform/tools/windows.py'
--- ubuntuone/platform/tools/windows.py	2012-04-09 20:07:05 +0000
+++ ubuntuone/platform/tools/windows.py	2012-04-16 12:24:17 +0000
@@ -126,10 +126,12 @@
 
     def __init__(self, bus=None):
         self.client = UbuntuOneClient()
-        self.connected = self.client.connect()
+        self.connected = None
 
     def _call_after_connection(self, method):
         """Make sure Perspective Broker is connected before calling."""
+        if not self.client.is_connected():
+            self.connected = self.client.connect()
 
         @defer.inlineCallbacks
         def call_after_connection_inner(*args, **kwargs):

=== modified file 'ubuntuone/platform/windows/ipc_client.py'
--- ubuntuone/platform/windows/ipc_client.py	2012-04-09 20:07:05 +0000
+++ ubuntuone/platform/windows/ipc_client.py	2012-04-16 12:24:17 +0000
@@ -759,6 +759,10 @@
                             'Could not connect to the syncdaemon ipc.', e)
         # pylint: disable=W0702
 
+    def is_connected(self):
+        """Return if the client is connected."""
+        return (self.client is not None)
+
     @defer.inlineCallbacks
     def register_to_signals(self):
         """Register the different clients to the signals."""