Rev 4692: Respect items() protocol for registry objects. in file:///home/vila/src/bzr/bugs/430510-transport-registry-items/

[Vincent Ladeuil]

At file:///home/vila/src/bzr/bugs/430510-transport-registry-items/

------------------------------------------------------------
revno: 4692
revision-id: v.ladeuil+lp at free.fr-20090916103729-m6h1pyvjprm6puvm
parent: pqm at pqm.ubuntu.com-20090916025214-6cyz9179xs7f1w70
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 430510-transport-registry-items
timestamp: Wed 2009-09-16 12:37:29 +0200
message:
  Respect items() protocol for registry objects.
  
  * bzrlib/tests/test_registry.py:
  (TestRegistryIter): Test some corner cases where object are
  registered while the registry is iterated.
  
  * bzrlib/registry.py:
  (Registry): iteritems() and items() have different intents, don't
  mix them under the covers or devs get tricked (see bug #277048
  which seemed to have fixed the issue).
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS	2009-09-16 02:52:14 +0000
+++ b/NEWS	2009-09-16 10:37:29 +0000
@@ -70,6 +70,9 @@
   chk_bytes root records.
   (Andrew Bennetts, #423506)
 
+* Registry objects should not use iteritems() when asked to use items().
+  (Vincent Ladeuil, #430510)
+
 * Make sure that we unlock the tree if we fail to create a TreeTransform
   object when doing a merge, and there is limbo, or pending-deletions
   directory.  (Gary van der Merwe, #427773)

=== 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):