Merge lp:~gz/bzr/lazy_import_threadsafe_396819 into lp:bzr

[Martin Packman]

Make lazy imports proxy by default so that concurrent resolution is robust

Switch the default behaviour of lazy imports to do object proxying rather than raise an exception, which is the simplest and safest way of ensuring concurrent usage doesn't break. What this loses us is some detection of badly written code where a proxied object is aliased in more than one location, so copies persist even after the original is replaced by the resolved object.

This is a version of Martin von Gagern's branch, with some minor cleanups, and the complication of tracking the last replacement removed. This loses part of the compensation for changing the error detection, which was to flip proxying back to disallowed for selftest. Now imports that happen before that point will not be flagged. This is a minor additional loss, as selftest is already very limited in how it can track lazy imports, given it will only do one path and running other commands may take another, with different import order and behaviours.

The proposal doesn't mark without prerequisite set as the diff against trunk is quite different, but the changes against MvG's branch are also of interest:

<https://code.launchpad.net/~gagern/bzr/bug396819-lazy_import-threadsafe/+merge/73475>

There's a lot of prior discussion, but the short version is that without locks (which we don't want, and are dangerous when combined with python imports), the only way to support concurrent resolution of lazy objects is to accept that sometimes the factory will be called more than once, and not to treat that as an error. Detecting coding mistakes has to be left to other mechanisms than always raising an exception when a lazy object is used multiple times.

[Martin von Gagern (gagern)]

Had a look at the individual changesets; they all look good to me.

Why ScopeReplacer._should_proxy instead of object.__getattribute__(self, '_should_proxy')? The latter would allow overriding, although we don't need it yet. Performance shouldn't be an issue here, as this will be called rarely. Is it for the sake of readability?

Diff below shows merge problems, so you should probably pull from trunk and merge manually.

I'm setting review to Approve in LP, although I'm no core dev. Hope you won't consider this presumptuous of me.

review: Approve

[Martin Packman (gz)]

> Had a look at the individual changesets; they all look good to me.

Thank you for going over this Martin!

> Why ScopeReplacer._should_proxy instead of object.__getattribute__(self,
> '_should_proxy')? The latter would allow overriding, although we don't need it
> yet. Performance shouldn't be an issue here, as this will be called rarely. Is
> it for the sake of readability?

Simply because it's now not overridden in a subclass, but is changed on the class object itself, so I think this is a clearer spelling.

> Diff below shows merge problems, so you should probably pull from trunk and
> merge manually.

Thanks, will do, need to remember to add release notes too.

> I'm setting review to Approve in LP, although I'm no core dev. Hope you won't
> consider this presumptuous of me.

That's entirely appropriate, feel free to review any other merge proposals in the same manner as well. :)

[Vincent Ladeuil (vila)]

@gagern: You're definitely welcome to approve any proposal ! And in this specific case, you're even more welcome !

The approval rules is that 2 votes are required to land, core devs getting an implicit approval when submitting. Here, your vote gives mgz a confirmation that you agree with his modifications to your former work and this definitely has weight.

> Switch the default behaviour of lazy imports to do object proxying rather
  than raise an exception, which is the simplest and safest way of ensuring
  concurrent usage doesn't break. What this loses us is some detection of
  badly written code where a proxied object is aliased in more than one
  location, so copies persist even after the original is replaced by the
  resolved object.

Right, this is especially true if symbols (instead of modules) are lazily
imported, but this has been a hot topic for a long time and the only new
material I can add to the discussion is from
http://docs.python.org/howto/doanddont.html:

,----
| from module import name1, name2
|
| This is a “don’t” which is much weaker than the previous “don’t”s but is
| still something you should not do if you don’t have good reasons to do
| that. The reason it is usually a bad idea is because you suddenly have an
| object which lives in two separate namespaces. When the binding in one
| namespace changes, the binding in the other will not, so there will be a
| discrepancy between them. This happens when, for example, one module is
| reloaded, or changes the definition of a function at runtime.
`----

Two separate namespaces indeed which bite us anyway when monkey-patching.

So, you're not making things worse here and in fact, I think allowing
multiple replacements is just the right thing to do. I can't remember a case
where the expections raised really revealed a bug. I *can* remember multiple
cases where the fix was to *not* import lazily instead which is hardly a
win.

Anyway, under the hood, the module is still read from disk only once IIUC so
the lazy loading is achieved in any case.

380 + in order win the race, setting up the original caller to lose.

in order *to* ?

Other than that, if the tests are still passing, this seems clearer than
what we had.

review: Approve

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-12-30 14:36:46 +0000
+++ bzrlib/builtins.py	2012-01-05 10:16:30 +0000
@@ -22,8 +22,8 @@
 
 import bzrlib.bzrdir
 
-from bzrlib.lazy_import import lazy_import
-lazy_import(globals(), """
+from bzrlib import lazy_import
+lazy_import.lazy_import(globals(), """
 import cStringIO
 import errno
 import sys
@@ -4004,6 +4004,15 @@
             load_list=None, debugflag=None, starting_with=None, subunit=False,
             parallel=None, lsprof_tests=False,
             sync=False):
+
+        # During selftest, disallow proxying, as it can cause severe
+        # performance penalties and is only needed for thread
+        # safety. The selftest command is assumed to not use threads
+        # too heavily. The call should be as early as possible, as
+        # error reporting for past duplicate imports won't have useful
+        # backtraces.
+        lazy_import.disallow_proxying()
+
         from bzrlib import tests
 
         if testspecs_list is not None:

=== modified file 'bzrlib/lazy_import.py'
--- bzrlib/lazy_import.py	2011-12-19 14:40:24 +0000
+++ bzrlib/lazy_import.py	2012-01-05 10:16:30 +0000
@@ -53,10 +53,11 @@
 
     __slots__ = ('_scope', '_factory', '_name', '_real_obj')
 
-    # Setting this to True will allow you to do x = y, and still access members
-    # from both variables. This should not normally be enabled, but is useful
-    # when building documentation.
-    _should_proxy = False
+    # If you to do x = y, setting this to False will disallow access to
+    # members from the second variable (i.e. x). This should normally
+    # be enabled for reasons of thread safety and documentation, but
+    # will be disabled during the selftest command to check for abuse.
+    _should_proxy = True
 
     def __init__(self, scope, factory, name):
         """Create a temporary object in the specified scope.
@@ -73,75 +74,61 @@
         object.__setattr__(self, '_real_obj', None)
         scope[name] = self
 
-    def _replace(self):
-        """Actually replace self with other in the given scope"""
+    def _resolve(self):
+        """Return the real object for which this is a placeholder"""
         name = object.__getattribute__(self, '_name')
-        try:
+        real_obj = object.__getattribute__(self, '_real_obj')
+        if real_obj is None:
+            # No obj generated previously, so generate from factory and scope.
             factory = object.__getattribute__(self, '_factory')
             scope = object.__getattribute__(self, '_scope')
-        except AttributeError, e:
-            # Because ScopeReplacer objects only replace a single
-            # item, passing them to another variable before they are
-            # replaced would cause them to keep getting replaced
-            # (only they are replacing the wrong variable). So we
-            # make it forbidden, and try to give a good error.
+            obj = factory(self, scope, name)
+            if obj is self:
+                raise errors.IllegalUseOfScopeReplacer(name, msg="Object tried"
+                    " to replace itself, check it's not using its own scope.")
+
+            # Check if another thread has jumped in while obj was generated.
+            real_obj = object.__getattribute__(self, '_real_obj')
+            if real_obj is None:
+                # Still no prexisting obj, so go ahead and assign to scope and
+                # return. There is still a small window here where races will
+                # not be detected, but safest to avoid additional locking.
+                object.__setattr__(self, '_real_obj', obj)
+                scope[name] = obj
+                return obj
+
+        # Raise if proxying is disabled as obj has already been generated.
+        if not ScopeReplacer._should_proxy:
             raise errors.IllegalUseOfScopeReplacer(
-                name, msg="Object already cleaned up, did you assign it"
-                          " to another variable?",
-                extra=e)
-        obj = factory(self, scope, name)
-        if obj is self:
-            raise errors.IllegalUseOfScopeReplacer(name, msg="Object tried"
-                " to replace itself, check it's not using its own scope.")
-        if ScopeReplacer._should_proxy:
-            object.__setattr__(self, '_real_obj', obj)
-        scope[name] = obj
-        return obj
-
-    def _cleanup(self):
-        """Stop holding on to all the extra stuff"""
-        try:
-            del self._factory
-        except AttributeError:
-            # Oops, we just lost a race with another caller of _cleanup.  Just
-            # ignore it.
-            pass
-
-        try:
-            del self._scope
-        except AttributeError:
-            # Another race loss.  See above.
-            pass
-
-        # We keep _name, so that we can report errors
-        # del self._name
+                name, msg="Object already replaced, did you assign it"
+                          " to another variable?")
+        return real_obj
 
     def __getattribute__(self, attr):
-        obj = object.__getattribute__(self, '_real_obj')
-        if obj is None:
-            _replace = object.__getattribute__(self, '_replace')
-            obj = _replace()
-            _cleanup = object.__getattribute__(self, '_cleanup')
-            _cleanup()
+        obj = object.__getattribute__(self, '_resolve')()
         return getattr(obj, attr)
 
     def __setattr__(self, attr, value):
-        obj = object.__getattribute__(self, '_real_obj')
-        if obj is None:
-            _replace = object.__getattribute__(self, '_replace')
-            obj = _replace()
-            _cleanup = object.__getattribute__(self, '_cleanup')
-            _cleanup()
+        obj = object.__getattribute__(self, '_resolve')()
         return setattr(obj, attr, value)
 
     def __call__(self, *args, **kwargs):
-        _replace = object.__getattribute__(self, '_replace')
-        obj = _replace()
-        _cleanup = object.__getattribute__(self, '_cleanup')
-        _cleanup()
+        obj = object.__getattribute__(self, '_resolve')()
         return obj(*args, **kwargs)
 
 
+def disallow_proxying():
+    """Disallow lazily imported modules to be used as proxies.
+
+    Calling this function might cause problems with concurrent imports
+    in multithreaded environments, but will help detecting wasteful
+    indirection, so it should be called when executing unit tests.
+
+    Only lazy imports that happen after this call are affected.
+    """
+    ScopeReplacer._should_proxy = False
+
+
 class ImportReplacer(ScopeReplacer):
     """This is designed to replace only a portion of an import list.
 

=== modified file 'bzrlib/tests/test_lazy_import.py'
--- bzrlib/tests/test_lazy_import.py	2011-12-19 12:14:20 +0000
+++ bzrlib/tests/test_lazy_import.py	2012-01-05 10:16:30 +0000
@@ -39,10 +39,6 @@
     def use_actions(actions):
         InstrumentedReplacer.actions = actions
 
-    def _replace(self):
-        InstrumentedReplacer.actions.append('_replace')
-        return lazy_import.ScopeReplacer._replace(self)
-
     def __getattribute__(self, attr):
         InstrumentedReplacer.actions.append(('__getattribute__', attr))
         return lazy_import.ScopeReplacer.__getattribute__(self, attr)
@@ -62,10 +58,6 @@
         InstrumentedImportReplacer.actions.append(('_import', name))
         return lazy_import.ImportReplacer._import(self, scope, name)
 
-    def _replace(self):
-        InstrumentedImportReplacer.actions.append('_replace')
-        return lazy_import.ScopeReplacer._replace(self)
-
     def __getattribute__(self, attr):
         InstrumentedImportReplacer.actions.append(('__getattribute__', attr))
         return lazy_import.ScopeReplacer.__getattribute__(self, attr)
@@ -144,7 +136,6 @@
         self.assertIsInstance(test_obj1, TestClass)
         self.assertEqual('foo', test_obj1.foo(2))
         self.assertEqual([('__getattribute__', 'foo'),
-                          '_replace',
                           'factory',
                           'init',
                           ('foo', 1),
@@ -243,7 +234,6 @@
         self.assertEqual('class_member', test_class1.class_member)
         self.assertEqual(test_class1, TestClass)
         self.assertEqual([('__getattribute__', 'class_member'),
-                          '_replace',
                           'factory',
                          ], actions)
 
@@ -273,7 +263,6 @@
         self.assertIsInstance(obj, TestClass)
         self.assertEqual('class_member', obj.class_member)
         self.assertEqual([('__call__', (), {}),
-                          '_replace',
                           'factory',
                           'init',
                          ], actions)
@@ -306,7 +295,6 @@
 
         self.assertEqual((1,2,'3'), val)
         self.assertEqual([('__call__', (1,2), {'c':'3'}),
-                          '_replace',
                           'factory',
                           'func',
                          ], actions)
@@ -368,14 +356,12 @@
                           getattr, test_obj3, 'foo')
 
         self.assertEqual([('__getattribute__', 'foo'),
-                          '_replace',
                           'factory',
                           'init',
                           ('foo', 1),
                           ('foo', 2),
                           ('foo', 3),
                           ('__getattribute__', 'foo'),
-                          '_replace',
                          ], actions)
 
     def test_enable_proxying(self):
@@ -426,7 +412,6 @@
                          object.__getattribute__(test_obj5, '__class__'))
 
         self.assertEqual([('__getattribute__', 'foo'),
-                          '_replace',
                           'factory',
                           'init',
                           ('foo', 1),
@@ -464,7 +449,6 @@
         e = self.assertRaises(errors.IllegalUseOfScopeReplacer, test_obj7)
         self.assertIn("replace itself", e.msg)
         self.assertEqual([('__call__', (), {}),
-                          '_replace',
                           'factory'], actions)
 
 
@@ -599,7 +583,6 @@
         self.assertEqual('x', root1.func1('x'))
 
         self.assertEqual([('__getattribute__', 'var1'),
-                          '_replace',
                           ('_import', 'root1'),
                           ('import', self.root_name, [], 0),
                          ], self.actions)
@@ -625,7 +608,6 @@
         self.assertEqual('y', mod1.func2('y'))
 
         self.assertEqual([('__getattribute__', 'var2'),
-                          '_replace',
                           ('_import', 'mod1'),
                           ('import', mod_path, [], 0),
                          ], self.actions)
@@ -650,7 +632,6 @@
         self.assertEqual('y', mod2.func2('y'))
 
         self.assertEqual([('__getattribute__', 'var2'),
-                          '_replace',
                           ('_import', 'mod2'),
                           ('import', self.root_name, [self.mod_name], 0),
                          ], self.actions)
@@ -683,11 +664,9 @@
 
         mod_path = self.root_name + '.' + self.mod_name
         self.assertEqual([('__getattribute__', 'var1'),
-                          '_replace',
                           ('_import', 'root3'),
                           ('import', self.root_name, [], 0),
                           ('__getattribute__', 'var2'),
-                          '_replace',
                           ('_import', 'mod3'),
                           ('import', mod_path, [], 0),
                          ], self.actions)
@@ -727,11 +706,9 @@
 
         mod_path = self.root_name + '.' + self.mod_name
         self.assertEqual([('__getattribute__', 'mod4'),
-                          '_replace',
                           ('_import', 'root4'),
                           ('import', self.root_name, [], 0),
                           ('__getattribute__', 'var2'),
-                          '_replace',
                           ('_import', 'mod4'),
                           ('import', mod_path, [], 0),
                          ], self.actions)
@@ -792,23 +769,18 @@
         submodb_path = sub_path + '.' + self.submodb_name
 
         self.assertEqual([('__getattribute__', 'mod5'),
-                          '_replace',
                           ('_import', 'root5'),
                           ('import', self.root_name, [], 0),
                           ('__getattribute__', 'submoda5'),
-                          '_replace',
                           ('_import', 'sub5'),
                           ('import', sub_path, [], 0),
                           ('__getattribute__', 'var2'),
-                          '_replace',
                           ('_import', 'mod5'),
                           ('import', mod_path, [], 0),
                           ('__getattribute__', 'var4'),
-                          '_replace',
                           ('_import', 'submoda5'),
                           ('import', submoda_path, [], 0),
                           ('__getattribute__', 'var5'),
-                          '_replace',
                           ('_import', 'submodb5'),
                           ('import', submodb_path, [], 0),
                          ], self.actions)
@@ -1104,7 +1076,6 @@
         self.assertEqual('x', root6.func1('x'))
 
         self.assertEqual([('__getattribute__', 'var1'),
-                          '_replace',
                           ('_import', 'root6'),
                           ('import', self.root_name, [], 0),
                          ], self.actions)
@@ -1140,7 +1111,6 @@
         submoda_path = sub_path + '.' + self.submoda_name
 
         self.assertEqual([('__getattribute__', 'var4'),
-                          '_replace',
                           ('_import', 'submoda7'),
                           ('import', submoda_path, [], 0),
                          ], self.actions)
@@ -1165,7 +1135,6 @@
         self.assertEqual(1, root8.func1(1))
 
         self.assertEqual([('__getattribute__', 'var1'),
-                          '_replace',
                           ('_import', 'root8'),
                           ('import', self.root_name, [], 0),
                          ], self.actions)
@@ -1175,41 +1144,42 @@
     """The ScopeReplacer should be reentrant.
 
     Invoking a replacer while an invocation was already on-going leads to a
-    race to see which invocation will be the first to delete the _factory and
-    _scope attributes.  The loosing caller used to see AttributeErrors (bug
-    702914).
+    race to see which invocation will be the first to call _replace.
+    The losing caller used to see an exception (bugs 396819 and 702914).
 
-    These tests set up a tracer that stops at the moment just before one of
-    the attributes is being deleted and starts another call to the
-    functionality in question (__call__, __getattribute__, __setattr_) in
-    order win the race, setting up the originall caller to loose.
+    These tests set up a tracer that stops at a suitable moment (upon
+    entry of a specified method) and starts another call to the
+    functionality in question (__call__, __getattribute__, __setattr_)
+    in order to win the race, setting up the original caller to lose.
     """
 
     def tracer(self, frame, event, arg):
+        if event != 'call':
+            return self.tracer
         # Grab the name of the file that contains the code being executed.
-        filename = frame.f_globals["__file__"]
+        code = frame.f_code
+        filename = code.co_filename
         # Convert ".pyc" and ".pyo" file names to their ".py" equivalent.
         filename = re.sub(r'\.py[co]$', '.py', filename)
+        function_name = code.co_name
         # If we're executing a line of code from the right module...
-        if event == 'line' and 'lazy_import.py' in filename:
-            line = linecache.getline(filename, frame.f_lineno)
-            # ...and the line of code is the one we're looking for...
-            if 'del self._factory' in line:
-                # We don't need to trace any more.
-                sys.settrace(None)
-                # Run another racer.  This one will "win" the race, deleting
-                # the attributes.  When the first racer resumes it will loose
-                # the race, generating an AttributeError.
-                self.racer()
+        if (filename.endswith('lazy_import.py') and
+            function_name == self.method_to_trace):
+            # We don't need to trace any more.
+            sys.settrace(None)
+            # Run another racer.  This one will "win" the race.
+            self.racer()
         return self.tracer
 
-    def run_race(self, racer):
+    def run_race(self, racer, method_to_trace='_resolve'):
+        self.overrideAttr(lazy_import.ScopeReplacer, '_should_proxy', True)
         self.racer = racer
+        self.method_to_trace = method_to_trace
         sys.settrace(self.tracer)
-        self.racer() # Should not raise an AttributeError
-        # Make sure the tracer actually found the code it was looking for.  If
-        # not, maybe the code was refactored in such a way that these tests
-        # aren't needed any more.
+        self.racer() # Should not raise any exception
+        # Make sure the tracer actually found the code it was
+        # looking for.  If not, maybe the code was refactored in
+        # such a way that these tests aren't needed any more.
         self.assertEqual(None, sys.gettrace())
 
     def test_call(self):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-01-04 16:02:14 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-05 10:16:30 +0000
@@ -82,6 +82,11 @@
   of revisions whose ancestry is not obviously on the same developement
   line. (Vincent Ladeuil, #904744)
 
+* Make lazy imports resilient when resolved concurrently from multiple
+  threads. Now the stand-in object will behave as a proxy for the real object
+  after the initial access, rather than throwing. Assigning the object to
+  multiple names should still be avoided. (Martin von Gagern, #396819)
+
 * Not setting ``gpg_signing_key`` or setting it to ``default`` will use the
   user email (obtained from the ``email`` configuration option or its
   default value). (Vincent Ladeuil, Jelmer Vernooij, #904550)