[PATCH] atomictempfile: avoid infinite recursion in destructor

[Greg Ward]

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

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.

Greg