[PATCH] atomictempfile: avoid infinite recursion in destructor
Matt Mackall
mpm at selenic.com
Mon Dec 13 10:08:14 CST 2010
On Mon, 2010-12-13 at 09:19 -0500, Greg Ward wrote:
> On Fri, Dec 10, 2010 at 4:57 PM, Matt Mackall <mpm at selenic.com> wrote:
> > On Mon, 2010-11-29 at 22:03 -0500, Greg Ward wrote:
> >> # HG changeset patch
> >> # User Greg Ward <greg-hg at gerg.ca>
> >> # Date 1291085861 18000
> >> # Branch stable
> >> # Node ID 76a458f91f5baad3dd75a26d445ea51402fc32d1
> >> # Parent 5fb924ee44d516e2f91bb7f5cfd5c81b0629faa4
> >> atomictempfile: avoid infinite recursion in destructor.
> >>
> >> This is a weird one: if Python sees that the caller is not passing the
> >> right args to the constructor, it raises TypeError rather than enter
> >> the constructor. But it still ends up calling the destructor, which
> >> assumes that self._fp has been set in the constructor. But it hasn't,
> >> because the constructor wasn't actually executed. So we go into a
> >> nasty infinite recursion between __del__() and __getattr__(), when we
> >> should just crash with TypeError so the programmer can see what he did
> >> wrong.
> >
> > I think it's a bit too weird. Either we do this for all variables used
> > by all destructors, or skip it. As this only defends against explosively
> > incorrect code, I don't think we should worry about it.
>
> There are so many levels of fail here that it's hard to explain, but let me try.
>
> 1. extension author screws up call to atomictempfile
> 2. atomictempfile has an infinite recursion bug in that case
> 3. Python (?) makes the infinite recursion particularly hard to deal
> with -- sometime Ctrl-C works, but I have had to "kill -9" the
> offending process as well
You might be interested to learn about Ctrl-\.
> First: a minor error in client code (forgot a required parameter)
> should result in an ordinary TypeError crash, not an infinite
> recursion. So atomictempfile magnifies my silly programmer error. If
> I could kill the offending process with a single Ctrl-C, I would
> probably have left it at that.
>
> But, for unknown reasons, the infinite recursion is hard to kill. (I
> suspect there's something fishy about Python's signal handing around
> __del__(), but I have not investigated.) So a simple programmer error
> is doubly magnified into a hard-to-kill runaway process.
>
> I can't fix every future extension or script that accidentally leaves
> out a required arg to atomictempfile. Nor can I go back in time and
> fix Python's signal handling in 2.4, 2.5, and 2.6. But I *can* do
> something about atomictempfile, at least in Mercurial 1.8 (possibly
> 1.7.3). Hence this patch.
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()
The problem lies in how __getattr__ works and we could kill it with a
simple 'if name != "a":'. 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()
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)
+ file.__init__(self, temp, mode)
self.__name = name
- self._fp = None
- self.temp = mktempcopy(name, emptyok=('w' in mode),
- createmode=createmode)
- self._fp = posixfile(self.temp, mode)
-
- def __getattr__(self, name):
- return getattr(self._fp, name)
+ self.__temp = temp
def rename(self):
- if not self._fp.closed:
- self._fp.close()
- rename(self.temp, localpath(self.__name))
+ if not self.closed:
+ file.close(self)
+ rename(self.__temp, localpath(self.__name))
def close(self):
- if not self._fp:
- return
- if not self._fp.closed:
+ if not self.closed:
try:
os.unlink(self.temp)
- except: pass
- self._fp.close()
+ except:
+ pass
+ file.close(self)
def __del__(self):
self.close()
--
Mathematics is the supreme nostalgia of our time.
More information about the Mercurial-devel
mailing list