Merge lp:~mvo/ubuntu-sso-client/lp711413 into lp:ubuntu-sso-client

[Michael Vogt]

This is one way of hopefully solving bug #711413 that shows up on errors.ubuntu.com currntly (via its duplicate #926678).

The idea is that if its a race on logout when the dbus session daemon gets killed before the syncdaemon, we can simply retry to access the dbus daemon until ubuntu-sso-client is also killed by the session logout (this branch assumes that this is the case). Its not entirely clear if it is - but the patch should not make anything worse, the worst outcome would be that it just dosn't fix anything in which case it should be reverted and the approach needs to be rethought.

[dobey (dobey)]

Please don't add a new dependency on python-mock here. Instead, you should use BaseTestCase from ubuntu_sso.main.tests.linux and use self.patch() inside a test to patch a method, rather than using the @patch decorator from mock. You can see some usage of self.patch() in test_clients.py in the same directory.

review: Needs Fixing

[Michael Vogt (mvo)]

On Wed, May 23, 2012 at 04:54:22PM -0000, Rodney Dawes wrote:
> Review: Needs Fixing
>
> Please don't add a new dependency on python-mock here. Instead, you should use BaseTestCase from ubuntu_sso.main.tests.linux and use self.patch() inside a test to patch a method, rather than using the @patch decorator from mock. You can see some usage of self.patch() in test_clients.py in the same directory.

Thanks, I agree with your point! Unfortunately I didn't had time to
update it yet and the recent feedback I got from seb128 about session
shutdown makes me uncertain that the fix is actually sufficient. I
wonder if we could do a experiment and push it to -proposed as it is
(should do no harm, famous last words ;) and see if the new version
number appears on errors.ubuntu.com, if so, the patch is not
sufficient and the whole branch can die. If its not showing up we have
some indication that its the right approach and that more work on it
is justified (and I will be happy to do it). What do you think?

[dobey (dobey)]

I don't think that is a sufficient test, and I don't like the idea of using -proposed as a means of discovering whether or not a bug is fixed by a change. What happens if it is uploaded, and nobody installs it, because all the people hitting the bug, do not have -proposed enabled? It won't appear, but doesn't mean this is the fix or the right approach to fixing it, either.

Is there a better way to have people test this, who are actually hitting the bug? Is there a reasonable way to duplicate the issue directly in a simple unit test case?

[dobey (dobey)]

Rejecting this as we have landed a better workaround for this issue in trunk already.

review: Disapprove

Unmerged revisions

958. By Michael Vogt on 2012-05-23

try to fix #711413 via a retry approach if the session bus is not available, the idea is that ubuntu-sso-client gets killed on logout hopefully before the retry-timeout is reached

=== modified file 'ubuntu_sso/main/linux.py'
--- ubuntu_sso/main/linux.py	2012-04-10 10:43:48 +0000
+++ ubuntu_sso/main/linux.py	2012-05-23 15:49:21 +0000
@@ -39,6 +39,7 @@
 """
 
 import signal
+import time
 
 import dbus
 import dbus.service
@@ -340,12 +341,31 @@
 class UbuntuSSOProxy(object):
     """Object that exposes the diff referenceable objects."""
 
+    # the time and retry count to wait for the session bus to become
+    # available before the system errors out
+    DBUS_RETRY_WAIT_TIME = 0.2
+    DBUS_RETRY_COUNT_MAX = 10
+
     def __init__(self, root):
         self.root = root
-        self.bus = dbus.SessionBus()
+        self.bus = self._get_session_bus_with_retry()
         self.sso_login = None
         self.cred_manager = None
 
+    def _get_session_bus_with_retry(self):
+        # keep retrying to avoid race on login/logout, see LP: #711413
+        for i in range(self.DBUS_RETRY_COUNT_MAX):
+            try:
+                bus = dbus.SessionBus()
+                break
+            except dbus.DBusException:
+                logger.exception("Failed to get the session dbus""")
+                time.sleep(self.DBUS_RETRY_WAIT_TIME)
+        else:
+            # if we reach this point, we couldn't get a bus, so give up
+            raise
+        return bus
+
     def start(self):
         """Start listening, nothing async to be done in this platform."""
         # Register DBus service for making sure we run only one instance

=== added file 'ubuntu_sso/main/tests/test_linux.py'
--- ubuntu_sso/main/tests/test_linux.py	1970-01-01 00:00:00 +0000
+++ ubuntu_sso/main/tests/test_linux.py	2012-05-23 15:49:21 +0000
@@ -0,0 +1,77 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# In addition, as a special exception, the copyright holders give
+# permission to link the code of portions of this program with the
+# OpenSSL library under certain conditions as described in each
+# individual source file, and distribute linked combinations
+# including the two.
+# You must obey the GNU General Public License in all respects
+# for all of the code used other than OpenSSL.  If you modify
+# file(s) with this exception, you may extend this exception to your
+# version of the file(s), but you are not obligated to do so.  If you
+# do not wish to do so, delete this exception statement from your
+# version.  If you delete this exception statement from all source
+# files in the program, then also delete it here.
+"""Linux specific tests for the main module."""
+
+import time
+
+from mock import patch
+
+import dbus
+
+from ubuntu_sso.tests import TestCase
+from ubuntu_sso.main import UbuntuSSOProxy
+
+# because we are using twisted we have java like names C0103
+# pylint: disable=C0103
+
+
+class UbuntuSSOProxyTestCase(TestCase):
+    """Tests for the UbuntuSSOProxy """
+
+
+    def test_retry_on_race_lp711413(self):
+        """Test the retry on dbus failure feature to fix bug #711413 """
+        def _simulate_dbus_unavailable_once():
+            """ helper to simulate a dbus not-yet-ready error once """
+            patcher.stop()
+            raise dbus.DBusException()
+        # simulate that the session bus is unavailable initially
+        patcher = patch("dbus.SessionBus")
+        session_bus_mock = patcher.start()
+        session_bus_mock.side_effect = _simulate_dbus_unavailable_once
+        # get proxy and measure the time to access the bus
+        now = time.time()
+        proxy = UbuntuSSOProxy(None)
+        bus = proxy.bus
+        delay = time.time() - now
+        # verify that we have a bus and that it took the expected time
+        self.assertNotEqual(bus, None)
+        self.assertTrue(delay > 1*proxy.DBUS_RETRY_WAIT_TIME and
+                        delay < 2*proxy.DBUS_RETRY_WAIT_TIME)
+
+    
+    @patch("time.sleep")
+    @patch("dbus.SessionBus")
+    def test_raise_on_timeout(self, mock_session_bus, mock_sleep):
+        """Test that  the normal dbus.DBusException is raised on timeout """
+        def _raise_dbus_exception():
+            raise dbus.DBusException()
+        mock_session_bus.side_effect = _raise_dbus_exception
+        self.assertRaises(dbus.DBusException, UbuntuSSOProxy, None)
+