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

Andrew Bennetts andrew-twisted at puzzling.org
Sat Apr 17 18:25:34 MDT 2004


It looks like I've succeeded in rekindling interest in this issue :)

On Sat, Apr 17, 2004 at 06:22:50PM -0400, Jonathan Simms wrote:
> This is response to issue 178 on the twisted tracker:
> http://www.twistedmatrix.com/users/roundup.twistd/twisted/issue178,
> about the deferred method setTimeout.
> 
> 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?

> 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.

> 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)
> 
> -------------------------------------------------------
> 
> Here you see that each step is very explicit. You can see what happens
> on success and on timeout (failure). I think one problem with setTimeout
> is the handling of the IDelayedCall is something that a new user could
> miss.  

First, there's a bug.  Presumably, cancelTimeout should return val,
otherwise you swallow the result/failure, regardless of what actually
happens.

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?

With setTimeout, this code becomes:

----
d = iReturnADeferred()

def onTimeout():
    handleTimeoutCondition()
    raise defer.TimeoutError

d.setTimeout(timeoutLen, onTimeout)

def success(value):
    handleSuccess()

def nonTimeoutFailure(reason):
    if reason.check(defer.TimeoutError):
        raise reason  # don't process TimeoutErrors here
    handleErrorCondition()

d.addCallback(success)
d.addErrback(nonTimeoutFailure)
----

I think that's an accurate translation, anyway, your example relies a bit
too heavily on mysterious global functions to be 100% clear.

> For instance, someone who hadn't /read/ the code of setTimeout, would
> most likely miss the returned IDelayedCall, and wouldn't know to cancel
> the pending timeout call in the event of success OR other failure. This
> is _a crucial piece of the code path_, and setTimeout makes this easier
> to miss.

I think you misunderstand setTimeout.  You can safely ignore the
IDelayedCall it returns (and the fact it does so isn't even documented in
the docstring), and the delayed call will be cancelled for you anyway if the
deferred is called back before the timeout triggers.  setTimeout doesn't
make this easier to miss; it makes it so you don't have to worry about it at
all.

> As from the Zen of Python Programming:
> 
> - Explicit is better than implicit
>     
>     setTimeout hides a small piece of code that contains all of the
>     important conceptual elements that a user is required to understand
>     so that they may handle this side-exit condition properly.

This is true of many functions and classes, e.g. how LineReceiver's
dataReceived handler works is an excellent example of how to do a certain
task in Twisted, but the implementation is hidden from the user!  Oh, except
that they can always read the source if they want to know :)

I don't believe that convenience functions are inherently bad because they
hide things from the user (and defer.py has lots of them).  If they mean
there's one less code idiom to learn, then great.  If they mean there's a
working example for people to read, including warts that real-life imposes
that wouldn't be in an artificial example, great.

>     
> - Special cases aren't special enough to break the rules
> 
>     Delayed calls are orthogonal to deferreds. 

This I somewhat agree with, enough that I'd be tempted to make setTimeout
live as a help function in twisted.internet.util rather than a method of
Deferred, or similar, except that it would be considerably harder and
messier to write.

>     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*.

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.

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.

>     Timeout conditions are _not necessarily_ something that a user needs
>     to deal with 80% of the time they are working with deferreds. 

Great.  So they won't call setTimeout.  Why is this a problem?

> - Simple is better than complex
> 
>     The deferred api should contain absolutely necessary functionality. 
> 
>     Let's emulate the python language, not the python standard library

On the other hand, timeouts are often desired functionality, and they should
have a common API for people to implement them and use them.

It would be a shame if "oh, I got a deferred from twisted.news, but I want
to set a timeout on it, so I do d.callback(twisted.news.magicTimeoutResult)
in a DIY delayed call... but if I get a deferred from twisted.mail I want to
set a timeout on, I do mailConnection.timeoutRequest(d), and if..." etc.

Functions/methods to support timeouts are an ideal candidate for inclusion
in Twisted, if only we could agree on what functions/methods :)

> - There should be one, and preferrably only one way to do it
> 
>     It is better to have users of our API use a widely-understood and
>     flexible idiom to accomplish this rather than a half-baked method
>     that saves them 8 lines of easily written and conceptually crucial
>     code

You mean error-prone, like your example ;)

>     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?

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.

> - If an implementation is hard to explain it's a bad idea
> 
>     Explaining to users how to properly use setTimeout takes more effort
>     than explaining to them how to use the tools that setTimeout employs
>     to achieve its functionality.

I believe your idiom suffers from this even worse than setTimeout.

I'm happy to write some docs to support correct usage of setTimeout that
people can be referred to.

> 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'm going to spend a little time pondering it, then I'll look at
implementing a patch and adding it to the bug.

> This is a less-than-optimal solution to an edge-case use of two
> orthogonal parts of our API.

I disagree.

> It makes no sense to leave a sharp, pointy thing lying around for users
> to hurt themselves with, and to constantly tell them "DON'T USE THAT".

Agreed.  It should be properly documented at least.

-Andrew.





More information about the Twisted-Python mailing list