Rev 4696: (vila) registry.items() stop using iteritems() in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

[Canonical.com Patch Queue Manager]

At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 4696 [merge]
revision-id: pqm at pqm.ubuntu.com-20090917080942-rhh806k1mgcmj3p1
parent: pqm at pqm.ubuntu.com-20090916123002-46bfl24pa898h8cq
parent: v.ladeuil+lp at free.fr-20090917073122-h70lcv6ttt0q5bfv
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Thu 2009-09-17 09:09:42 +0100
message:
  (vila) registry.items() stop using iteritems()
modified:
  NEWS                           NEWS-20050323055033-4e00b5db738777ff
  bzrlib/registry.py             lazy_factory.py-20060809213415-2gfvqadtvdn0phtg-1
  bzrlib/tests/test_registry.py  test_lazy_factory.py-20060809213415-2gfvqadtvdn0phtg-2
=== modified file 'NEWS'
--- a/NEWS	2009-09-16 10:29:18 +0000
+++ b/NEWS	2009-09-17 07:31:22 +0000
@@ -74,6 +74,9 @@
   chk_bytes root records.
   (Andrew Bennetts, #423506)
 
+* Registry objects should not use iteritems() when asked to use items().
+  (Vincent Ladeuil, #430510)
+
 Improvements
 ************
 

=== modified file 'bzrlib/registry.py'
--- a/bzrlib/registry.py	2009-03-23 14:59:43 +0000
+++ b/bzrlib/registry.py	2009-09-16 10:37:29 +0000
@@ -222,7 +222,10 @@
             yield key, getter.get_obj()
 
     def items(self):
-        return sorted(self.iteritems())
+        # We should not use the iteritems() implementation below (see bug
+        # #430510)
+        return sorted([(key, getter.get_obj())
+                       for key, getter in self._dict.items()])
 
     def _set_default_key(self, key):
         if not self._dict.has_key(key):

=== modified file 'bzrlib/tests/test_registry.py'
--- a/bzrlib/tests/test_registry.py	2009-03-23 14:59:43 +0000
+++ b/bzrlib/tests/test_registry.py	2009-09-16 10:37:29 +0000
@@ -21,13 +21,13 @@
 
 from bzrlib import (
     errors,
+    osutils,
     registry,
-    osutils,
+    tests,
     )
-from bzrlib.tests import TestCase, TestCaseInTempDir
-
-
-class TestRegistry(TestCase):
+
+
+class TestRegistry(tests.TestCase):
 
     def register_stuff(self, a_registry):
         a_registry.register('one', 1)
@@ -202,7 +202,64 @@
         self.assertIs(sftp_object, found_object)
 
 
-class TestRegistryWithDirs(TestCaseInTempDir):
+class TestRegistryIter(tests.TestCase):
+    """Test registry iteration behaviors.
+
+    There are dark corner cases here when the registered objects trigger
+    addition in the iterated registry.
+    """
+
+    def setUp(self):
+        super(TestRegistryIter, self).setUp()
+
+        # We create a registry with "official" objects and "hidden"
+        # objects. The later represent the side effects that led to bug #277048
+        # and #430510
+        self.registry =  registry.Registry()
+
+        def register_more():
+            self.registry.register('hidden', None)
+
+        self.registry.register('passive', None)
+        self.registry.register('active', register_more)
+        self.registry.register('passive-too', None)
+
+        class InvasiveGetter(registry._ObjectGetter):
+
+            def get_obj(inner_self):
+                # Surprise ! Getting a registered object (think lazy loaded
+                # module) register yet another object !
+                self.registry.register('more hidden', None)
+                return inner_self._obj
+
+        self.registry.register('hacky', None)
+        # We peek under the covers because the alternative is to use lazy
+        # registration and create a module that can reference our test registry
+        # it's too much work for such a corner case -- vila 090916
+        self.registry._dict['hacky'] = InvasiveGetter(None)
+
+    def _iter_them(self, iter_func_name):
+        iter_func = getattr(self.registry, iter_func_name, None)
+        self.assertIsNot(None, iter_func)
+        count = 0
+        for name, func in iter_func():
+            count += 1
+            self.assertFalse(name in ('hidden', 'more hidden'))
+            if func is not None:
+                # Using an object register another one as a side effect
+                func()
+        self.assertEqual(4, count)
+
+    def test_iteritems(self):
+        # the dict is modified during the iteration
+        self.assertRaises(RuntimeError, self._iter_them, 'iteritems')
+
+    def test_items(self):
+        # we should be able to iterate even if one item modify the dict
+        self._iter_them('items')
+
+
+class TestRegistryWithDirs(tests.TestCaseInTempDir):
     """Registry tests that require temporary dirs"""
 
     def create_plugin_file(self, contents):