Merge lp:~robru/friends/dont-block-refresh into lp:friends

[Robert Bruce Park]

This change prevents Twitter's rate limiter from being able to block the whole MainThread when Dispatcher.Refresh tries to join() all previous threads.

Instead, we now simply abort Dispatcher.Refresh if there is a previous refresh still pending. This approach allows Dispatcher.Refresh to always return immediately, so that it stops blocking the GLib mainloop and the Dee model, without causing any problems with multiple concurrent refreshes stepping on each other's toes, or running afoul of twitter's rate limiter.

[Ken VanDine (ken-vandine)]

Does the main thread counts as 1 in threading.activeCount() ?

[Robert Bruce Park (robru)]

Yeah, when there is only MainThread and no subthreads, then activeCount() is 1. One single subthread active makes activeCount() equal 2, etc. I doublechecked that in an interactive terminal before writing that code:

>>> import threading
>>> import time
>>> threading.activeCount()
1
>>> threading.Thread(target=lambda:time.sleep(10)).start()
>>> threading.activeCount()
2
>>> time.sleep(10)
>>> threading.activeCount()
1
>>>

[Ken VanDine (ken-vandine)]

Looks good

review: Approve

=== modified file 'friends/service/dispatcher.py'
--- friends/service/dispatcher.py	2012-12-04 21:06:49 +0000
+++ friends/service/dispatcher.py	2012-12-07 01:40:26 +0000
@@ -105,20 +105,17 @@
         self._unread_count = 0
 
         log.debug('Refresh requested')
-        # Wait for all previous actions to complete before
-        # starting a load of new ones.
-        current = threading.current_thread()
-        for thread in threading.enumerate():
-            if thread != current:
-                thread.join()
+
+        if threading.activeCount() > 1:
+            log.debug('Aborting refresh because previous refresh incomplete!')
+            return True
 
         if not self.online:
             return
 
-        # account.protocol() starts a new thread for each account present
-        # and returns immediately. So this method should be quick unless
-        # there are a bunch of old, incomplete jobs waiting around from
-        # the last refresh.
+        # account.protocol() starts a new thread and then returns
+        # immediately, so there is no delay or blocking during the
+        # execution of this method.
         for account in self.account_manager.get_all():
             try:
                 account.protocol('receive')

=== modified file 'friends/tests/test_dispatcher.py'
--- friends/tests/test_dispatcher.py	2012-12-04 21:06:49 +0000
+++ friends/tests/test_dispatcher.py	2012-12-07 01:40:26 +0000
@@ -70,22 +70,36 @@
     @mock.patch('friends.service.dispatcher.threading')
     def test_refresh(self, threading_mock):
         account = mock.Mock()
-        thread = mock.Mock()
-        threading_mock.enumerate.return_value = [thread]
-        threading_mock.current_thread.return_value = 'MainThread'
+        threading_mock.activeCount.return_value = 1
         self.dispatcher.account_manager = mock.Mock()
         self.dispatcher.account_manager.get_all.return_value = [account]
 
         self.assertTrue(self.dispatcher.Refresh())
-        threading_mock.current_thread.assert_called_once_with()
-        threading_mock.enumerate.assert_called_once_with()
-        thread.join.assert_called_once_with()
+        threading_mock.activeCount.assert_called_once_with()
 
         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')
 
+    @mock.patch('friends.service.dispatcher.threading')
+    def test_refresh_premature(self, threading_mock):
+        account = mock.Mock()
+        threading_mock.activeCount.return_value = 10
+        self.dispatcher.account_manager = mock.Mock()
+        self.dispatcher.account_manager.get_all.return_value = [account]
+
+        self.assertTrue(self.dispatcher.Refresh())
+        threading_mock.activeCount.assert_called_once_with()
+
+        self.assertEqual(self.dispatcher.account_manager.get_all.call_count, 0)
+        self.assertEqual(account.protocol.call_count, 0)
+
+        self.assertEqual(
+            self.log_mock.empty(),
+            'Refresh requested\n' +
+            'Aborting refresh because previous refresh incomplete!\n')
+
     def test_clear_indicators(self):
         self.dispatcher.menu_manager = mock.Mock()
         self.dispatcher.ClearIndicators()