[PATCH] atomictempfile: avoid infinite recursion in destructor

Greg Ward greg-hg at gerg.ca
Mon Dec 13 11:46:33 CST 2010


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

Greg


More information about the Mercurial-devel mailing list