Merge lp:~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear into lp:desktopcouch
- dbus-refs-explicit-and-rm-resolve-on-disappear
- Merge into trunk
Proposed by
Chad Miller
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Chad Miller | ||||
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 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Eric Casteleijn (community) | Approve | ||
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.
Description of the change
To post a comment you must log in.
- 80. By Chad Miller
-
Clean up Eric's complaints. This fixes three bugs.
Revision history for this message
Eric Casteleijn (thisfred) wrote : | # |
Looks great, tests pass!
review:
Approve
- 81. By Chad Miller
-
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 | 103 | class LocationAdvertisement(Advertisement): | 103 | class LocationAdvertisement(Advertisement): |
6 | 104 | """An advertised couchdb location. See Advertisement class.""" | 104 | """An advertised couchdb location. See Advertisement class.""" |
7 | 105 | def __init__(self, *args, **kwargs): | 105 | def __init__(self, *args, **kwargs): |
12 | 106 | if "stype" in kwargs: | 106 | kwargs["stype"] = location_discovery_service_type |
13 | 107 | kwargs.pop(stype) | 107 | super(LocationAdvertisement, self).__init__(*args, **kwargs) |
10 | 108 | super(LocationAdvertisement, self).__init__( | ||
11 | 109 | stype=location_discovery_service_type, *args, **kwargs) | ||
14 | 110 | 108 | ||
15 | 111 | class PairAdvertisement(Advertisement): | 109 | class PairAdvertisement(Advertisement): |
16 | 112 | """An advertised couchdb pairing opportunity. See Advertisement class.""" | 110 | """An advertised couchdb pairing opportunity. See Advertisement class.""" |
17 | 113 | def __init__(self, *args, **kwargs): | 111 | def __init__(self, *args, **kwargs): |
22 | 114 | if "stype" in kwargs: | 112 | kwargs["stype"] = invitations_discovery_service_type |
23 | 115 | kwargs.pop(stype) | 113 | super(PairAdvertisement, self).__init__(*args, **kwargs) |
20 | 116 | super(PairAdvertisement, self).__init__( | ||
21 | 117 | stype=invitations_discovery_service_type, *args, **kwargs) | ||
24 | 118 | 114 | ||
25 | 119 | def avahitext_to_dict(avahitext): | 115 | def avahitext_to_dict(avahitext): |
26 | 120 | text = {} | 116 | text = {} |
27 | @@ -155,51 +151,39 @@ | |||
28 | 155 | def maintain_discovered_servers(add_cb=cb_found_desktopcouch_server, | 151 | def maintain_discovered_servers(add_cb=cb_found_desktopcouch_server, |
29 | 156 | del_cb=cb_lost_desktopcouch_server): | 152 | del_cb=cb_lost_desktopcouch_server): |
30 | 157 | 153 | ||
32 | 158 | def remove_item_handler(interface, protocol, name, stype, domain, flags): | 154 | def remove_item_handler(cb, interface, protocol, name, stype, domain, |
33 | 155 | flags): | ||
34 | 159 | """A service disappeared.""" | 156 | """A service disappeared.""" |
35 | 160 | 157 | ||
57 | 161 | def handle_error(*args): | 158 | if name.startswith("desktopcouch "): |
58 | 162 | """An error in resolving a new service.""" | 159 | hostid = name[13:] |
59 | 163 | logging.error("zeroconf ItemNew error for services, %s", args) | 160 | logging.debug("lost sight of %r", hostid) |
60 | 164 | 161 | cb(hostid) | |
61 | 165 | def handle_resolved(*args): | 162 | else: |
62 | 166 | """Successfully resolved a new service, which we decode and send | 163 | logging.error("annc doesn't look like one of ours. %r", name) |
63 | 167 | back to our calling environment with the callback function.""" | 164 | |
64 | 168 | 165 | def new_item_handler(cb, interface, protocol, name, stype, domain, flags): | |
44 | 169 | name, host, port = args[2], args[5], args[8] | ||
45 | 170 | if name.startswith("desktopcouch "): | ||
46 | 171 | hostid = name[13:] | ||
47 | 172 | logging.debug("lost sight of %r", hostid) | ||
48 | 173 | del_cb(hostid) | ||
49 | 174 | else: | ||
50 | 175 | logging.error("no UUID in zeroconf message, %r", args) | ||
51 | 176 | |||
52 | 177 | server.ResolveService(interface, protocol, name, stype, | ||
53 | 178 | domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), | ||
54 | 179 | reply_handler=handle_resolved, error_handler=handle_error) | ||
55 | 180 | |||
56 | 181 | def new_item_handler(interface, protocol, name, stype, domain, flags): | ||
65 | 182 | """A service appeared.""" | 166 | """A service appeared.""" |
66 | 183 | 167 | ||
67 | 184 | def handle_error(*args): | 168 | def handle_error(*args): |
68 | 185 | """An error in resolving a new service.""" | 169 | """An error in resolving a new service.""" |
69 | 186 | logging.error("zeroconf ItemNew error for services, %s", args) | 170 | logging.error("zeroconf ItemNew error for services, %s", args) |
70 | 187 | 171 | ||
72 | 188 | def handle_resolved(*args): | 172 | def handle_resolved(cb, *args): |
73 | 189 | """Successfully resolved a new service, which we decode and send | 173 | """Successfully resolved a new service, which we decode and send |
74 | 190 | back to our calling environment with the callback function.""" | 174 | back to our calling environment with the callback function.""" |
75 | 191 | 175 | ||
76 | 192 | name, host, port = args[2], args[5], args[8] | 176 | name, host, port = args[2], args[5], args[8] |
77 | 193 | # FIXME strip off "desktopcouch " | ||
78 | 194 | if name.startswith("desktopcouch "): | 177 | if name.startswith("desktopcouch "): |
80 | 195 | add_cb(name[13:], host, port) | 178 | cb(name[13:], host, port) |
81 | 196 | else: | 179 | else: |
83 | 197 | logging.error("no UUID in zeroconf message, %r", name) | 180 | logging.error("annc doesn't look like one of ours. %r", name) |
84 | 198 | return True | 181 | return True |
85 | 199 | 182 | ||
86 | 200 | server.ResolveService(interface, protocol, name, stype, | 183 | server.ResolveService(interface, protocol, name, stype, |
87 | 201 | domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), | 184 | domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), |
89 | 202 | reply_handler=handle_resolved, error_handler=handle_error) | 185 | reply_handler=lambda *a: handle_resolved(cb, *a), |
90 | 186 | error_handler=handle_error) | ||
91 | 203 | 187 | ||
92 | 204 | bus, server = get_dbus_bus_server() | 188 | bus, server = get_dbus_bus_server() |
93 | 205 | domain_name = get_local_hostname()[1] | 189 | domain_name = get_local_hostname()[1] |
94 | @@ -209,8 +193,10 @@ | |||
95 | 209 | 193 | ||
96 | 210 | sbrowser = dbus.Interface(browser_name, | 194 | sbrowser = dbus.Interface(browser_name, |
97 | 211 | avahi.DBUS_INTERFACE_SERVICE_BROWSER) | 195 | avahi.DBUS_INTERFACE_SERVICE_BROWSER) |
100 | 212 | sbrowser.connect_to_signal("ItemNew", new_item_handler) | 196 | sbrowser.connect_to_signal("ItemNew", |
101 | 213 | sbrowser.connect_to_signal("ItemRemove", remove_item_handler) | 197 | lambda *a: new_item_handler(add_cb, *a)) |
102 | 198 | sbrowser.connect_to_signal("ItemRemove", | ||
103 | 199 | lambda *a: remove_item_handler(del_cb, *a)) | ||
104 | 214 | sbrowser.connect_to_signal("Failure", | 200 | sbrowser.connect_to_signal("Failure", |
105 | 215 | lambda *a: logging.error("avahi error %r", a)) | 201 | lambda *a: logging.error("avahi error %r", a)) |
106 | 216 | 202 | ||
107 | @@ -220,27 +206,26 @@ | |||
108 | 220 | """Start looking for services. Use two callbacks to handle seeing | 206 | """Start looking for services. Use two callbacks to handle seeing |
109 | 221 | new services and seeing services disappear.""" | 207 | new services and seeing services disappear.""" |
110 | 222 | 208 | ||
112 | 223 | def remove_item_handler(interface, protocol, name, stype, domain, flags): | 209 | def remove_item_handler(cb, interface, protocol, name, stype, domain, flags): |
113 | 224 | """A service disappeared.""" | 210 | """A service disappeared.""" |
114 | 225 | 211 | ||
115 | 226 | if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL: | 212 | if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL: |
116 | 227 | return | 213 | return |
121 | 228 | 214 | cb(name) | |
122 | 229 | del_commport_name_cb(name) | 215 | |
123 | 230 | 216 | def new_item_handler(cb, interface, protocol, name, stype, domain, flags): | |
120 | 231 | def new_item_handler(interface, protocol, name, stype, domain, flags): | ||
124 | 232 | """A service appeared.""" | 217 | """A service appeared.""" |
125 | 233 | 218 | ||
126 | 234 | def handle_error(*args): | 219 | def handle_error(*args): |
127 | 235 | """An error in resolving a new service.""" | 220 | """An error in resolving a new service.""" |
128 | 236 | logging.error("zeroconf ItemNew error for services, %s", args) | 221 | logging.error("zeroconf ItemNew error for services, %s", args) |
129 | 237 | 222 | ||
131 | 238 | def handle_resolved(*args): | 223 | def handle_resolved(cb, *args): |
132 | 239 | """Successfully resolved a new service, which we decode and send | 224 | """Successfully resolved a new service, which we decode and send |
133 | 240 | back to our calling environment with the callback function.""" | 225 | back to our calling environment with the callback function.""" |
134 | 241 | text = avahitext_to_dict(args[9]) | 226 | text = avahitext_to_dict(args[9]) |
135 | 242 | name, host, port = args[2], args[5], args[8] | 227 | name, host, port = args[2], args[5], args[8] |
137 | 243 | add_commport_name_cb(name, text.get("description", "?"), | 228 | cb(name, text.get("description", "?"), |
138 | 244 | host, port, text.get("version", None)) | 229 | host, port, text.get("version", None)) |
139 | 245 | 230 | ||
140 | 246 | if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL: | 231 | if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL: |
141 | @@ -248,8 +233,8 @@ | |||
142 | 248 | 233 | ||
143 | 249 | server.ResolveService(interface, protocol, name, stype, | 234 | server.ResolveService(interface, protocol, name, stype, |
144 | 250 | domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), | 235 | domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), |
147 | 251 | reply_handler=handle_resolved, error_handler=handle_error) | 236 | reply_handler=lambda *a: handle_resolved(cb, *a), |
148 | 252 | 237 | error_handler=handle_error) | |
149 | 253 | 238 | ||
150 | 254 | bus, server = get_dbus_bus_server() | 239 | bus, server = get_dbus_bus_server() |
151 | 255 | domain_name = get_local_hostname()[1] | 240 | domain_name = get_local_hostname()[1] |
152 | @@ -260,7 +245,9 @@ | |||
153 | 260 | 245 | ||
154 | 261 | sbrowser = dbus.Interface(browser_name, | 246 | sbrowser = dbus.Interface(browser_name, |
155 | 262 | avahi.DBUS_INTERFACE_SERVICE_BROWSER) | 247 | avahi.DBUS_INTERFACE_SERVICE_BROWSER) |
158 | 263 | sbrowser.connect_to_signal("ItemNew", new_item_handler) | 248 | sbrowser.connect_to_signal("ItemNew", |
159 | 264 | sbrowser.connect_to_signal("ItemRemove", remove_item_handler) | 249 | lambda *a: new_item_handler(add_commport_name_cb, *a)) |
160 | 250 | sbrowser.connect_to_signal("ItemRemove", | ||
161 | 251 | lambda *a: remove_item_handler(del_commport_name_cb, *a)) | ||
162 | 265 | sbrowser.connect_to_signal("Failure", | 252 | sbrowser.connect_to_signal("Failure", |
163 | 266 | lambda *a: logging.error("avahi error %r", a)) | 253 | lambda *a: logging.error("avahi error %r", a)) |
164 | 267 | 254 | ||
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 | 16 | # | 16 | # |
170 | 17 | # Authors: Chad Miller <chad.miller@canonical.com> | 17 | # Authors: Chad Miller <chad.miller@canonical.com> |
171 | 18 | 18 | ||
172 | 19 | import threading | ||
173 | 20 | import logging | 19 | import logging |
174 | 21 | import logging.handlers | ||
175 | 22 | log = logging.getLogger("replication") | 20 | log = logging.getLogger("replication") |
176 | 23 | 21 | ||
177 | 24 | import dbus.exceptions | 22 | import dbus.exceptions |
178 | 25 | 23 | ||
179 | 26 | import desktopcouch | ||
180 | 27 | from desktopcouch.pair.couchdb_pairing import couchdb_io | 24 | from desktopcouch.pair.couchdb_pairing import couchdb_io |
181 | 28 | from desktopcouch.pair.couchdb_pairing import dbus_io | 25 | from desktopcouch.pair.couchdb_pairing import dbus_io |
182 | 29 | from desktopcouch import replication_services | 26 | from desktopcouch import replication_services |
183 | @@ -52,7 +49,7 @@ | |||
184 | 52 | return getattr(mod, service_name).db_name_prefix | 49 | return getattr(mod, service_name).db_name_prefix |
185 | 53 | except ImportError, e: | 50 | except ImportError, e: |
186 | 54 | log.error("The service %r is unknown. It is not a " | 51 | log.error("The service %r is unknown. It is not a " |
188 | 55 | "module in the %s package ." % (sn, container)) | 52 | "module in the %s package ." % (service_name, container)) |
189 | 56 | return "" | 53 | return "" |
190 | 57 | except Exception, e: | 54 | except Exception, e: |
191 | 58 | log.exception("Not changing remote db name.") | 55 | log.exception("Not changing remote db name.") |
192 | @@ -68,7 +65,7 @@ | |||
193 | 68 | return getattr(mod, service_name).get_oauth_data() | 65 | return getattr(mod, service_name).get_oauth_data() |
194 | 69 | except ImportError, e: | 66 | except ImportError, e: |
195 | 70 | log.error("The service %r is unknown. It is not a " | 67 | log.error("The service %r is unknown. It is not a " |
197 | 71 | "module in the %s package ." % (sn, container)) | 68 | "module in the %s package ." % (service_name, container)) |
198 | 72 | return None | 69 | return None |
199 | 73 | 70 | ||
200 | 74 | def do_all_replication(local_port): | 71 | def do_all_replication(local_port): |
201 | @@ -106,11 +103,12 @@ | |||
202 | 106 | # push caught exception back... | 103 | # push caught exception back... |
203 | 107 | except: | 104 | except: |
204 | 108 | # ... so that we log it here. | 105 | # ... so that we log it here. |
206 | 109 | logging.exception("failed to unpair from other end.") | 106 | logging.exception( |
207 | 107 | "failed to unpair from other end.") | ||
208 | 110 | continue | 108 | continue |
209 | 111 | else: | 109 | else: |
210 | 112 | # Finally, find your inner peace... | 110 | # Finally, find your inner peace... |
212 | 113 | couchdb_io.expunge_pairing(remote_identifier) | 111 | couchdb_io.expunge_pairing(remote_hostid) |
213 | 114 | # ...and move on. | 112 | # ...and move on. |
214 | 115 | continue | 113 | continue |
215 | 116 | 114 | ||
216 | @@ -184,10 +182,12 @@ | |||
217 | 184 | log.error("skipping %r on %s. %s", db_name, sn, e) | 182 | log.error("skipping %r on %s. %s", db_name, sn, e) |
218 | 185 | continue | 183 | continue |
219 | 186 | 184 | ||
221 | 187 | db_name = remote_db_name[1+len(str(remote_db_name_prefix)):] | 185 | prefix_len = len(str(remote_db_name_prefix)) |
222 | 186 | db_name = remote_db_name[1+prefix_len:] | ||
223 | 188 | if db_name.strip("/") == "management": | 187 | if db_name.strip("/") == "management": |
224 | 189 | continue # be paranoid about what we accept. | 188 | continue # be paranoid about what we accept. |
226 | 190 | log.debug("want to replipull %r from static host %r @ %s", | 189 | log.debug( |
227 | 190 | "want to replipull %r from static host %r @ %s", | ||
228 | 191 | db_name, remote_hostid, addr) | 191 | db_name, remote_hostid, addr) |
229 | 192 | couchdb_io.replicate(remote_db_name, db_name, | 192 | couchdb_io.replicate(remote_db_name, db_name, |
230 | 193 | source_host=addr, source_port=port, | 193 | source_host=addr, source_port=port, |
231 | @@ -224,8 +224,6 @@ | |||
232 | 224 | "our zeroconf advert. %s", e) | 224 | "our zeroconf advert. %s", e) |
233 | 225 | return None | 225 | return None |
234 | 226 | 226 | ||
235 | 227 | dbus_io.discover_services(None, None, True) | ||
236 | 228 | |||
237 | 229 | dbus_io.maintain_discovered_servers() | 227 | dbus_io.maintain_discovered_servers() |
238 | 230 | 228 | ||
239 | 231 | t = task.LoopingCall(replicate_local_databases_to_paired_hosts, port) | 229 | t = task.LoopingCall(replicate_local_databases_to_paired_hosts, port) |
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):
return ""
this has an undefined variable 'remote_identifier'
and there are several long lines.
dbus_io
args is undefined in:
also, this scares me:
class LocationAdverti sement( Advertisement) :
kwargs. pop(stype)
super( LocationAdverti sement, self).__init__(
stype= location_ discovery_ service_ type, *args, **kwargs)
"""An advertised couchdb location. See Advertisement class."""
def __init__(self, *args, **kwargs):
if "stype" in kwargs:
class PairAdvertiseme nt(Advertisemen t):
kwargs. pop(stype)
super( PairAdvertiseme nt, self).__init__(
stype= invitations_ discovery_ service_ type, *args, **kwargs)
"""An advertised couchdb pairing opportunity. See Advertisement class."""
def __init__(self, *args, **kwargs):
if "stype" in kwargs:
Isn't it possible to make it a little more explicit? I think I would prefer:
kwargs. pop(kwargs[ "stype" ])