[Twisted-Python] Raising exception from a Deferred canceller.

[Glyph]

Thank you, Kai, for a great post describing the issue in detail.

On Aug 29, 2013, at 2:27 AM, zhang kai <kylerzhang11 at gmail.com> wrote:

> So, what’s your opinion on raising an exception from the canceller?

I feel pretty strongly that it ought to be handled in this manner:

Index: twisted/internet/defer.py
===================================================================
--- twisted/internet/defer.py	(revision 39819)
+++ twisted/internet/defer.py	(working copy)
@@ -456,7 +456,10 @@
         if not self.called:
             canceller = self._canceller
             if canceller:
-                canceller(self)
+                try:
+                    canceller(self)
+                except:
+                    log.err(failure.Failure(), "Canceller raised exception.")
             else:
                 # Arrange to eat the callback that will eventually be fired
                 # since there was no real canceller.

Raising an exception from a canceller is a bug.  It was never really supposed to do anything.  I guess you could be relying on the behavior right now where raising an exception will allow you to avoid callbacking or errbacking the Deferred, but only at the cost of delivering some random exception to some unsuspecting application code.

As Jean-Paul already pointed out, turning this into a boolean flag is not useful to anybody.  The way that you tell a cancelled operation has been cancelled is to add a callback to a point in the chain and then observe that it has been cancelled.

So, separately from how to handle unhandled exceptions, there's the question of making a Deferred 'atomic', by which I mean, a Deferred whose .cancel() method has no apparent external effect; no result is obtained.  (I am using the term 'atomic' because it seems like half the uses in this thread use "uncancellable" to mean "doesn't have an explicit canceller" and the other half of the uses mean "cancellation has no effect").

Currently, it is, at least, possible to construct a Deferred that will continue waiting for a result after .cancel() has been invoked.  However, it's surprisingly challenging.  You have to do this, or something like it:

def atomize(deferred):
    def complete(result):
        complete.done = True
        complete.result = result
        public.cancel()
        return result
    complete.result = None
    complete.done = False
    def again():
        return (Deferred(lambda x: x.callback(complete.result))
                .addCallback(await))
    def await(result):
        if complete.done:
            return result
        else:
            return again()
    public = again()
    deferred.addBoth(complete)
    return public

This took *me* like an hour to construct again from memory, so I have to assume that ~75% of Twisted users will either never realize it's possible or not really figure it out.  And I'm still not quite sure what sort of resource consumption this involves; will each .cancel() stack up another Deferred or will they be tail-recursed out somehow (/me pours out a 40 for tm.tl/411)?

Asking all these questions to implement something apparently simple seems an undue burden.  So it does seem reasonable that a canceller have some way to communicate that it doesn't actually want to callback or errback a Deferred.

We didn't want to make this too easy, because a no-op canceller is a crummy default behavior, but I think that the current mechanism is too difficult to implement and has too many moving parts.

So then the question is: is raising a new kind of exception a good way to do this?  That raises the question: what are good criteria for raising an exception versus returning a value in an API?

The main distinction for when to return a value versus when to raise an exception is what the default behavior of the next stack frame up should be.  If an API calls another API that exhibits a certain behavior, does it make more sense to, by default, continue up the stack towards an error handler, or to move on to the next statement?  In other words, does the termination in the manner in question indicate the sort of error that one should not attempt to recover from unless one knows how?

In the example you gave in the original post, the caller always has to check the 'did cancellation work' flag, so that flag should be an exception.  Forgetting to check it is an error, so by default it makes sense to jump over the stack.

In this case though, "I didn't call .callback or .errback" is not a type of exception that should ever propagate through multiple stack frames.  It's just an API safety feature, to make sure that you really intended to do things that way.  Also, in terms of going through multiple frames, a canceller can't really meaningfully call another canceller, unless it's very explicitly implementing some kind of delegation pattern; with such a pattern, failing to propagate the result is quite likely the intentional behavior.  So I can't see a reason to use an exception to indicate this.

Instead, I think perhaps a new constant value would be useful, so a canceller could return a constant to indicate that it really meant to not invoke either a callback or an errback.  'return NOT_CANCELLED' would indicate that the canceller intentionally took no action to call .callback or .errback, and could be called again.

Sorry for the long email.

-glyph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20130829/70cbfa07/attachment-0002.html>