Opened 18 years ago

Last modified 11 years ago

#411 defect reopened

— at Returning a Deferred from the callback of another Deferred too many times results a RecursionErrorVersion 12

Reported by: etrepum Owned by: etrepum
Priority: highest Milestone:
Component: core Keywords:
Cc: Glyph, radix, Jean-Paul Calderone, spiv, itamarst, etrepum, titty, oubiwann, Tristan Seligmann, Thijs Triemstra, zooko@… Branch:
Author:

Description (last modified by Glyph)

Returning a Deferred from a callback to another Deferred will result in a RecursionError if it is done too many times with Defererds that have already fired.

The traceback in question doesn't have any application code on it and can be very difficult to interepret. At worst, Twisted should alert you to what has happened in a comprehensible manner; however, it should really be possible to just return Deferreds from callbacks indefinitely.

This is related to the more general issue of calling a one Deferred's callback from another Deferred's callback, but we can fix this specific case in a potentially more general way.

Change History (13)

comment:1 Changed 18 years ago by etrepum

The magic stack-blowing number for me is 250, YMMV:

    from twisted.internet.defer import succeed
    def print_(v): print v
    def blowstack(howmany):
        def _blowstack(v):
            print v
            if v < howmany:
                return succeed(v+1).addCallback(_blowstack)
            return 'did %d iterations' % (v,)
        return _blowstack(0)
    blowstack(250).addBoth(print_)

I have a rewrite of the deferred module that uses "tail call optimization" and "very un-
threadsafeness" that fixes this problem and probably makes deferreds much faster (at least 
chaining wise).  The bonus/problem is that it also does the implicit chaining (see issue #410).
I'll probably post the source this weekend.

Changed 18 years ago by etrepum

Attachment: defer.py added

comment:2 Changed 18 years ago by etrepum

ah what the hell, here's what I have so far.  not quite pluggable as a replacement for twisted's 
deferred yet.  replace microfailure with t.p.failure.

comment:3 Changed 18 years ago by titty

You also get very nasty bugs, which aren't easy to track down.
I was using the following class to transfer a 30mb file, after the file had been
transferred, the program suddenly used up to 500MB ram, before dumping core or
exiting with a MemoryError. Problem seems to be that the deferred code tries to
create a Failure object (which also fails, because of RecursionError), then a
failure object for the upper level and so on. I can supply a complete example if
anyone wants it.


class RemoteFile(object):
    def __init__(self, file=None):
        self.file = RemoteFileReference(file)
        self.data = []

    def readAll(self):
        def gotData(d):
            if d=='':
                return "".join(self.data)
            else:
                self.data.append(d)
                return self.read().addCallback(gotData)


        return self.read().addCallback(gotData)

    def read(self, size=0x10000):
        log.msg("%s: calling remote read(%s)" % (self, size))
        return self.file.callRemote('read', size)

comment:4 Changed 18 years ago by etrepum

My best guess in that particular example is that it should be rewritten using a callLater in there 
somewhere, because on localhost it will almost certainly recurse, since it doesn't have to wait for the 
data.

The fact that deferred fires back IMMEDIATELY instead of giving the runloop a chance isn't always what 
you want :)

comment:5 Changed 18 years ago by Glyph

Could you work on streamlining your Deferred improvements for inclusion?

comment:6 Changed 18 years ago by etrepum

I don't have any time to work on this for another week or two.. so if you want it ASAP then you'll have to 
have someone else give it a shot.

comment:7 Changed 18 years ago by Glyph

Nope, not high priority.  Just trying to make sure that everything is assigned
to *somebody*.

comment:8 Changed 15 years ago by oubiwann

Cc: oubiwann added
Component: conch

comment:9 Changed 15 years ago by radix

Component: conchcore

comment:10 Changed 15 years ago by Antoine Pitrou

Ok, I've looked at this and such an optimization looks simply incompatible with the expectations in the test cases, and the fact that Trial itself works with deferreds. (the replacement defer.py doesn't work at all with twisted.test.test_defer even if the modifications are carefully merged)

The killer is that most tests in test_defer expect callbacks to be called immediately if possible, which kills any possibility of transforming recursive calls to runCallbacks() into an iterative loop, because if you do so the callbacks are run *after* the test checks whether the callbacks have been run :-)) (they are run when the test method returns to Trial, which is too late).

Perhaps it is possible to optimize for some very specific cases rather than the general runCallbacks() recursivity, but I'm not sure it's worth the additional complexity (and I'm not even sure it's possible, because it probably depends on exactly what kind of things Trial does with callbacks). Also, witnessing my observations above, it would slightly change the semantics of Deferred which may break some existing code.

comment:11 Changed 14 years ago by Peaker

Keywords: core removed
Resolution: invalid
Status: newclosed

This breaks commonly expected behavior of Deferreds. It does not pass the test suite.

Deferreds calling callbacks synchronously is akin to normal function calls, and deferred users should be and are aware of this.

comment:12 Changed 13 years ago by Glyph

Description: modified (diff)
Priority: highnormal
Resolution: invalid
Status: closedreopened
Summary: Deferreds are good at blowing the stackReturning a Deferred from the callback of another Deferred too many times results a RecursionError

While the specific optimization in question might not work, there's definitely something we can do to improve this situation.

Brian Warner did a really good writeup of the implications of this problem in a ticket on the allmydata tracker which he later copied as a message to the Twisted list.

However, the suggested fix there (reactor.eventually()) is wrong, in my opinion. You don't need to rip the whole stack in order to fix this problem, only the portion that is already running inside defer.py.

Note: See TracTickets for help on using tickets.