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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

review: Needs Information

« Back to merge proposal