Merge lp:~nafai/gwibber/gnomekeyring-fix into lp:gwibber

Proposed by Travis B. Hartwell on 2010-04-08
Status: Merged
Merged at revision: not available
Proposed branch: lp:~nafai/gwibber/gnomekeyring-fix
Merge into: lp:gwibber
Diff against target: 187 lines (+75/-18)
4 files modified
bin/gwibber-service (+1/-3)
gwibber/accounts.py (+0/-1)
gwibber/microblog/dispatcher.py (+25/-14)
gwibber/microblog/util/keyring.py (+49/-0)
To merge this branch: bzr merge lp:~nafai/gwibber/gnomekeyring-fix
Reviewer Review Type Date Requested Status
Martin Pitt 2010-04-08 Pending
James W 2010-04-08 Pending
Ken VanDine 2010-04-08 Pending
Review via email: mp+22994@code.launchpad.net

Description of the Change

This is a proposed fix for bug #554005.

Since accessing Gnome Keyring from threads causes a spike in CPU
usage, I have moved the access to gnome-keyring to the start of
gwibber-service, before threads are started. This section loads the
passwords from the keyring into a dictionary and then provides a
method for lookup. Then the calls to gnomekeyring are replaced with a
call to that method.

This patch handles the CPU pegging problem, but isn't a complete fix.
The following needs still to be done:

1) pitti proposed that the memory where this is stored should be
   mlock()'d. He suggested we use ctypes to wrap the calls to mlock
   and munlock. I've included stub functions, and pitti indicated he
   could help with wrapping this.

2) This implementation only reads the accounts and their passwords at
   the beginning of gwibber-service. The previous implementation
   caught changes to the passwords as well as the addition and removal
   of accounts by whenever actions are taken. Code still needs to be
   added to handle this. There are a couple of options that I can
   think of:

   a) Start and restart the gwibber service when these changes are
      made (in gwibber/accounts.py, I believe). I'm not sure if there
      are unforseen problems with this -- for example,
      gwibber-accounts can be run in a separate process, I believe.
      How does the main gwibber process handle loosing the gwibber
      service connection?

   b) Add a method to the com.Gwibber.Accounts dbus service for
      updating or adding account information (id and password). This
      method would then call a function we could add to
      gwibber.microblog.keyring to modify the passwords dictionary.
      The problem I see with this is that other developers who may
      look at the dbus services may assume this actually changes the
      passwords, not just the in-memory data structure.

   I imagine either of these options shouldn't be too hard to
   implement. I'm hoping that Ken or someone else can find a quick
   way to do this.

To post a comment you must log in.
Ken VanDine (ken-vandine) wrote :

Good work, I'll look in more detail in the morning. Since gwibber-service is dbus activated, we could just make gwibber-accounts call the Quit method on the dispatcher and then before exiting itself start it back up via dbus. Before doing this, it should actually verify it was running.

lp:~nafai/gwibber/gnomekeyring-fix updated on 2010-04-08
712. By Travis B. Hartwell on 2010-04-08

Merge Ken's threading changes and other updates.

713. By Travis B. Hartwell on 2010-04-08

Move loading account passwords to the Dispatcher class and expose it
via DBus.

714. By Travis B. Hartwell on 2010-04-08

Access mlock/munlock via ctypes.

715. By Travis B. Hartwell on 2010-04-08

Call munlock properly

716. By Travis B. Hartwell on 2010-04-08

Clean-ups per pitti's suggestions.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/gwibber-service'
2--- bin/gwibber-service 2010-04-03 18:10:54 +0000
3+++ bin/gwibber-service 2010-04-08 16:24:30 +0000
4@@ -72,7 +72,7 @@
5 log.logger.addHandler(console)
6
7 ## Check to see if the database needs to be purged
8-dispatcher.purge()
9+#dispatcher.purge()
10
11 log.logger.debug("Setting up monitors")
12 connection_monitor = dispatcher.ConnectionMonitor()
13@@ -83,6 +83,4 @@
14 log.logger.debug("Monitors are up")
15
16 dispatcher = dispatcher.Dispatcher(loop)
17-dispatcher.daemon = True
18-dispatcher.start()
19 loop.run()
20
21=== modified file 'gwibber/accounts.py'
22--- gwibber/accounts.py 2010-04-03 17:20:33 +0000
23+++ gwibber/accounts.py 2010-04-08 16:24:30 +0000
24@@ -332,7 +332,6 @@
25 value = [getattr(widget.props, p) for p in ["text", "active", "color"] if widget and hasattr(widget.props, p)]
26
27 if value:
28-
29 if isinstance(value[0], gtk.gdk.Color):
30 self.account[config] = gtk.color_selection_palette_to_string(
31 gtk.color_selection_palette_from_string(value[0].to_string()))
32
33=== modified file 'gwibber/microblog/dispatcher.py'
34--- gwibber/microblog/dispatcher.py 2010-04-05 17:49:37 +0000
35+++ gwibber/microblog/dispatcher.py 2010-04-08 16:24:30 +0000
36@@ -1,13 +1,14 @@
37 #!/usr/bin/env python
38 # -*- coding: utf-8 -*-
39
40-import multiprocessing, threading, traceback, json, time
41+import multiprocessing, traceback, json, time
42 import gobject, dbus, dbus.service, mx.DateTime
43 import twitter, identica, statusnet, flickr, facebook
44-import qaiku, friendfeed, digg, gnomekeyring
45+import qaiku, friendfeed, digg
46
47 import urlshorter
48 import util, util.couch
49+import util.keyring
50 from util import log
51 from util import resources
52 from util.couch import Monitor as CouchMonitor
53@@ -58,11 +59,9 @@
54
55 for key, val in account.items():
56 if isinstance(val, str) and val.startswith(":KEYRING:"):
57- try:
58- value = gnomekeyring.find_items_sync(
59- gnomekeyring.ITEM_GENERIC_SECRET,
60- {"id": str("%s/%s" % (account["_id"], key))})[0].secret
61- except gnomekeyring.NoMatchError:
62+ value = util.keyring.get_account_password(account["_id"])
63+
64+ if value is None:
65 raise exceptions.GwibberProtocolError("keyring")
66 return ("Failure", 0)
67
68@@ -339,16 +338,14 @@
69 expire_timeout = 5000
70 n = util.notify(sender_name, message["text"], image, expire_timeout)
71
72-class MapAsync(threading.Thread):
73+class MapAsync:
74 def __init__(self, func, iterable, cbsuccess, cbfailure, timeout=120):
75- threading.Thread.__init__(self)
76 self.iterable = iterable
77 self.callback = cbsuccess
78 self.failure = cbfailure
79 self.timeout = timeout
80- self.daemon = True
81 self.func = func
82- self.start()
83+ self.run()
84
85 def run(self):
86 try:
87@@ -357,7 +354,7 @@
88 except Exception as e:
89 self.failure(e, traceback.format_exc())
90
91-class Dispatcher(dbus.service.Object, threading.Thread):
92+class Dispatcher(dbus.service.Object):
93 """
94 The Gwibber Dispatcher handles all the backend operations.
95 """
96@@ -367,13 +364,14 @@
97 self.bus = dbus.SessionBus()
98 bus_name = dbus.service.BusName("com.Gwibber.Service", bus=self.bus)
99 dbus.service.Object.__init__(self, bus_name, self.__dbus_object_path__)
100- threading.Thread.__init__(self)
101
102 self.collector = OperationCollector()
103
104 self.refresh_count = 0
105 self.mainloop = loop
106
107+ self.RefreshCreds()
108+
109 if autorefresh:
110 self.refresh()
111
112@@ -501,7 +499,20 @@
113 """
114 log.logger.info("Gwibber Service is being shutdown")
115 self.mainloop.quit()
116-
117+
118+ @dbus.service.method("com.Gwibber.Service")
119+ def RefreshCreds(self):
120+ """
121+ Reload accounts and credentials from Gnome Keyring
122+ example:
123+ import dbus
124+ obj = dbus.SessionBus().get_object("com.Gwibber.Service", "/com/gwibber/Service")
125+ service = dbus.Interface(obj, "com.Gwibber.Service")
126+ service.RefreshCreds()
127+ """
128+ log.logger.info("Gwibber Service is reloading account credentials")
129+ util.keyring.get_account_passwords()
130+
131 def loading_complete(self, output):
132 self.refresh_count += 1
133 self.LoadingComplete()
134
135=== added file 'gwibber/microblog/util/keyring.py'
136--- gwibber/microblog/util/keyring.py 1970-01-01 00:00:00 +0000
137+++ gwibber/microblog/util/keyring.py 2010-04-08 16:24:30 +0000
138@@ -0,0 +1,49 @@
139+from desktopcouch.records.server import CouchDatabase
140+from const import *
141+
142+import atexit
143+import ctypes
144+import gnomekeyring
145+
146+passwords = {}
147+
148+def get_account_passwords():
149+ global passwords
150+
151+ accounts = CouchDatabase(COUCH_DB_ACCOUNTS, create=True)
152+ ids = [a['id'] for a in accounts.get_records().rows]
153+
154+ for id in ids:
155+ account = dict(accounts.get_record(id).items())
156+ for key, val in account.items():
157+ if isinstance(val, str) and val.startswith(":KEYRING:"):
158+ try:
159+ value = gnomekeyring.find_items_sync(
160+ gnomekeyring.ITEM_GENERIC_SECRET,
161+ {"id": str("%s/%s" % (account["_id"], key))})[0].secret
162+ mlock(value)
163+ except gnomekeyring.NoMatchError:
164+ raise exceptions.GwibberProtocolError("keyring")
165+
166+ passwords[id] = value
167+
168+def get_account_password(accountid):
169+ global passwords
170+
171+ return passwords.get(accountid, None)
172+
173+libc = ctypes.CDLL("libc.so.6")
174+
175+def mlock(var):
176+ libc.mlock(var, len(var))
177+
178+def munlock(var):
179+ libc.munlock(var, len(var))
180+
181+def munlock_passwords():
182+ global passwords
183+
184+ for acctid in passwords.keys():
185+ munlock(passwords[acctid])
186+
187+atexit.register(munlock_passwords)

Subscribers

People subscribed via source and target branches