Merge lp:~isoschiz/endroid/joincleanup into lp:endroid

[Martin Morrison]

Clean up the room join handling to not leak errbacks off the end of the deferred chains in the case of failure.

Clean up the room join handling to not leak errbacks off the end of the deferred chains in the case of failure.

[Phil Connell (pconnell)]

At the top of _retry(), attempts -= 1 is surely a UnboundLocalError?

Why is it useful to return the room name?

review: Needs Fixing

[Martin Morrison (isoschiz)]

On 08/05/14 16:10, Phil Connell wrote:
> Review: Needs Fixing
>
> At the top of _retry(), attempts -= 1 is surely a UnboundLocalError?

It's an argument to the _retry method, so no. Unless I am missing
something? I have tested this too, and it appears to work. :-)

>
> Why is it useful to return the room name?

It probably isn't. Previously it was returning a string that should have
been logged but wasn't. The room name seemed as good a thing as any to
fire the deferred with. Got a better suggestion?

[ChrisD (gingerchris)]

Leading underscores on closures seems unnecessary, but meh

review: Approve

=== modified file 'src/endroid/usermanagement.py'
--- src/endroid/usermanagement.py	2013-10-01 18:16:05 +0000
+++ src/endroid/usermanagement.py	2014-05-08 15:18:21 +0000
@@ -12,6 +12,8 @@
 from random import choice
 from collections import namedtuple
 
+from twisted.internet import defer
+
 # we use ADJECTIVES to generate a random new nick if endroid's is taken
 ADJECTIVES = [
     "affronted", "annoyed", "antagonized", "bitter", "chafed", "convulsed",
@@ -491,38 +493,49 @@
 
     ### Room functions
 
-    # @@@ at the moment this assumes that if we failed to join a room it was because
-    # our nickname was taken - no provision for wrong passwords (should never be an
-    # issue) or other failures
-    def join_room(self, name, attempts=0):
+    def join_room(self, name, nick=None, max_attempts=0):
         """
-        Attempt to join the room 'name'. On failure will try again with
-        different nicknames until attempts == join_attempts.
+        Attempt to join the room 'name'.
+
+        Retries the join up to 'max_attempts' times. If 'max_attempts' is 0,
+        uses the configured value of 'join_attempts'.
+
+        Returns a Deferred that fires with the room name on success, or the
+        latest reason for failure if all attempts are exhausted.
 
         """
         if not name in self.room_rosters:
-            return "Failed to join room {}: not allowed".format(name)
-
-        def retry(failure, room, nick, attempts):
-            if attempts < self.join_attempts:
-                return self.join_room(room, attempts+1)
-            else:
-                return "Failed to join room {} as {}".format(room, nick)
-        def joined(_, room, nick, attempts):
-            if attempts:
-                self.kick(room, nick, "You've taken EnDroid's nick, "
-                          "please reconnect with another nick")
-                self.wh.nick(JID(room), nick)
-            return "Joined room {} as {}".format(room, nick)
-
+            return defer.fail(ValueError("{} not in configured rooms"
+                                         .format(name)))
+
+        if max_attempts == 0:
+            max_attempts = self.join_attempts
         room = self.room_rosters[name]
-        nick = room.nick
-        if attempts:  # someone has stolen our nickname - generate a new one
-            nick += " ({} and {})".format(choice(ADJECTIVES), choice(ADJECTIVES))
+        if nick is None:
+            nick = room.nick
+
+        def _retry(failure, room, nick, attempts):
+            attempts -= 1
+            if attempts > 0:
+                nick = room.nick + " ({} and {})".format(choice(ADJECTIVES),
+                                                         choice(ADJECTIVES))
+                return self.join_room(room.name, nick, max_attempts=attempts)
+            else:
+                return failure
+        @defer.inlineCallbacks
+        def _joined(_, room, nick, attempts):
+            if nick != room.nick:
+                yield self.kick(room.name, room.nick, "You've taken EnDroid's "
+                                "nick, please reconnect with another nick")
+                self.wh.nick(JID(room.name), room.nick)
+            defer.returnValue(room.name)
 
         d = self.wh.join(JID(name), nick, password=room.password)
-        d.addErrback(retry, name, room.nick, attempts)
-        d.addCallback(joined, name, room.nick, attempts)
+        # Use addCallbacks here so only one _joined is called in the event of
+        # potentially multiple retries
+        d.addCallbacks(_joined, _retry,
+                       callbackArgs=(room, nick, max_attempts),
+                       errbackArgs=(room, nick, max_attempts))
         
         return d
 
@@ -540,6 +553,9 @@
         def failure(_):
             logging.error("Failed to kick {} from {} ({})".format(
                           nick, room, reason))
+            #@@@ This swallows errors. Seems expected at the moment, but future
+            # callers may want to see the error. Make sure to update all
+            # current callers if this is ever changed.
 
         return self.wh.kick(JID(room), nick, reason).addCallbacks(success, failure)
 
@@ -567,9 +583,13 @@
         # Currently called from elsewhere
 
     def join_all_rooms(self):
+        def _fail(fail, room):
+            logging.error("Failed to join room ({}): {}"
+                          .format(room, str(fail.value)))
         for room in self.get_rooms():
             d = self.join_room(room)
             d.addCallback(lambda _, r: self.joined_room(r), room)
+            d.addErrback(_fail, room)
 
     # @@@ This function is in the wrong place. This is the room section
     def join_all_groups(self):