[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