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

[Chad Miller]

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.

[Eric Casteleijn (thisfred)]

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

[lp:~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear]

80. By Chad Miller on 2009-10-01

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

[Eric Casteleijn (thisfred)]

Looks great, tests pass!

review: Approve

[lp:~cmiller/desktopcouch/dbus-refs-explicit-and-rm-resolve-on-disappear]

81. By Chad Miller on 2009-10-01

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

=== modified file 'desktopcouch/pair/couchdb_pairing/dbus_io.py'
--- desktopcouch/pair/couchdb_pairing/dbus_io.py	2009-09-30 15:52:57 +0000
+++ desktopcouch/pair/couchdb_pairing/dbus_io.py	2009-10-01 21:11:09 +0000
@@ -103,18 +103,14 @@
 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)
+        kwargs["stype"] = location_discovery_service_type
+        super(LocationAdvertisement, self).__init__(*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)
+        kwargs["stype"] = invitations_discovery_service_type
+        super(PairAdvertisement, self).__init__(*args, **kwargs)
 
 def avahitext_to_dict(avahitext):
     text = {}
@@ -155,51 +151,39 @@
 def maintain_discovered_servers(add_cb=cb_found_desktopcouch_server, 
         del_cb=cb_lost_desktopcouch_server):
 
-    def remove_item_handler(interface, protocol, name, stype, domain, flags):
+    def remove_item_handler(cb, interface, protocol, name, stype, domain,
+            flags):
         """A service disappeared."""
 
-        def handle_error(*args):
-            """An error in resolving a new service."""
-            logging.error("zeroconf ItemNew error for services, %s", args)
-
-        def handle_resolved(*args):
-            """Successfully resolved a new service, which we decode and send
-            back to our calling environment with the callback function."""
-
-            name, host, port = args[2], args[5], args[8]
-            if name.startswith("desktopcouch "):
-                hostid = name[13:]
-                logging.debug("lost sight of %r", hostid)
-                del_cb(hostid)
-            else:
-                logging.error("no UUID in zeroconf message,  %r", args)
-        
-        server.ResolveService(interface, protocol, name, stype, 
-            domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), 
-            reply_handler=handle_resolved, error_handler=handle_error)
-
-    def new_item_handler(interface, protocol, name, stype, domain, flags):
+        if name.startswith("desktopcouch "):
+            hostid = name[13:]
+            logging.debug("lost sight of %r", hostid)
+            cb(hostid)
+        else:
+            logging.error("annc doesn't look like one of ours.  %r", name)
+
+    def new_item_handler(cb, interface, protocol, name, stype, domain, flags):
         """A service appeared."""
 
         def handle_error(*args):
             """An error in resolving a new service."""
             logging.error("zeroconf ItemNew error for services, %s", args)
 
-        def handle_resolved(*args):
+        def handle_resolved(cb, *args):
             """Successfully resolved a new service, which we decode and send
             back to our calling environment with the callback function."""
 
             name, host, port = args[2], args[5], args[8]
-            # FIXME strip off "desktopcouch "
             if name.startswith("desktopcouch "):
-                add_cb(name[13:], host, port)
+                cb(name[13:], host, port)
             else:
-                logging.error("no UUID in zeroconf message,  %r", name)
+                logging.error("annc doesn't look like one of ours.  %r", name)
             return True
 
         server.ResolveService(interface, protocol, name, stype, 
             domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), 
-            reply_handler=handle_resolved, error_handler=handle_error)
+            reply_handler=lambda *a: handle_resolved(cb, *a),
+            error_handler=handle_error)
 
     bus, server = get_dbus_bus_server()
     domain_name = get_local_hostname()[1]
@@ -209,8 +193,10 @@
 
     sbrowser = dbus.Interface(browser_name,
             avahi.DBUS_INTERFACE_SERVICE_BROWSER)
-    sbrowser.connect_to_signal("ItemNew", new_item_handler)
-    sbrowser.connect_to_signal("ItemRemove", remove_item_handler)
+    sbrowser.connect_to_signal("ItemNew",
+            lambda *a: new_item_handler(add_cb, *a))
+    sbrowser.connect_to_signal("ItemRemove",
+            lambda *a: remove_item_handler(del_cb, *a))
     sbrowser.connect_to_signal("Failure", 
             lambda *a: logging.error("avahi error %r", a))
 
@@ -220,27 +206,26 @@
     """Start looking for services.  Use two callbacks to handle seeing
     new services and seeing services disappear."""
 
-    def remove_item_handler(interface, protocol, name, stype, domain, flags):
+    def remove_item_handler(cb, interface, protocol, name, stype, domain, flags):
         """A service disappeared."""
 
         if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL:
             return
-
-        del_commport_name_cb(name)
-
-    def new_item_handler(interface, protocol, name, stype, domain, flags):
+        cb(name)
+
+    def new_item_handler(cb, interface, protocol, name, stype, domain, flags):
         """A service appeared."""
 
         def handle_error(*args):
             """An error in resolving a new service."""
             logging.error("zeroconf ItemNew error for services, %s", args)
 
-        def handle_resolved(*args):
+        def handle_resolved(cb, *args):
             """Successfully resolved a new service, which we decode and send
             back to our calling environment with the callback function."""
             text = avahitext_to_dict(args[9])
             name, host, port = args[2], args[5], args[8]
-            add_commport_name_cb(name, text.get("description", "?"),
+            cb(name, text.get("description", "?"),
                     host, port, text.get("version", None))
 
         if not show_local and flags & avahi.LOOKUP_RESULT_LOCAL:
@@ -248,8 +233,8 @@
 
         server.ResolveService(interface, protocol, name, stype, 
             domain, avahi.PROTO_UNSPEC, dbus.UInt32(0), 
-            reply_handler=handle_resolved, error_handler=handle_error)
-
+            reply_handler=lambda *a: handle_resolved(cb, *a), 
+            error_handler=handle_error)
 
     bus, server = get_dbus_bus_server()
     domain_name = get_local_hostname()[1]
@@ -260,7 +245,9 @@
 
     sbrowser = dbus.Interface(browser_name,
             avahi.DBUS_INTERFACE_SERVICE_BROWSER)
-    sbrowser.connect_to_signal("ItemNew", new_item_handler)
-    sbrowser.connect_to_signal("ItemRemove", remove_item_handler)
+    sbrowser.connect_to_signal("ItemNew", 
+            lambda *a: new_item_handler(add_commport_name_cb, *a))
+    sbrowser.connect_to_signal("ItemRemove",
+            lambda *a: remove_item_handler(del_commport_name_cb, *a))
     sbrowser.connect_to_signal("Failure",
             lambda *a: logging.error("avahi error %r", a))

=== modified file 'desktopcouch/replication.py'
--- desktopcouch/replication.py	2009-09-30 19:30:37 +0000
+++ desktopcouch/replication.py	2009-10-01 21:11:09 +0000
@@ -16,14 +16,11 @@
 #
 # Authors: Chad Miller <[e-mail]>
 
-import threading
 import logging
-import logging.handlers
 log = logging.getLogger("replication")
 
 import dbus.exceptions
 
-import desktopcouch
 from desktopcouch.pair.couchdb_pairing import couchdb_io
 from desktopcouch.pair.couchdb_pairing import dbus_io
 from desktopcouch import replication_services
@@ -52,7 +49,7 @@
         return getattr(mod, service_name).db_name_prefix
     except ImportError, e:
         log.error("The service %r is unknown.  It is not a "
-                "module in the %s package ." % (sn, container))
+                "module in the %s package ." % (service_name, container))
         return ""
     except Exception, e:
         log.exception("Not changing remote db name.")
@@ -68,7 +65,7 @@
         return getattr(mod, service_name).get_oauth_data()
     except ImportError, e:
         log.error("The service %r is unknown.  It is not a "
-                "module in the %s package ." % (sn, container))
+                "module in the %s package ." % (service_name, container))
         return None
 
 def do_all_replication(local_port):
@@ -106,11 +103,12 @@
                                 # push caught exception back...
                             except:
                                 # ... so that we log it here.
-                                logging.exception("failed to unpair from other end.")
+                                logging.exception(
+                                        "failed to unpair from other end.")
                                 continue
                     else:
                         # Finally, find your inner peace...
-                        couchdb_io.expunge_pairing(remote_identifier)
+                        couchdb_io.expunge_pairing(remote_hostid)
                     # ...and move on.
                     continue
 
@@ -184,10 +182,12 @@
                             log.error("skipping %r on %s.  %s", db_name, sn, e)
                             continue
 
-                        db_name = remote_db_name[1+len(str(remote_db_name_prefix)):]
+                        prefix_len = len(str(remote_db_name_prefix))
+                        db_name = remote_db_name[1+prefix_len:]
                         if db_name.strip("/") == "management":
                             continue  # be paranoid about what we accept.
-                        log.debug("want to replipull %r from static host %r @ %s",
+                        log.debug(
+                                "want to replipull %r from static host %r @ %s",
                                 db_name, remote_hostid, addr)
                         couchdb_io.replicate(remote_db_name, db_name,
                                 source_host=addr, source_port=port, 
@@ -224,8 +224,6 @@
                     "our zeroconf advert.  %s", e)
             return None
 
-    dbus_io.discover_services(None, None, True)
-
     dbus_io.maintain_discovered_servers()
 
     t = task.LoopingCall(replicate_local_databases_to_paired_hosts, port)