Merge lp:~robru/friends/keepalive into lp:~super-friends/friends/raring

[Robert Bruce Park]

Keep the Dispatcher alive for 30s beyond the return of the final method invocation.

[Ken VanDine (ken-vandine)]

review: Approve

=== modified file 'friends/main.py'
--- friends/main.py	2013-03-14 19:44:13 +0000
+++ friends/main.py	2013-03-20 01:26:22 +0000
@@ -62,11 +62,9 @@
 GObject.threads_init(None)
 
 from friends.service.dispatcher import Dispatcher, DBUS_INTERFACE
-from friends.utils.base import _OperationThread, _publish_lock
-from friends.utils.base import Base, initialize_caches
+from friends.utils.base import Base, initialize_caches, _publish_lock
 from friends.utils.model import Model, prune_model
 from friends.utils.logging import initialize
-from friends.utils.avatar import Avatar
 
 
 # Optional performance profiling module.
@@ -89,10 +87,6 @@
             print(class_name)
         return
 
-    # Our threading implementation needs to know how to quit the
-    # application once all threads have completed.
-    _OperationThread.shutdown = loop.quit
-
     # Disallow multiple instances of friends-dispatcher
     bus = dbus.SessionBus()
     obj = bus.get_object('org.freedesktop.DBus', '/org/freedesktop/DBus')

=== modified file 'friends/protocols/twitter.py'
--- friends/protocols/twitter.py	2013-03-19 03:11:28 +0000
+++ friends/protocols/twitter.py	2013-03-20 01:26:22 +0000
@@ -26,7 +26,6 @@
 import logging
 
 from urllib.parse import quote
-from gi.repository import GLib
 
 from friends.utils.avatar import Avatar
 from friends.utils.base import Base, feature

=== modified file 'friends/service/dispatcher.py'
--- friends/service/dispatcher.py	2013-03-14 13:29:53 +0000
+++ friends/service/dispatcher.py	2013-03-20 01:26:22 +0000
@@ -28,12 +28,13 @@
 import dbus.service
 
 from gi.repository import GLib
+from contextlib import ContextDecorator
 
 from friends.utils.avatar import Avatar
 from friends.utils.account import AccountManager
 from friends.utils.manager import protocol_manager
 from friends.utils.menus import MenuManager
-from friends.utils.model import Model
+from friends.utils.model import Model, persist_model
 from friends.shorteners import lookup
 
 
@@ -43,6 +44,48 @@
 STUB = lambda *ignore, **kwignore: None
 
 
+# Avoid race condition during shut-down
+_exit_lock = threading.Lock()
+
+
+class ManageTimers(ContextDecorator):
+    """Exit the dispatcher 30s after the most recent method call returns."""
+    timers = set()
+    callback = STUB
+
+    def __enter__(self):
+        self.clear_all_timers()
+
+    def __exit__(self, *ignore):
+        self.set_new_timer()
+
+    def clear_all_timers(self):
+        log.debug('Clearing {} shutdown timer(s)...'.format(len(self.timers)))
+        while self.timers:
+            GLib.source_remove(self.timers.pop())
+
+    def set_new_timer(self):
+        # Concurrency will cause two methods to exit near each other,
+        # causing two timers to be set, so we have to clear them again.
+        self.clear_all_timers()
+        log.debug('Starting new shutdown timer...')
+        self.timers.add(GLib.timeout_add_seconds(30, self.terminate))
+
+    def terminate(self, *ignore):
+        """Exit the dispatcher, but only if there are no active subthreads."""
+        with _exit_lock:
+            if threading.activeCount() < 2:
+                log.debug('No threads found, shutting down.')
+                persist_model()
+                self.timers.add(GLib.idle_add(self.callback))
+            else:
+                log.debug('Delaying shutdown because active threads found.')
+                self.set_new_timer()
+
+
+exit_after_idle = ManageTimers()
+
+
 class Dispatcher(dbus.service.Object):
     """This is the primary handler of dbus method calls."""
     __dbus_object_path__ = '/com/canonical/friends/Dispatcher'
@@ -59,10 +102,13 @@
         self.menu_manager = MenuManager(self.Refresh, self.mainloop.quit)
         Model.connect('row-added', self._increment_unread_count)
 
+        ManageTimers.callback = mainloop.quit
+
     def _increment_unread_count(self, model, itr):
         self._unread_count += 1
         self.menu_manager.update_unread_count(self._unread_count)
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE)
     def Refresh(self):
         """Download new messages from each connected protocol."""
@@ -80,6 +126,7 @@
                 # If a protocol doesn't support receive then ignore it.
                 pass
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE)
     def ClearIndicators(self):
         """Indicate that messages have been read.
@@ -92,8 +139,8 @@
             service.ClearIndicators()
         """
         self.menu_manager.update_unread_count(0)
-        GLib.idle_add(self.mainloop.quit)
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE,
                          in_signature='sss',
                          out_signature='s',
@@ -138,6 +185,7 @@
         if not called:
             failure('No accounts supporting {} found.'.format(action))
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE,
                          in_signature='s',
                          out_signature='s',
@@ -172,6 +220,7 @@
         if not sent:
             failure('No send_enabled accounts found.')
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE,
                          in_signature='sss',
                          out_signature='s',
@@ -205,6 +254,7 @@
             failure(message)
             log.error(message)
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE,
                          in_signature='sss',
                          out_signature='s',
@@ -262,6 +312,7 @@
             failure(message)
             log.error(message)
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE, in_signature='s', out_signature='s')
     def GetFeatures(self, protocol_name):
         """Returns a list of features supported by service as json string.
@@ -274,9 +325,9 @@
             features = json.loads(service.GetFeatures('facebook'))
         """
         protocol = protocol_manager.protocols.get(protocol_name)
-        GLib.idle_add(self.mainloop.quit)
-        return json.dumps(protocol.get_features())
+        return json.dumps(protocol.get_features() if protocol else [])
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE, in_signature='s', out_signature='s')
     def URLShorten(self, url):
         """Shorten a URL.
@@ -291,7 +342,6 @@
             service = dbus.Interface(obj, DBUS_INTERFACE)
             short_url = service.URLShorten(url)
         """
-        GLib.idle_add(self.mainloop.quit)
         service_name = self.settings.get_string('urlshorter')
         log.info('Shortening URL {} with {}'.format(url, service_name))
         if (lookup.is_shortened(url) or
@@ -305,7 +355,7 @@
             log.exception('URL shortening class: {}'.format(service))
             return url
 
+    @exit_after_idle
     @dbus.service.method(DBUS_INTERFACE)
     def ExpireAvatars(self):
         Avatar.expire_old_avatars()
-        GLib.idle_add(self.mainloop.quit)

=== modified file 'friends/tests/test_dispatcher.py'
--- friends/tests/test_dispatcher.py	2013-03-13 04:36:33 +0000
+++ friends/tests/test_dispatcher.py	2013-03-20 01:26:22 +0000
@@ -26,7 +26,7 @@
 
 from dbus.mainloop.glib import DBusGMainLoop
 
-from friends.service.dispatcher import Dispatcher, STUB
+from friends.service.dispatcher import Dispatcher, ManageTimers, STUB
 from friends.tests.mocks import LogMock, mock
 
 
@@ -61,7 +61,11 @@
         self.dispatcher.account_manager.get_all.assert_called_once_with()
         account.protocol.assert_called_once_with('receive')
 
-        self.assertEqual(self.log_mock.empty(), 'Refresh requested\n')
+        self.assertEqual(self.log_mock.empty(),
+                         'Clearing 1 shutdown timer(s)...\n'
+                         'Refresh requested\n'
+                         'Clearing 0 shutdown timer(s)...\n'
+                         'Starting new shutdown timer...\n')
 
     def test_clear_indicators(self):
         self.dispatcher.menu_manager = mock.Mock()
@@ -81,7 +85,10 @@
             'like', '23346356767354626', success=STUB, failure=STUB)
 
         self.assertEqual(self.log_mock.empty(),
-                         '345: like 23346356767354626\n')
+                         'Clearing 1 shutdown timer(s)...\n'
+                         '345: like 23346356767354626\n'
+                         'Clearing 0 shutdown timer(s)...\n'
+                         'Starting new shutdown timer...\n')
 
     def test_failing_do(self):
         account = mock.Mock()
@@ -93,7 +100,10 @@
         self.assertEqual(account.protocol.call_count, 0)
 
         self.assertEqual(self.log_mock.empty(),
-                         'Could not find account: 6\n')
+                         'Clearing 1 shutdown timer(s)...\n'
+                         'Could not find account: 6\n'
+                         'Clearing 0 shutdown timer(s)...\n'
+                         'Starting new shutdown timer...\n')
 
     def test_send_message(self):
         account1 = mock.Mock()
@@ -128,7 +138,10 @@
             success=STUB, failure=STUB)
 
         self.assertEqual(self.log_mock.empty(),
-                         'Replying to 2, objid\n')
+                         'Clearing 1 shutdown timer(s)...\n'
+                         'Replying to 2, objid\n'
+                         'Clearing 0 shutdown timer(s)...\n'
+                         'Starting new shutdown timer...\n')
 
     def test_send_reply_failed(self):
         account = mock.Mock()
@@ -140,8 +153,11 @@
         self.assertEqual(account.protocol.call_count, 0)
 
         self.assertEqual(self.log_mock.empty(),
-                         'Replying to 2, objid\n' +
-                         'Could not find account: 2\n')
+                         'Clearing 1 shutdown timer(s)...\n'
+                         'Replying to 2, objid\n'
+                         'Could not find account: 2\n'
+                         'Clearing 0 shutdown timer(s)...\n'
+                         'Starting new shutdown timer...\n')
 
     def test_upload_async(self):
         account = mock.Mock()
@@ -166,7 +182,10 @@
             )
 
         self.assertEqual(self.log_mock.empty(),
-                         'Uploading file://path/to/image.png to 2\n')
+                         'Clearing 1 shutdown timer(s)...\n'
+                         'Uploading file://path/to/image.png to 2\n'
+                         'Clearing 0 shutdown timer(s)...\n'
+                         'Starting new shutdown timer...\n')
 
     def test_get_features(self):
         self.assertEqual(json.loads(self.dispatcher.GetFeatures('facebook')),
@@ -205,6 +224,52 @@
             self.dispatcher.URLShorten(long_url),
             'short url')
         lookup_mock.is_shortened.assert_called_once_with(long_url)
-        self.dispatcher.settings.get_boolean.assert_called_once_with('shorten-urls')
+        self.dispatcher.settings.get_boolean.assert_called_once_with(
+            'shorten-urls')
         lookup_mock.lookup.assert_called_once_with('is.gd')
-        lookup_mock.lookup.return_value.shorten.assert_called_once_with(long_url)
+        lookup_mock.lookup.return_value.shorten.assert_called_once_with(
+            long_url)
+
+    @mock.patch('friends.service.dispatcher.GLib')
+    def test_manage_timers_clear(self, glib):
+        manager = ManageTimers()
+        manager.timers = {1}
+        manager.__enter__()
+        glib.source_remove.assert_called_once_with(1)
+        manager.timers = {1, 2, 3}
+        manager.clear_all_timers()
+        self.assertEqual(glib.source_remove.call_count, 4)
+
+    @mock.patch('friends.service.dispatcher.GLib')
+    def test_manage_timers_set(self, glib):
+        manager = ManageTimers()
+        manager.timers = set()
+        manager.clear_all_timers = mock.Mock()
+        manager.__exit__()
+        glib.timeout_add_seconds.assert_called_once_with(30, manager.terminate)
+        manager.clear_all_timers.assert_called_once_with()
+        self.assertEqual(len(manager.timers), 1)
+
+    @mock.patch('friends.service.dispatcher.persist_model')
+    @mock.patch('friends.service.dispatcher.threading')
+    @mock.patch('friends.service.dispatcher.GLib')
+    def test_manage_timers_terminate(self, glib, thread, persist):
+        manager = ManageTimers()
+        manager.timers = set()
+        thread.activeCount.return_value = 1
+        manager.terminate()
+        thread.activeCount.assert_called_once_with()
+        persist.assert_called_once_with()
+        glib.idle_add.assert_called_once_with(manager.callback)
+
+    @mock.patch('friends.service.dispatcher.persist_model')
+    @mock.patch('friends.service.dispatcher.threading')
+    @mock.patch('friends.service.dispatcher.GLib')
+    def test_manage_timers_dont_kill_threads(self, glib, thread, persist):
+        manager = ManageTimers()
+        manager.timers = set()
+        manager.set_new_timer = mock.Mock()
+        thread.activeCount.return_value = 10
+        manager.terminate()
+        thread.activeCount.assert_called_once_with()
+        manager.set_new_timer.assert_called_once_with()

=== modified file 'friends/utils/base.py'
--- friends/utils/base.py	2013-03-19 04:17:58 +0000
+++ friends/utils/base.py	2013-03-20 01:26:22 +0000
@@ -92,9 +92,6 @@
 # publishing new data into the SharedModel.
 _publish_lock = threading.Lock()
 
-# Avoid race condition during shut-down
-_exit_lock = threading.Lock()
-
 
 log = logging.getLogger(__name__)
 
@@ -138,8 +135,6 @@
 
 class _OperationThread(threading.Thread):
     """Manage async callbacks, and log subthread exceptions."""
-    # main.py will replace this with a reference to the mainloop.quit method
-    shutdown = lambda: log.error('Failed to exit friends-dispatcher main loop')
 
     def __init__(self, *args, id=None, success=STUB, failure=STUB, **kws):
         self._id = id
@@ -171,13 +166,6 @@
         log.debug('{} has completed in {:.2f}s, thread exiting.'.format(
                 self._id, elapsed))
 
-        # If this is the last thread to exit, then the refresh is
-        # completed and we should save the model, and then exit.
-        with _exit_lock:
-            if threading.activeCount() < 3:
-                persist_model()
-                GLib.idle_add(self.shutdown)
-
 
 class Base:
     """Parent class for any protocol plugin such as Facebook or Twitter.