[PATCH] store: break up reference cycle introduced in 9cbff8a39a2a

[Adrian Buehlmann]

On 2011-05-05 01:02, Matt Mackall wrote:
> On Wed, 2011-05-04 at 14:22 +0200, Adrian Buehlmann wrote:
>> On 2011-05-04 13:42, Adrian Buehlmann wrote:
>>> # HG changeset patch
>>> # User Adrian Buehlmann <adrian at cadifra.com>
>>> # Date 1304506739 -7200
>>> # Node ID 6aa9981d0f3dbdeaa14ed5a12f68f49fd171a5f8
>>> # Parent  bf951d58b9172e3dee1be36032784956e5775636
>>> store: break up reference cycle introduced in 9cbff8a39a2a
>>>
>>> see also 996c1cd8f530
>>>
>>> diff --git a/mercurial/store.py b/mercurial/store.py
>>> --- a/mercurial/store.py
>>> +++ b/mercurial/store.py
>>> @@ -365,26 +365,27 @@
>>>              self._load()
>>>          return iter(self.entries)
>>>  
>>> +class _fncacheopener(scmutil.abstractopener):
>>> +    def __init__(self, op, fnc, encode):
>>> +        self.opener = op
>>> +        self.fncache = fnc
>>> +        self.encode = encode
>>> +
>>> +    def __call__(self, path, mode='r', *args, **kw):
>>> +        if mode not in ('r', 'rb') and path.startswith('data/'):
>>> +            self.fncache.add(path)
>>> +        return self.opener(self.encode(path), mode, *args, **kw)
>>> +
>>>  class fncachestore(basicstore):
>>>      def __init__(self, path, openertype, encode):
>>>          self.encode = encode
>>>          self.path = path + '/store'
>>>          self.createmode = _calcmode(self.path)
>>> -
>>> -        storeself = self
>>> -
>>> -        class fncacheopener(openertype):
>>> -            def __call__(self, path, mode='r', *args, **kw):
>>> -                if mode not in ('r', 'rb') and path.startswith('data/'):
>>> -                    fnc.add(path)
>>> -                return openertype.__call__(self, storeself.encode(path), mode,
>>> -                                           *args, **kw)
>>> -
>>> -        op = fncacheopener(self.path)
>>> +        op = openertype(self.path)
>>>          op.createmode = self.createmode
>>>          fnc = fncache(op)
>>>          self.fncache = fnc
>>> -        self.opener = op
>>> +        self.opener = _fncacheopener(op, fnc, encode)
>>>  
>>>      def join(self, f):
>>>          return self.path + '/' + self.encode(f)
>>
>> I think the reference cycle in 9cbff8a39a2a goes like this:
>>
>>       --> fncacheopener object
>>      |        |
>>      |    local scope of function fncachestore.__init__
>>      |        |
>>       --- fncache object bound to name fnc
>>
>> Not sure if that's really a problem though.
> 
> Yes, there's a reference cycle here (not ideal). But it's not clear to
> me that it's anything new though:
> 
> http://www.selenic.com/hg/file/222c8ec7a274/mercurial/store.py#l369
> 
> Here the nested fncacheopener function holds a reference to its parent
> scope, which contains self, and then self.opener takes a reference to
> that, creating a loop.
> 
>> With my patch, the references go like this:
>>
>>        -- fncachestore object
>>       |       |
>>       |   _fncacheopener object  --
>>       |       |                    |
>>        -> fncache object           |
>>               |                    |
>>           object of openertype  <--
> 
> And this should indeed fix it, so I'm going to queue it up.
> 
> But it's not ideal as it's kind of a hybrid inheritance-by-wrapping
> thing again. More thought is needed here. Ideally, we'd come up with a
> way to avoid wrapping openers in openers or dynamically build classes. 
> 
> Off the top of my head, the base opener might allow stacking name
> transformations on top of the basic open method:
> 
> Then fncachestore could do something like:
> 
>   self.opener = openertype(self.path)
>   self.fncache = fncache(openertype(self.path))
>   self.opener.addfilter(self.encode)
>   self.opener.addfilter(self.fncache.add)
> 

It's unlikely that I will do anything further here. So whoever would
like to jump in here, feel free to try making Matt happier :-).