[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