Rev 6426: (gz) Make lazy imports proxy by default so that concurrent resolution is in file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/
Patch Queue Manager
pqm at pqm.ubuntu.com
Thu Jan 5 11:06:48 UTC 2012
At file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 6426 [merge]
revision-id: pqm at pqm.ubuntu.com-20120105110647-1vtwq3fi5v88ybqb
parent: pqm at pqm.ubuntu.com-20120105103949-xxe5dkbfiu16q4ch
parent: martin.packman at canonical.com-20120105101213-m66rld2juma44ozv
committer: Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Thu 2012-01-05 11:06:47 +0000
message:
(gz) Make lazy imports proxy by default so that concurrent resolution is
robust (Martin Packman)
modified:
bzrlib/builtins.py builtins.py-20050830033751-fc01482b9ca23183
bzrlib/lazy_import.py lazy_import.py-20060910203832-f77c54gf3n232za0-1
bzrlib/tests/test_lazy_import.py test_lazy_import.py-20060910203832-f77c54gf3n232za0-2
doc/en/release-notes/bzr-2.5.txt bzr2.5.txt-20110708125756-587p0hpw7oke4h05-1
=== modified file 'bzrlib/builtins.py'
--- a/bzrlib/builtins.py 2011-12-30 14:36:46 +0000
+++ b/bzrlib/builtins.py 2012-01-05 09:50:04 +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'
--- a/bzrlib/lazy_import.py 2011-12-19 14:40:24 +0000
+++ b/bzrlib/lazy_import.py 2012-01-05 09:50:04 +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'
--- a/bzrlib/tests/test_lazy_import.py 2011-12-19 12:14:20 +0000
+++ b/bzrlib/tests/test_lazy_import.py 2012-01-05 09:50:04 +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'
--- a/doc/en/release-notes/bzr-2.5.txt 2012-01-05 10:39:49 +0000
+++ b/doc/en/release-notes/bzr-2.5.txt 2012-01-05 11:06:47 +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)
More information about the bazaar-commits
mailing list