[PATCH] atomictempfile: avoid infinite recursion in destructor

Matt Mackall mpm at selenic.com
Mon Dec 13 12:10:49 CST 2010


On Mon, 2010-12-13 at 12:46 -0500, Greg Ward wrote:
> On Mon, Dec 13, 2010 at 11:08 AM, Matt Mackall <mpm at selenic.com> wrote:
> > Ok, so the minimal version of this bug looks like:
> >
> > class a(object):
> >    def __init__(self, b):
> >        self.a = b
> >    def __getattr__(self, name):
> >        return getattr(self.a, name)
> >    def __del__(self):
> >        print self.a
> >
> > m = a()
> 
> Yup.
> 
> > The problem lies in how __getattr__ works and we could kill it with a
> > simple 'if name != "a":'.
> 
> Yup again.  That still seems like the way to go *if* we want to fix
> this on stable.  But a good long-term fix would be even better, so
> getting it fixed in stable is not that important to me.
> 
> 
> > But this code is much better written as:
> >
> > class b(file):
> >    def __init__(self, args):
> >        file.__init__(self, *args)
> >    def __del__(self):
> >        print self
> >
> > m = b()
> 
> Agreed.  That did not occur to me because of atomictempfile's odd
> interface: it was a file-like object with no apparent close() method.
> And it looks like close() means "discard changes"; you have to call
> rename() to keep your changes.  So it's still not quite a writeable
> file-like object.
> 
> > So if we're going to go to the trouble of fixing this ugly thing, I'd
> > rather fix it like this. We can do this to atomictempfile thusly and get
> > a cleaner result, but there are some issue with "posixfile" that need to
> > be sorted out here:
> >
> > diff -r 97c5189b8607 mercurial/util.py
> > --- a/mercurial/util.py Fri Dec 10 20:32:39 2010 -0600
> > +++ b/mercurial/util.py Mon Dec 13 10:03:45 2010 -0600
> > @@ -800,7 +800,7 @@
> >         raise
> >     return temp
> >
> > -class atomictempfile(object):
> > +class atomictempfile(file):
> >     """file-like object that atomically updates a file
> >
> >     All writes will be redirected to a temporary copy of the original
> > @@ -808,28 +808,24 @@
> >     name, making the changes visible.
> >     """
> >     def __init__(self, name, mode='w+b', createmode=None):
> > +        temp = mktempcopy(name, emptyok=('w' in mode),
> > +                               createmode=createmode)
> 
> Funny indentation there.
> 
> > +        if not self.closed:
> > +            file.close(self)
> 
> Why not super()?
> 
> > +            file.close(self)
> 
> Ditto.
> 
> Anyways, I'll give this patch a workout this afternoon and see if
> anything goes wrong.  But would you care to explain what you mean by
> "issues with posixfile that need to get sorted out"?  I don't *see*
> anything fishy from reading the code...

See util.posixfile. A posixfile is a file with sensible behavior, as
opposed to what open() on Windows gives us.

-- 
Mathematics is the supreme nostalgia of our time.




More information about the Mercurial-devel mailing list