Merge lp:~dobey/ubuntuone-client/http-threaded into lp:ubuntuone-client

[dobey]

Fix the HTTP REST calls to load from threads, instead of GIOChannels
  - This avoids blocking the main loop for network activity
Fix the callback handlers for the REST calls to use obj.get method
  - Avoids potential KeyError conditions
Fix oauthdesktop to call ensure_access_token in a timeout instead of thread
  - Avoids threadlock due to changes in libgnome-keyring

[Natalia Bidart (nataliabidart)]

There is a lint issue:

== Python Lint Notices ==

./bin/ubuntuone-preferences:
    27: 'glib' imported but unused

[lp:~dobey/ubuntuone-client/http-threaded]

454. By dobey on 2010-03-30

Fix the lint warning

[Natalia Bidart (nataliabidart)]

review: Approve

[Tim Cole (tcole)]

Hmm... this should work I think. As an alternative to calling callback(result), you might want to consider using g_add_idle to have the callback run in the event thread; that may make things less surprising down the road.

review: Approve

[Tim Cole (tcole)]

Well. Not sure it's necessary going through add_idle here. But do consider using the gtk.gdk.lock context manager rather than explicit calls to threads_enter/threads_exit; it's probably more important here than in the other branch, since a generic callback is (in principle) more likely to raise an exception.

[Joshua Hoover (joshuahoover)]

Tested on Lucid. Performance is much improved versus previous branches where interface froze for up to 30 seconds after launching. Worked even when network was turned off.

review: Approve (ran on lucid)

=== modified file 'bin/ubuntuone-preferences'
--- bin/ubuntuone-preferences	2010-03-29 22:50:39 +0000
+++ bin/ubuntuone-preferences	2010-03-30 17:34:28 +0000
@@ -24,12 +24,12 @@
 import pygtk
 pygtk.require('2.0')
 import gobject
-import glib
 import gtk
 import os
 import gettext
 import gnomekeyring
 import simplejson
+from threading import Thread
 from oauth import oauth
 from ubuntuone import clientdefs
 from ubuntuone.syncdaemon.tools import SyncDaemonTool
@@ -97,26 +97,11 @@
     secret = items[0].secret
     return oauth.OAuthToken.from_string(secret)
 
-def rest_response_handler(source, condition, conn, callback):
+def do_rest_request(url, method, token, callback):
     """
     Helper that handles the REST response.
     """
     result = None
-    response = conn.getresponse() # shouldn't block
-    if response.status == 200:
-        data = response.read() # neither should this
-        result = simplejson.loads(data)
-    else:
-        result = response.reason
-    callback(result)
-    return False
-
-def make_rest_request(url=None, method='GET',
-                      callback=None, keyring=None):
-    """
-    Helper that makes an oauth-wrapped REST request.
-    """
-    token = get_access_token(keyring)
     consumer = oauth.OAuthConsumer('ubuntuone', 'hammertime')
 
     oauth_request = oauth.OAuthRequest.from_consumer_and_token(
@@ -133,13 +118,27 @@
     conn = httplib.HTTPSConnection(netloc)
     try:
         conn.request(method, path, headers=oauth_request.to_header())
-    except socket.error:
-        callback(None)
-    else:
-        glib.io_add_watch(
-            conn.sock,
-            glib.IO_IN | glib.IO_PRI | glib.IO_ERR | glib.IO_HUP,
-            rest_response_handler, conn, callback)
+        response = conn.getresponse() # shouldn't block
+        if response.status == 200:
+            data = response.read() # neither should this
+            result = simplejson.loads(data)
+        else:
+            result = {'status' : response.status,
+                      'reason' : response.reason}
+    except socket.error, e:
+        result = {'error' : e}
+
+    gtk.gdk.threads_enter()
+    callback(result)
+    gtk.gdk.threads_leave()
+
+def make_rest_request(url=None, method='GET',
+                      callback=None, keyring=None):
+    """
+    Helper that makes an oauth-wrapped REST request.
+    """
+    token = get_access_token(keyring)
+    Thread(target=do_rest_request, args=(url, method, token, callback)).start()
 
 
 class DevicesWidget(gtk.Table):
@@ -301,12 +300,27 @@
         """
         Parse the list of devices, and hook up list_devices if it worked.
         """
-        if result:
+        is_error = False
+        error = None
+        if result and isinstance(result, list):
             self.devices = result
-            gobject.idle_add(self.list_devices)
+        elif isinstance(result, dict):
+            error = result.get('error', None)
+            if not error and result.get('status', None):
+                is_error = True
+                error = result.get('reason', None)
+            else:
+                is_error = True
         else:
-            self.clear_devices_view()
-            self.error(result)
+            error = "Got empty result for devices list."
+            is_error = True
+
+        if is_error:
+            self.devices = None
+            self.error(error)
+
+        gobject.idle_add(self.list_devices)
+
 
     def clear_devices_view(self):
         """
@@ -330,8 +344,6 @@
         If the list of devices is empty, make a fake one that refers
         to the local machine (to get the connect/restart buttons).
         """
-        self.resize(len(self.devices)+1, 3)
-
         self.clear_devices_view()
 
         token = get_access_token(self.keyring)
@@ -342,6 +354,8 @@
                              'description': _("<LOCAL MACHINE>"),
                              'token': token.key,
                              'FAKE': 'YES'}]
+        else:
+            self.resize(len(self.devices)+1, 3)
 
         self.status_label.set_label("")
 
@@ -599,7 +613,9 @@
     def got_quota_info(self, quota):
         """Handle the result from the quota REST call."""
         if quota:
-            self.update_quota_display(quota['used'], quota['total'])
+            used = quota.get('used', 0)
+            total = quota.get('total', 2)
+            self.update_quota_display(used, total)
 
     def request_quota_info(self):
         """Request new quota info from server, and update display."""
@@ -610,8 +626,8 @@
     def got_account_info(self, user):
         """Handle the result from the account REST call."""
         if user:
-            self.name_label.set_text(user['nickname'])
-            self.mail_label.set_text(user['email'])
+            self.name_label.set_text(user.get('nickname', _("Unknown")))
+            self.mail_label.set_text(user.get('email', _("Unknown")))
 
     def request_account_info(self):
         """Request account info from server, and update display."""
@@ -994,13 +1010,18 @@
     gettext.bindtextdomain(clientdefs.GETTEXT_PACKAGE, clientdefs.LOCALEDIR)
     gettext.textdomain(clientdefs.GETTEXT_PACKAGE)
 
+    gobject.threads_init()
+    gtk.gdk.threads_init()
+
     gtk.rc_parse_string(RCSTYLE)
     gobject.set_application_name("ubuntuone-preferences")
 
     try:
+        gtk.gdk.threads_enter()
         login = UbuntuoneLoginHandler()
         login.register_signal_handlers()
         login.do_login_check()
         gtk.main()
+        gtk.gdk.threads_leave()
     except KeyboardInterrupt:
         pass

=== modified file 'ubuntuone/oauthdesktop/main.py'
--- ubuntuone/oauthdesktop/main.py	2010-01-18 12:29:29 +0000
+++ ubuntuone/oauthdesktop/main.py	2010-03-30 17:34:28 +0000
@@ -25,7 +25,7 @@
 signal so they can retrieve the new token.
 """
 
-import dbus.service, urlparse, time
+import dbus.service, urlparse, time, gobject
 import pynotify
 
 from dbus.mainloop.glib import DBusGMainLoop
@@ -91,7 +91,7 @@
                                      do_login=do_login)
 
         logger.debug("Calling auth.client.ensure_access_token in thread")
-        deferToThread(client.ensure_access_token).addErrback(self.error_handler)
+        gobject.timeout_add_seconds(1, client.ensure_access_token)
 
     def clear_token(self, realm, consumer_key):
         """Remove the currently stored OAuth token from the keyring."""