Merge lp://staging/~gagern/bzr/bug396819-lazy_import-threadsafe into lp://staging/bzr

Thanks for working on this !

This looks promising and I won't mind polishing it as patch pilot
(or helping you) but before digging I'd like some feedback from
jam and benji which have been involved recently.

[feedback] Overall, the approach sounds good. I'm not sure I'm
reading this correctly but it seems you've now made
ScopeReplacer._replace() the only place where races can occur. If
that's the case and we want to lock something, that should
probably be there.

[nit]

17 +
18 + disallow_proxying()

A bit naked ;) A comment explaining why it's done here (as early
as possible but no sooner) will be good.

[needsinfo]

218 - if event == 'line' and 'lazy_import.py' in filename:
219 - line = linecache.getline(filename, frame.f_lineno)
220 - # ...and the line of code is the one we're looking for...
221 - if 'del self._factory' in line:
<...>
228 + if (filename.endswith('lazy_import.py') and
229 + function_name == self.method_to_trace):

This looks nice and less hackish, but I thought you were relying
on the code you're replacing here in lp:~gagern/bzr/bug-702914 ?

I wouldn't mind merging these tests instead (or their moral equivalent).

On 01.09.2011 10:11, Vincent Ladeuil wrote:
> [feedback] Overall, the approach sounds good. I'm not sure I'm
> reading this correctly but it seems you've now made
> ScopeReplacer._replace() the only place where races can occur. If
> that's the case and we want to lock something, that should
> probably be there.

Depends on what race you're talking about.

A race resulting in double import will occur if the second thread reads
the _real_obj member before the first thread has written it. As the read
of _real_obj occurs in _resolve, that's involved in the race as well,
and a lock should encompass that whole code section, i.e. be aquired
before _real_obj is first accessed.

A race resulting in an UNDETECTED double replacement is restricted to
the very end of _replace, starting at the second read for _real_obj, the
one to the prev_obj local variable.

Reducing the chances for the second kind of race is the only reason for
the duplicate read of _real_obj. The assumption is that importing a
module might take considerable time, so a critical code section only
after the import represents a much shorter timespan.

> [nit]
>
> 17 +
> 18 + disallow_proxying()
>
> A bit naked ;) A comment explaining why it's done here (as early
> as possible but no sooner) will be good.

Guess you're right. To make things even cleaner, I could import the
lazy_import module instead of the function of the same name. That would
make this call use a qualified name, giving it more context. Will do
both soon.

> [needsinfo]
>
> 218 - if event == 'line' and 'lazy_import.py' in filename:
> 219 - line = linecache.getline(filename, frame.f_lineno)
> 220 - # ...and the line of code is the one we're looking for...
> 221 - if 'del self._factory' in line:
> <...>
> 228 + if (filename.endswith('lazy_import.py') and
> 229 + function_name == self.method_to_trace):
>
> This looks nice and less hackish, but I thought you were relying
> on the code you're replacing here in lp:~gagern/bzr/bug-702914 ?

This branch here supersedes that one, in my opinion. Notice that I never
requested that one to be merged. It was only intended as a proof that
the race problems hadn't been completely resolved yet. And the fact that
problems remained despite the original test cases shows another fact:
you cannot reasonably test for all possible races, but only those you
can think of. Thus the more people reviewing the code and thinking of
possible races, the more likely they won't miss one. The number of test
cases alone does little to increase confidence.

> I wouldn't mind merging these tests instead (or their moral equivalent).

There is no direct equivalent, as the lines of code mentioned no longer
exist in that fashion. I also came to believe that the scenarios
approach was bad, as it implied that all three kinds of test could be
matched by the same break point line regexp, which isn't necessarily
obvious.

The tests here are imo suitable to guard against a wide range of likely
races. The fact that the method name is an argument makes adding more
tests easy. So I'd rather prefer these tests here over the ones from
lp:~gagern/bzr/bug-702914 . I'd be happy to add three more tests for
_replace instead of ...

Ping?

On Tue 06 Sep 2011 20:53:04 CEST, Martin von Gagern wrote:
> Ping?
John is sprinting this week so it might take a while longer before he
has time to look at this.

Cheers,

Jelmer

Second ping? Merge request has been out more than a month, please don't forget about it!

Sorry we've not managed to get this reviewed MvG. Your analysis here and in the bug about the previous change seem correct to me, but I'm unconvinced about making further changes to lazy_import in this regard.

I can't offer useful suggestions for improving the branch, only complain about the state of the pitch:

* There's no way to disable lazy imports for things like loggerhead that care about threading not startup time.

* Telling whether a given lazy import is actually doing anything useful is far too hard.

  * It's not uncommon to see an object get imported then unconditionally used further down in the module.

  * Many parts of bzrlib are lazily imported somewhere, but end up always being required anyway.

  * The import tariff tests are nice, but very heavy compared to say, a lint style check.

* Parsing import statements out of strings is silly in many respects.

What I'd like to see would be a replacement with fewer features bzrlib could move to instead. I'm imagining it would:

* Use the import statement directly.

* Only works on module objects.

* Use a proxy that remains in place but gains attributes.

Since bzrlib.lazy_import was introduced we've gained a bunch of other lazy mechanisms which take over some of the work, but it's all very ad hoc.

Am 23.11.11 15:19, schrieb Martin Packman:
> Sorry we've not managed to get this reviewed MvG. Your analysis here and in the bug about the previous change seem correct to me, but I'm unconvinced about making further changes to lazy_import in this regard.

Currently, lazy_import is not merely ill designed, but actually breaking
stuff. So I believe that this proposal still should receive a proper
review, and the fix be committed. Unless there actually is code ready to
replace it, fixing it is better than not fixing it.

> I can't offer useful suggestions for improving the branch, only complain about the state of the pitch:
> * There's no way to disable lazy imports for things like loggerhead that care about threading not startup time.

Would be easy, but a different issue, to be implemented in a different
branch, preferrably on top of this one.

> * Telling whether a given lazy import is actually doing anything useful is far too hard.
> * It's not uncommon to see an object get imported then unconditionally used further down in the module.
> * Many parts of bzrlib are lazily imported somewhere, but end up always being required anyway.
> * The import tariff tests are nice, but very heavy compared to say, a lint style check.

tariff tests can deal with only a very limited number of scenarios to
check, but will by design also catch obscure paths leading to some
import. Lint-style checks could process a lot more code paths, detecting
lots of the stuff you describe above, but would be likely to miss some
more obscure constructs. So I'd consider them complementary.

> What I'd like to see would be a replacement with fewer features bzrlib could move to instead. I'm imagining it would:
>
> * Use the import statement directly.

I know that strange things are possible, but how would this work?
"from bzrlib.lazyly import foobar", with "lazily" not being a module but
instead an object working as a proxy factory? I guess that should be
possible, although I have some concerns about nested modules.

> * Only works on module objects.

That would surely make things a lot easier and cleaner.

> * Use a proxy that remains in place but gains attributes.

Not sure about performance here. I can imagine why a self-replacing
proxy would at least feel faster. It should be possible to do some
benchmarks on this.

> Currently, lazy_import is not merely ill designed, but actually breaking
> stuff. So I believe that this proposal still should receive a proper
> review, and the fix be committed. Unless there actually is code ready to
> replace it, fixing it is better than not fixing it.

That's fair. Between me and Jelmer I'll see if we can manage a review here.

I'll leave the discussion of alternatives to a later merge proposal. :)

Okay, Jelmer and I have discussed the general issue and the approach this branch takes. So, the code here is aimed at removing cases that result in exceptions, either usefully (in the case of multiply assigned proxies), or incorrectly (in the case of thread races). On that basis, the branch should land, and soon so it has some time on trunk to shake out any further issues.

The disallow_proxying mechanism for keeping the old testing for accidentally assigning a lazy object before it gets thunked I'm not convinced should be included with this change.

+ ScopeReplacer._last_duplicate_replacement = self

This is module-global state which is only used so the one call point of disallow_proxying() in selftest can retroactively raise (the most recent occurrence of) the error. Given that bzrlib.tests and test modules do not lazily import the will have already pulled in many things (varying if -s is used), this will only test one real import path.

For this branch, I suggest pulling out that code and living with the reduced coverage of assignment accidents, and just using `self.overrideAttr(ScopeReplacer, "_should_proxy", False)` for tests.

To get coverage for import issues again, using a debug flag to set the _should_proxy value and something similar to the import tarrif tests would work, and should be done in a future branch.

+ The loosing caller used to see an exception (bugs 396819 and 702914).
...
+ in order win the race, setting up the original caller to loose.

When changing this docstring it'd be nice to fix the spelling of 'lose'. :)