[Twisted-Python] why deferred.setTimeout is not my favorite API method

[slyphon at twistedmatrix.com]

>It looks like I've succeeded in rekindling interest in this issue :)
>
>> The reason why I added a "DON'T USE THIS" to the docstring was that I
>> find that we are telling people this at least once a week. I think it
>> has more gravitas coming from the API documentation, as there is this
>> illusion that the people on IRC don't really know what they're talking
>> about, but if the author has marked something in the documentation, it
>> has a certain level of legitimacy.
>
>Well, I guess I'm a bit confused.  Why not raise a DeprecationWarning as
>well then?  Is "DON'T USE THIS" meant to be a watered-down version of a
>DeprecationWarning?

Yes. It was a diplomatic move. I understand that you wrote this
function, and that you feel strongly that it adds value to the twisted
library. To me, a DeprecationWarning represents that there has been a
consensus reached that a certain function is not going to be supported
in the future. I wouldn't be so haughty as to decide that on my own, so
I put a note that reflected the advice that we normally give users on
IRC into the API docs.

>
>> I agree with spiv in the sense that it would be nice to have a minimal
>> interface that would provide this functionality. However, i think that
>> the level of control that is needed to pull this off properly is best
>> served by writing an explicit timeout method and accompanying call to
>> callLater. 
>
>The tricky part isn't the callLater, it's the actual cancellation of
>whatever the operation that would've produced a deferred result -- and
>that's something defer.py can't help with, because it is app-specific.

Indeed, i realised the flaws in my example about 20 minutes after
sending it

>>
>> now, I'd like to present the idiom that we are try to encourage users to
>> follow every time we have to steer them away from setTimeout. 
>> 
>> ------------------------------------------------------
>> 
>> d = iReturnADeferred()
>> 
>> def onTimeout():
>>     handleTimeoutCondition()
>> 
>> delayedCall = reactor.callLater(timeoutLen, onTimeout)
>> 
>> def success(value):
>>     handleSuccess()
>> 
>> def nonTimeoutFailure(reason):
>>     handleErrorCondition()
>> 
>> def cancelTimeout(val):
>>     if delayedCall.active():
>>         delayedCall.cancel()
>> 
>> d.addBoth(cancelTimeout)
>> d.addCallback(success)
>> d.addErrback(nonTimeoutFailure)
>> 
>> -------------------------------------------------------
>> 
>First, there's a bug.  Presumably, cancelTimeout should return val,
>otherwise you swallow the result/failure, regardless of what actually
>happens.

Yes, indeed.

>
>What I don't see here is the most important part: presumably,
>iReturnADeferred starts a long-running operation going, maybe a call to a
>remote server.  This code doesn't do anything to stop that operation, and
>doesn't do anything to prevent an AlreadyCalledError happening in that code
>when the long-running operation completes.  Or does the mysterious
>handleTimeoutCondition method do that?
>
>setTimeout doesn't provide that either -- but it was my understanding that
>that was the main objection to it.  Why is handling this yourself (which as
>you demonstrate is error-prone!) an improvement over setTimeout?

I apologise for the magicness implied by my example. 

What offends me about setTimeout is that it is a solution to a vague
problem. It seems to me that there are two different issues here, and
setTimeout doesn't solve either of them cleanly. 

The first is how to actually stop a long-running operation after a given
amount of time. Indeed, this functionality must be provided by the
framework author, perhaps exposing a public '.cancel()' method. There is
no way to provide or deal with this behavior in a generic way.

The other problem that I believe could be solved by a setTimeout-y
method is how to indicate that the client isn't interested in the
result of a long-running operation any more. 

I think this is an important distinction to make. It is a condition that
happens at the interface between your code and iReturnADeferred().
Instead of trying to stop whatever iReturnADeferred() is doing, you come
up with a way of saying, "after foo seconds, _i don't care about this
result_ and this is what we should do..." 


>
>>     Deferreds are a _reactive_ tool, they fire in response to an event
>>     
>>     delayed calls are a _proactive_ tool, they cause events to fire
>> 
>>     Mixing the two together for the special case of dealing with a
>>     timeout is at best hairy and at worst ugly and confusing.
>
>I disagree.  The event that Deferreds react to has to come from *somewhere*.

Yes, but the deferred is a reaction to an event. My point was that
you're adding event-creating code to an object whose job it is to react
to events. All i'm saying is that this adds complexity. Without the
current implementation of setTimeout, deferreds would be /completely
reactive/, and i think we can find a way to make them _appear_ that way.


>
>What is problematic is that timeouts provide a *second* place that might
>trigger the Deferred, in addition to whatever the usual place the creator
>expects.  With setTimeout, this means that AlreadyCalledErrors can arise
>from calls to d.callback/d.errback where the original author didn't expect
>them, just because a user of that module is setting timeouts to Deferreds.
>I thought this was the primary objection to setTimeout?  It's certainly the
>biggest concern I have about it.

indeed, i think this is the biggest problem I've faced when dealing with
deferreds

>
>Ok, how about a compromise: if a creator a Deferred is able to cope with
>timeouts, they should pass an "allowTimeouts=True" flag to the constructor.
>Without it, the setTimeout method will raise an AssertionError.  (For
>backwards-compatibility, for a release it will merely raise a Warning rather
>than an exception).  I'm willing to implement this, and properly document
>setTimeout in the Deferred HOWTO while I'm at it.  This is now my preferred
>option.

Hrrrrrrrrrrrrrrrrrrrmmmmm. Not a baaaaaaad compromise, but there's
something about it that doesn't sit with me. It seems that it would add
complexity to the very elegant design of Deferreds. My overall qualm
with setTimeout is that I think that if the API was in the state of
stability it is now, and setTimeout was proposed, that it's current
implementation would be rejected. 

I think that if we're going to provide this functionality, we should try
to come up with as transparent a solution as we can.

the following is an example of an idea i've been kicking around. Why
can't we just ignore the deferred after the timeout happens? 

(I use the style of my example above because that is what i'm
more comfortable with. If setTimeout would provide a similar
functionality, then i'm all for it.)

-----------------------------------------------------------------------

#!/usr/bin/env python2.3

from twisted.internet import defer, reactor

iTimedOut = False

defer.Deferred.debug = True

def setTimeoutForDeferred(d, seconds):
    aNewDeferred = defer.Deferred()
    
    from twisted.internet import reactor
    
    def onTimeout():
        # kill any further action on this deferred to avoid AlreadyCalledErrors
        d.callbacks = []  

        iTimedOut = True
        aNewDeferred.errback(defer.TimeoutError('deferred timed out'))
        
    delayed = reactor.callLater(seconds, onTimeout)
        
    def cb(value):
        if not iTimedOut:
            aNewDeferred.callback(value)
        
    def eb(reason):
        if not iTimedOut:
            aNewDeferred.errback(reason)
            
    def cancelTimeout(val):
        if delayed.active():
            delayed.cancel()
        return val

    d.addCallbacks(cb, eb)
    d.addBoth(cancelTimeout)

    return aNewDeferred


def iReturnADeferred():    # this is frameworky code
    d = defer.Deferred()
    reactor.callLater(5, d.callback, 'winnar!')
    return d


def iSetATimeout():    # this represents client-programmer code
    origd = iReturnADeferred()

    d = setTimeoutForDeferred(origd, 2)

    def cb(val):
        # if we time out, this code should not be called
        print "hooray!iReturnADeferred returned: %s" % val

    def eb(reason):
        e = reason.trap(defer.TimeoutError)
        if e == defer.TimeoutError:
            print "woops! we timed out"
        else:
            print "uh-oh! we didn't time out and there was an error!"

    d.addCallbacks(cb, eb)
    return d



if __name__ == "__main__":
    d = iSetATimeout()

    reactor.run()


-----------------------------------------------------------------------

Again, this is just code to illustrate my idea. There's no reason why
we have to hand the deferred that's returned by iReturnADeferred to the
rest of our code. We can hold on to that one and can substitute our own
deferred that is a bit more intelligent, it understands how to break the
chain after a length of time has passed.

The advantage here is that the author of iReturnADeferred doesn't need
to plan specially for timeout conditions, and the client doesn't have to
worry about AlreadyCalled errors (which to me are not something to
pooh-pooh, I take them as a sign that there's something wrong with my
design).

>>     I would like to recommend that we document a best-practice idiom
>>     in the deferred api documentation, and remove setTimeout.
>
>I don't believe it's easily written until you have a pretty advanced grasp
>of both Deferreds and reactor.callLater.  I don't see why allowing users of
>Deferreds to just say "d.setTimeout(5)" is a bad thing?


again, there's a difference between stopping a long-running or
long-waiting operation, which is something that deferreds cannot handle
at all, and indicating that after a certain amount of time, we don't
care about the result of that operation and would like to consider it an
erroneous condition.

I don't think that setTimeout makes this distinction clear (which may,
of course be a fault of the documentation/examples). Also, I want
whatever solution is accepted to make it easier to avoid
AlreadyCalled errors.


>
>setTimeout has the big big advantage that it's dead easy for *users* of
>Deferreds to work with.  The trouble with it is that it requires some work
>from the *creators* of Deferreds to support safely -- but I see the same
>flaw in every solution proposed, including your own of documenting and
>entirely by-hand idiom.
>

okay, i can understand this, and can agree in some sense. 


>> spiv said: 
>> 
>> "In hindsight, I should've thought harder about the implications of
>> setTimeout before I committed it... but now it's there, I don't like the
>> idea of ripping it out unless we have a clearly better solution."
>
>I think my "allowTimeouts" proposal above is a clearly better solution. :)

I don't like it. It just seems hacky. deferreds should handle this case,
without any extra specification from the author, as I believe that my
idea above would. 

>
>I'm going to spend a little time pondering it, then I'll look at
>implementing a patch and adding it to the bug.

please, lets come to a consensus before committing anything. ;)


Respectfully yours,
Jonathan