Opened 14 years ago

Last modified 14 years ago

#860 enhancement closed fixed (fixed)

patch: post-mortem executor for Deferred

Reported by: zooko Owned by:
Priority: normal Milestone:
Component: Keywords: core
Cc: Glyph, spiv, jknight, mcfletch, zooko Branch:
Author:

Description


Attachments (3)

svn.diff.txt (7.7 KB) - added by zooko 14 years ago.
defer2.diff (4.2 KB) - added by jknight 14 years ago.
deferdelpatch.diff (5.7 KB) - added by mcfletch 14 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 14 years ago by zooko

Here is a patch that allows Deferreds to reference one another in a cycle and
still to get garbage-collected and do their useful __del__() behavior.  There is
a test case added to test_defer that illustrates the issue.  Twisted without
this patch fails that test case, and with that patch passes that test case.

(Without the patch, Deferreds which are in a reference cycle will become
uncollectable garbage.)

The patch could undoubtedly be improved -- this is my first serious foray into
hacking Twisted internals after all.

In particular, much of this patch consists of adding a line to update the pmx
after each line that changes one of the Deferred's members.  I considered using
a setattr hack or whatever the modern equivalent is, but decided to leave it the
simple way and see what you think.

This patch is backwards compatible to python 2.2.

Regards,

Zooko

Changed 14 years ago by zooko

Attachment: svn.diff.txt added

comment:2 Changed 14 years ago by zooko

patch attached

comment:3 Changed 14 years ago by spiv

Assigning to glyph, I think he should have the final word on this.

Some comments from me...

It feels like a lot of extra complexity for a relatively small gain.  The test
case looks to contrived to be a real motivation for me, can you give a
real-world example where this helps?

This creates yet another object when Deferreds are constructed, slowing them
down more.  A better idea might be to only use pmx when Deferred.debug is True,
so you only incur the cost when debugging?  I don't see a clean way to do this,
though.

Also, I think you should probably define a property for 'called', for backwards
compatibility.  Although it's not meant to be a public attribute, I bet there is
code out there that relies upon it anyway.  And make it raise a DeprecationWarning.

Also, defining a class within a class isn't a good idea.  It confuses e.g.
serialisation machinery.  Just make it a top-level class, it won't hurt.

Finally, have you tried ditching the __del__ method of Deferred completely, and
reimplementing it with a weakref?  If the weakref bugs in various versions of
Python don't bite us (and they shouldn't, we wouldn't be doing crud like
resurrecting objects or making weakrefs inside weakref callbacks), then that
might be a nicer way to get rid of the __del__.  I'm not sure how feasible this
is, but if it is I think I'd much rather see it done that way.

comment:4 Changed 14 years ago by zooko

> It feels like a lot of extra complexity for a relatively small gain.  
> The test
> case looks to contrived to be a real motivation for me, can you give a
> real-world example where this helps?

I'm motivated by it because I like to be able to assume that there is 
no uncollectable garbage in my program.

In particular, yesterday I had a problem of "why is this object still 
alive?".  Those kinds of problems are somewhat ugly to diagnose, and in 
the process of debugging it I had to spend a few minutes thinking about 
whether I might have a reference cycle of Deferreds that were holding 
onto the offending object and preventing it from being collected.  I 
could have stopped after those few minutes, e.g. by re-running the 
program and checking gc.garbage, but instead I spent a few hours 
hacking up this patch.

Of course, the reason the object was alive turned out to be a bug in my 
code, namely in an earlier version of this:

http://mnetproject.org/repos/pyutil-unstable/pyutil/weakutil.py

Anyway, my point is that I get a nice happy feeling from saying to 
myself "Nope -- no chance of reference cycles with __del__'s anywhere 
in *this* codebase".

> This creates yet another object when Deferreds are constructed, 
> slowing them
> down more.  A better idea might be to only use pmx when Deferred.debug 
> is True,
> so you only incur the cost when debugging?  I don't see a clean way to 
> do this,
> though.

I think the finalizer behavior is required even when not in debug mode. 
  You want, for example, "unhandled error in Deferred" log msgs even 
when not in debug mode, right?

> Also, I think you should probably define a property for 'called', for 
> backwards
> compatibility.  Although it's not meant to be a public attribute, I 
> bet there is
> code out there that relies upon it anyway.  And make it raise a 
> DeprecationWarning.

Okay.  If I'm going to use properties, I can probably move all of the 
"update _debugInfo" and "update reprer" lines into properties.

> Also, defining a class within a class isn't a good idea.  It confuses 
> e.g.
> serialisation machinery.  Just make it a top-level class, it won't 
> hurt.

Okay.  I had a reason to use an inner class which turns out to be not a 
reason.

> Finally, have you tried ditching the __del__ method of Deferred 
> completely, and
> reimplementing it with a weakref?  If the weakref bugs in various 
> versions of
> Python don't bite us (and they shouldn't, we wouldn't be doing crud 
> like
> resurrecting objects or making weakrefs inside weakref callbacks), 
> then that
> might be a nicer way to get rid of the __del__.  I'm not sure how 
> feasible this
> is, but if it is I think I'd much rather see it done that way.

So, create a weakref to the new Deferred whenever a Deferred is 
created, with a weakref callback that generates the log?  I can't see 
how this could be any nicer or more efficient.  The ugly part of this, 
however you do it, is keeping the information necessary for the log 
alive even after the Deferred itself is gone.

Regards,

Zooko

comment:5 Changed 14 years ago by Glyph

OK, I am not quite decided yet, but I think I am leaning towards merging this
patch.  Something nags at me about it that isn't quite right though... I will
spend some time on this and think about its implications some more over the weekend.

comment:6 Changed 14 years ago by jknight

I like the idea, but I don't like a few things about this exact 
implementation. Lemme cook up a new patch for comment before you merge.

Changed 14 years ago by jknight

Attachment: defer2.diff added

comment:7 Changed 14 years ago by jknight

See attached. Creates an object that holds the destructor info 
on demand (at creation time, if debug set. Otherwise, when the callback 
chain is empty and a failure is the last element.) Because 
DebugInfo.failResult can only be a cleaned failure, I believe it should be 
safe from reference cycles.

If everyone's agreeable, I'll commit.

comment:8 Changed 14 years ago by zooko

> See attached. Creates an object that holds the destructor info
> on demand (at creation time, if debug set. Otherwise, when the callback
> chain is empty and a failure is the last element.) Because
> DebugInfo.failResult can only be a cleaned failure, I believe it 
> should be
> safe from reference cycles.

I like it.  I might have done something similar, but I didn't know if I 
had to allow .debug to be turned on dynamically after the Deferred was 
instantiated.

You should remove the line that says "Avoid reference cycles because 
this object defines __del__.".  :-)

Regards,

Zooko

Changed 14 years ago by mcfletch

Attachment: deferdelpatch.diff added

comment:9 Changed 14 years ago by mcfletch

I can vouch for it being a real problem.  TwistedSNMP ran afoul of a number of
deferred reference cycles, and as a result was leaking memory horribly.  I
developed my own patch before I was pointed to this tracker item by Glyph.  I've
attached it.

My strategy was essentially the same; create a sub-object and forward the
log-related attributes to it.  I used a descriptor class for that forwarding. 
The patch still has the overhead of creating a new object as well, so likely
zooko's patch is a better approach.

Anyway, patch attached for reference.

comment:10 Changed 14 years ago by jknight

Committed in r12960.

comment:11 Changed 8 years ago by <automation>

Owner: Glyph deleted
Note: See TracTickets for help on using tickets.