Merge lp:~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear into lp:desktopcouch

Proposed by Chad Miller on 2009-10-01
Status: Merged
Approved by: Chad Miller on 2009-10-01
Approved revision: 81
Merged at revision: not available
Proposed branch: lp:~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear
Merge into: lp:desktopcouch
Diff against target: 239 lines
2 files modified
desktopcouch/pair/couchdb_pairing/dbus_io.py (+35/-48)
desktopcouch/replication.py (+9/-11)
To merge this branch: bzr merge lp:~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear
Reviewer Review Type Date Requested Status
Eric Casteleijn (community) 2009-10-01 Approve on 2009-10-01
Review via email: mp+12747@code.launchpad.net

Commit message

Be explicit in references, as evidently there is a way for the name/reference to disappear. (LP: #440072)

Do not attempt to resolve messages for nearby machines when they disappear.

Fix a few unreported bugs in error handling.

To post a comment you must log in.
Eric Casteleijn (thisfred) wrote :

in replication.py, the following imports are unused:

import threading
import logging.handlers
import desktopcouch

this is unconventionally indented and uses an undefined variable 'sn' (occurs twice):

        log.error("The service %r is unknown. It is not a "
                "module in the %s package ." % (sn, container))
        return ""

this has an undefined variable 'remote_identifier'

                        couchdb_io.expunge_pairing(remote_identifier)

and there are several long lines.

dbus_io

args is undefined in:

             logging.error("annc doesn't look like one of ours. %r", args)

also, this scares me:

class LocationAdvertisement(Advertisement):
    """An advertised couchdb location. See Advertisement class."""
    def __init__(self, *args, **kwargs):
        if "stype" in kwargs:
            kwargs.pop(stype)
        super(LocationAdvertisement, self).__init__(
                stype=location_discovery_service_type, *args, **kwargs)

class PairAdvertisement(Advertisement):
    """An advertised couchdb pairing opportunity. See Advertisement class."""
    def __init__(self, *args, **kwargs):
        if "stype" in kwargs:
            kwargs.pop(stype)
        super(PairAdvertisement, self).__init__(
                stype=invitations_discovery_service_type, *args, **kwargs)

Isn't it possible to make it a little more explicit? I think I would prefer:

kwargs.pop(kwargs["stype"])

review: Needs Fixing
80. By Chad Miller on 2009-10-01

Clean up Eric's complaints. This fixes three bugs.

Eric Casteleijn (thisfred) wrote :

Looks great, tests pass!

review: Approve
81. By Chad Miller on 2009-10-01

Eric is pleasantly persistent. More cleanups, more bug fixes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'desktopcouch/pair/couchdb_pairing/dbus_io.py'
2--- desktopcouch/pair/couchdb_pairing/dbus_io.py 2009-09-30 15:52:57 +0000
3+++ desktopcouch/pair/couchdb_pairing/dbus_io.py 2009-10-01 21:11:09 +0000
4@@ -103,18 +103,14 @@
5 class LocationAdvertisement(Advertisement):
6 """An advertised couchdb location. See Advertisement class."""
7 def __init__(self, *args, **kwargs):
8- if "stype" in kwargs:
9- kwargs.pop(stype)
10- super(LocationAdvertisement, self).__init__(
11- stype=location_discovery_service_type, *args, **kwargs)
12+ kwargs["stype"] = location_discovery_service_type
13+ super(LocationAdvertisement, self).__init__(*args, **kwargs)
14
15 class PairAdvertisement(Advertisement):
16 """An advertised couchdb pairing opportunity. See Advertisement class."""
17 def __init__(self, *args, **kwargs):
18- if "stype" in kwargs:
19- kwargs.pop(stype)
20- super(PairAdvertisement, self).__init__(
21- stype=invitations_discovery_service_type, *args, **kwargs)
22+ kwargs["stype"] = invitations_discovery_service_type
23+ super(PairAdvertisement, self).__init__(*args, **kwargs)
24
25 def avahitext_to_dict(avahitext):
26 text = {}
27@@ -155,51 +151,39 @@
28 def maintain_discovered_servers(add_cb=cb_found_desktopcouch_server,
29 del_cb=cb_lost_desktopcouch_server):
30
31- def remove_item_handler(interface, protocol, name, stype, domain, flags):
32+ def remove_item_handler(cb, interface, protocol, name, stype, domain,
33+ flags):
34 """A service disappeared."""
35
36- def handle_error(*args):
37- """An error in resolving a new service."""
38- logging.error("zeroconf ItemNew error for services, %s", args)
39-
40- def handle_resolved(*args):
41- """Successfully resolved a new service, which we decode and send
42- back to our calling environment with the callback function."""
43-
44- name, host, port = args[2], args[5], args[8]
45- if name.startswith("desktopcouch "):
46- hostid = name[13:]
47- logging.debug("lost sight of %r", hostid)
48- del_cb(hostid)
49- else:
50- logging.error("no UUID in zeroconf message, %r", args)
51-
52- server.ResolveService(interface, protocol, name, stype,
53- domain, avahi.PROTO_UNSPEC, dbus.UInt32(0),
54- reply_handler=handle_resolved, error_handler=handle_error)
55-
56- def new_item_handler(interface, protocol, name, stype, domain, flags):
57+ if name.startswith("desktopcouch "):
58+ hostid = name[13:]
59+ logging.debug("lost sight of %r", hostid)
60+ cb(hostid)
61+ else:
62+ logging.error("annc doesn't look like one of ours. %r", name)
63+
64+ def new_item_handler(cb, interface, protocol, name, stype, domain, flags):
65 """A service appeared."""
66
67 def handle_error(*args):
68 """An error in resolving a new service."""
69 logging.error("zeroconf ItemNew error for services, %s", args)
70
71- def handle_resolved(*args):
72+ def handle_resolved(cb, *args):
73 """Successfully resolved a new service, which we decode and send
74 back to our calling environment with the callback function."""
75
76 name, host, port = args[2], args[5], args[8]
77- # FIXME strip off "desktopcouch "
78 if name.startswith("desktopcouch "):
79- add_cb(name[13:], host, port)
80+ cb(name[13:], host, port)
81 else:
82- logging.error("no UUID in zeroconf message, %r", name)
83+ logging.error("annc doesn't look like one of ours. %r", name)
84 return True
85
86 server.ResolveService(interface, protocol, name, stype,
87 domain, avahi.PROTO_UNSPEC, dbus.UInt32(0),
88- reply_handler=handle_resolved, error_handler=handle_error)
89+ reply_handler=lambda *a: handle_resolved(cb, *a),
90+ error_handler=handle_error)
91
92 bus, server = get_dbus_bus_server()
93 domain_name = get_local_hostname()[1]
94@@ -209,8 +193,10 @@
95
96 sbrowser = dbus.Interface(browser_name,
97 avahi.DBUS_INTERFACE_SERVICE_BROWSER)
98- sbrowser.connect_to_signal("ItemNew", new_item_handler)
99- sbrowser.connect_to_signal("ItemRemove", remove_item_handler)
100+ sbrowser.connect_to_signal("ItemNew",
101+ lambda *a: new_item_handler(add_cb, *a))
102+ sbrowser.connect_to_signal("ItemRemove",
103+ lambda *a: remove_item_handler(del_cb, *a))
104 sbrowser.connect_to_signal("Failure",
105 lambda *a: logging.error("avahi error %r", a))
106
107@@ -220,27 +206,26 @@
108 """Start looking for services. Use two callbacks to handle seeing
109 new services and seeing services disappear."""
110
111- def remove_item_handler(interface, protocol, name, stype, domain, flags):
112+ def remove_item_handler(cb, interface, protocol, name, stype, domain, flags):
113 """A service disappeared."""
114
115 if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL:
116 return
117-
118- del_commport_name_cb(name)
119-
120- def new_item_handler(interface, protocol, name, stype, domain, flags):
121+ cb(name)
122+
123+ def new_item_handler(cb, interface, protocol, name, stype, domain, flags):
124 """A service appeared."""
125
126 def handle_error(*args):
127 """An error in resolving a new service."""
128 logging.error("zeroconf ItemNew error for services, %s", args)
129
130- def handle_resolved(*args):
131+ def handle_resolved(cb, *args):
132 """Successfully resolved a new service, which we decode and send
133 back to our calling environment with the callback function."""
134 text = avahitext_to_dict(args[9])
135 name, host, port = args[2], args[5], args[8]
136- add_commport_name_cb(name, text.get("description", "?"),
137+ cb(name, text.get("description", "?"),
138 host, port, text.get("version", None))
139
140 if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL:
141@@ -248,8 +233,8 @@
142
143 server.ResolveService(interface, protocol, name, stype,
144 domain, avahi.PROTO_UNSPEC, dbus.UInt32(0),
145- reply_handler=handle_resolved, error_handler=handle_error)
146-
147+ reply_handler=lambda *a: handle_resolved(cb, *a),
148+ error_handler=handle_error)
149
150 bus, server = get_dbus_bus_server()
151 domain_name = get_local_hostname()[1]
152@@ -260,7 +245,9 @@
153
154 sbrowser = dbus.Interface(browser_name,
155 avahi.DBUS_INTERFACE_SERVICE_BROWSER)
156- sbrowser.connect_to_signal("ItemNew", new_item_handler)
157- sbrowser.connect_to_signal("ItemRemove", remove_item_handler)
158+ sbrowser.connect_to_signal("ItemNew",
159+ lambda *a: new_item_handler(add_commport_name_cb, *a))
160+ sbrowser.connect_to_signal("ItemRemove",
161+ lambda *a: remove_item_handler(del_commport_name_cb, *a))
162 sbrowser.connect_to_signal("Failure",
163 lambda *a: logging.error("avahi error %r", a))
164
165=== modified file 'desktopcouch/replication.py'
166--- desktopcouch/replication.py 2009-09-30 19:30:37 +0000
167+++ desktopcouch/replication.py 2009-10-01 21:11:09 +0000
168@@ -16,14 +16,11 @@
169 #
170 # Authors: Chad Miller <chad.miller@canonical.com>
171
172-import threading
173 import logging
174-import logging.handlers
175 log = logging.getLogger("replication")
176
177 import dbus.exceptions
178
179-import desktopcouch
180 from desktopcouch.pair.couchdb_pairing import couchdb_io
181 from desktopcouch.pair.couchdb_pairing import dbus_io
182 from desktopcouch import replication_services
183@@ -52,7 +49,7 @@
184 return getattr(mod, service_name).db_name_prefix
185 except ImportError, e:
186 log.error("The service %r is unknown. It is not a "
187- "module in the %s package ." % (sn, container))
188+ "module in the %s package ." % (service_name, container))
189 return ""
190 except Exception, e:
191 log.exception("Not changing remote db name.")
192@@ -68,7 +65,7 @@
193 return getattr(mod, service_name).get_oauth_data()
194 except ImportError, e:
195 log.error("The service %r is unknown. It is not a "
196- "module in the %s package ." % (sn, container))
197+ "module in the %s package ." % (service_name, container))
198 return None
199
200 def do_all_replication(local_port):
201@@ -106,11 +103,12 @@
202 # push caught exception back...
203 except:
204 # ... so that we log it here.
205- logging.exception("failed to unpair from other end.")
206+ logging.exception(
207+ "failed to unpair from other end.")
208 continue
209 else:
210 # Finally, find your inner peace...
211- couchdb_io.expunge_pairing(remote_identifier)
212+ couchdb_io.expunge_pairing(remote_hostid)
213 # ...and move on.
214 continue
215
216@@ -184,10 +182,12 @@
217 log.error("skipping %r on %s. %s", db_name, sn, e)
218 continue
219
220- db_name = remote_db_name[1+len(str(remote_db_name_prefix)):]
221+ prefix_len = len(str(remote_db_name_prefix))
222+ db_name = remote_db_name[1+prefix_len:]
223 if db_name.strip("/") == "management":
224 continue # be paranoid about what we accept.
225- log.debug("want to replipull %r from static host %r @ %s",
226+ log.debug(
227+ "want to replipull %r from static host %r @ %s",
228 db_name, remote_hostid, addr)
229 couchdb_io.replicate(remote_db_name, db_name,
230 source_host=addr, source_port=port,
231@@ -224,8 +224,6 @@
232 "our zeroconf advert. %s", e)
233 return None
234
235- dbus_io.discover_services(None, None, True)
236-
237 dbus_io.maintain_discovered_servers()
238
239 t = task.LoopingCall(replicate_local_databases_to_paired_hosts, port)

Subscribers

People subscribed via source and target branches