Test for re-adding multiple status receivers, and fix.

This catches a regression - looping over self, while removing things from self, results in undefined behavior.
buildbot · Nov 18, 2011 · aa3a86a · aa3a86a
1 parent 31b65a0
commit aa3a86a
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 8 deletions.
diff --git a/master/buildbot/process/users/manager.py b/master/buildbot/process/users/manager.py
@@ -31,7 +31,7 @@ def reconfigService(self, new_config):
         # this is easy - kick out all of the old managers, and add the
         # new ones.
 
-        for mgr in self:
+        for mgr in list(self):
             wfd = defer.waitForDeferred(
                 defer.maybeDeferred(lambda :
                     mgr.disownServiceParent()))

diff --git a/master/buildbot/status/master.py b/master/buildbot/status/master.py
@@ -65,7 +65,7 @@ def startService(self):
     @defer.deferredGenerator
     def reconfigService(self, new_config):
         # remove the old listeners, then add the new
-        for sr in self:
+        for sr in list(self):
             wfd = defer.waitForDeferred(
                 defer.maybeDeferred(lambda :
                     sr.disownServiceParent()))

diff --git a/master/buildbot/test/unit/test_status_master.py b/master/buildbot/test/unit/test_status_master.py
@@ -56,17 +56,44 @@ def test_reconfigService(self):
 
         config = mock.Mock()
 
-        # add a user manager
-        sr = FakeStatusReceiver()
-        config.status = [ sr ]
+        # add a status reciever
+        sr0 = FakeStatusReceiver()
+        config.status = [ sr0 ]
 
         wfd = defer.waitForDeferred(
                 status.reconfigService(config))
         yield wfd
         wfd.getResult()
 
-        self.assertTrue(sr.running)
-        self.assertIdentical(sr.master, m)
+        self.assertTrue(sr0.running)
+        self.assertIdentical(sr0.master, m)
+
+        # add a status reciever
+        sr1 = FakeStatusReceiver()
+        sr2 = FakeStatusReceiver()
+        config.status = [ sr1, sr2 ]
+
+        wfd = defer.waitForDeferred(
+                status.reconfigService(config))
+        yield wfd
+        wfd.getResult()
+
+        self.assertFalse(sr0.running)
+        self.assertIdentical(sr0.master, None)
+        self.assertTrue(sr1.running)
+        self.assertIdentical(sr1.master, m)
+        self.assertTrue(sr2.running)
+        self.assertIdentical(sr2.master, m)
+
+        # reconfig with those two (a regression check)
+        sr1 = FakeStatusReceiver()
+        sr2 = FakeStatusReceiver()
+        config.status = [ sr1, sr2 ]
+
+        wfd = defer.waitForDeferred(
+                status.reconfigService(config))
+        yield wfd
+        wfd.getResult()
 
         # and back to nothing
         config.status = [ ]
@@ -75,4 +102,6 @@ def test_reconfigService(self):
         yield wfd
         wfd.getResult()
 
-        self.assertIdentical(sr.master, None)
+        self.assertIdentical(sr0.master, None)
+        self.assertIdentical(sr1.master, None)
+        self.assertIdentical(sr2.master, None)