Code review comment for lp:~gagern/bzr/bug396819-lazy_import-threadsafe

Revision history for this message
Martin Packman (gz) wrote :

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'. :)

review: Needs Fixing

« Back to merge proposal