Deferred cancellation

[exarkun]

Here's the current plan:
Deferred.__init__ is changed to take an optional no-argument callable as an
argument.  If given, the Deferred can be cancelled.
Deferred gains a new method, cancel.  If called on a Deferred for which no
canceller has been supplied to __init__, UncancellableDeferred is raised.
Otherwise, if the Deferred has no result, the canceller supplied to __init__ is
called and the Deferred errbacks itself with CancelledDeferred.  If the Deferred
does have a result, if the result is not a Deferred, AlreadyCalled is raised.
If the result is a Deferred, cancel is called on it.
Things I don't like about this:
    d = defer.Deferred(cancel)
    d.addCallback(lambda ign: defer.Deferred())
    # d.cancel() - works
    d.callback(None)
    # d.cancel() - doesn't
Basically the same problem, but an even worse behavior:
    d = defer.Deferred(canceller)
    d.addCallback(
        lambda ign: random.choice([
            defer.Deferred(canceller), defer.Deferred()]))
    d.callback(None)
    # d.cancel() - who knows if it works or not

[jknight]

I think the first issue is not likely to be a problem, any more than
cancellation of timed calls is. The second is a bit more worrisome, to me.
If I'm writing user code that wants to cancel an operation, in many cases I
don't particularly care whether or not the actual operation was cancelled or
not, as long as it pretends to have been. That is: it is inefficient to have a
thread continue blocking on a dns query, but it's still okay, as long as from my
point of view the query was aborted.
This perhaps points to an API which will either actually cancel via the
canceller, or just errback and silently suppress further callbacks if there is
no canceller.
That does seem a bit unclean, but I can't think of anything immediately wrong
with it.

[exarkun]

I think this sounds right.  Glyph and I had discussed this briefly last night
and came to pretty much the exact same conclusion.  The one extra detail we
discussed is that constructing a Deferred without a canceller should log a
deprecation warning.  To explicitly construct a Deferred which has no
cancellation behavior (and just has .callback()/.errback() suppressed after it
is cancelled), None should be passed in.  This should encourage people to think
more about cancellation, which should improve this situation overall in the future.

[exarkun]

Additionally, I think we decided that cancelling an already-fired Deferred is a
no-op.  This might bear some further discussion though.  As I write this
comment, I am leaning towards thinking it should raise an error, since that
parallels how DelayedCalls work, as you pointed out.

[jknight]

Patch to add .cancel and cancellers to lock/semaphore/queue, and tests for
above. Also makes deferred timeout be the same as cancellation.
This does point out an issue with having a default cancellation behavior. For
lock/semaphore, not providing a canceller function would cause the instance
state to essentially be corrupted if the deferred actually was cancelled, as the
lock would never be released.
That is not necessarily a fatal problem, as you can always just say "don't do
that, then".

[jknight]

> Patch to add .cancel and cancellers to lock/semaphore/queue, and tests for
                      ^ to deferred

[exarkun]

It'd be good to get this patch applied and checked in to a branch, so it's
easier to play around with, and for other people to test out.

[glyph]

The patch no longer applies cleanly; I'm going to try to clean it up and put it
in the cancel-990 branch.

[exarkun]

I don't think it even applied cleanly when it was initially applied.
Reverting the revert looks like the way to go.
svn merge -r 14694:14692
Results in only one extremely trivial conflict.

[glyph]

I've updated the reverting-the-revert thing that exarkun suggested so it applies cleanly and attached it as a patch, although I'm not sure what to do next. This was overhead on IRC:

<glyph> who wants to review deferred cancellation
<exarkun> glyph: -1 no use-cases

So I'm dropping the priority and leaving it around for future discussion.

I'm not sure if that's accurate since there are still calls to setTimeout in twisted.mail and twisted.names, but somehow this ticket seems to have fallen off of everyone's radar. I suspect some more GUI applications where there is a "cancel" button visible in the UI would perhaps drive some discussion here.

[moshez]

Explanation of use case:

We have a protocol that gets a request, then sends it to the "first layer" for approval, then sends it to the "second layer" for approval, and if the second layer also approves, implementing the request. The code looks something like:

d = sendtoFirstLayer()
d.addCallback(sendToSecondLayer)
d.addCallback(commitRequest)
d.addErrback(rollbackRequest)

Where sendToSecondLayer returns a deferred that is called back when the request was approved, or erred back if it is not approved. If after sending to the second layer, someone on the second layer disconnected, the request will never get approved or disapproved. I want to cancel the deferred, so the rollback will be done (and perhaps the request to be retried later).

[glyph]

Replying to moshez:

We have a protocol that gets a request,

That is way too abstract of a use-case, but #1442 has got something nice and concrete now.

I've updated the code a bit and put it up for review in cancel-990

[radix]

I would prefer that L{Deferred.cancel} on a Deferred that has no canceller raise something like NoCancellerError? immediately instead of errbacking with a CancelledError?. And get rid of the "swallowing" behavior.

I also don't understand this:

+        """
+        Verify that a canceller which invokes a (non-errback) callback will not
+        get errbacked with L{CancelledError}
+        """
+        def cancel(d):
+            self.cancellerCalled=True
+            d.errback(FooError())

The docstring says it's going to *callback* the deferred, not errback it (I think?) and then the test goes directly on to errback it instead.

I'm also confused about the following because the test cancels *A*, not *B*.

+    def test_cancelNestedDeferred(self):
+        """
+        Verify that a Deferred, A, which is waiting on another Deferred, B,
+        returned from one of its callbacks, will propagate L{CancelledError}
+        when B is cancelled.
+        """

The support for chained deferreds also seems dubious; perhaps it would be best to leave that up to implementations of cancellers.

I am really disgusted by the modification to testLock. Do not add anything more to that test. It should be broken up into smaller unit tests. If you want to leave it alone entirely, then fine, but put the new assertion in a different unit test. Same for the change to testSemaphore.

Thanks for the branch!

[jknight]

The swallowing and behavior when there is no canceller is for two reasons:

1) the user of the deferred should not have to care whether the provider provided special behavior for cancellation. The user simply wants to inform that he no longer gives a crap what happens to it.

2) To sensibly support the existing timeout mechanism.

Not sure what you meant by the first comment on the comment.

For test_cancelNestedDeferred, that's a typo in the docstring: it meant "when A is cancelled".

[glyph]

Replying to jknight:

The swallowing and behavior when there is no canceller is for two reasons: 1) the user of the deferred should not have to care whether the provider provided special behavior for cancellation. The user simply wants to inform that he no longer gives a crap what happens to it.

That would be nice, except then the code that returned the Deferred is going to be dealing with synchronous exceptions suddenly coming out of perfectly valid calls to callback() or errback(). Who knows what impact that's going to have on its state. I actually agree with Chris's review; I don't think that behavior is really acceptable.

2) To sensibly support the existing timeout mechanism.

The existing timeout mechanism is basically a bug. Don't use it. The functionality even being there makes it impossible to use the deferred from the reactor; there would be a circular dependency, since deferreds (as it stands) depend on the reactor for setTimeout.

It should be removed and replaced with a free utility function that can cancel the deferred at a later time rather than an inherent feature of Deferred.

For test_cancelNestedDeferred, that's a typo in the docstring: it meant "when A is cancelled".

Sorry. I wrote those docstrings with only a brief glance at the code.

[radix]

Replying to glyph:

Replying to jknight:

The swallowing and behavior when there is no canceller is for two reasons: 1) the user of the deferred should not have to care whether the provider provided special behavior for cancellation. The user simply wants to inform that he no longer gives a crap what happens to it.

That would be nice, except then the code that returned the Deferred is going to be dealing with synchronous exceptions suddenly coming out of perfectly valid calls to callback() or errback(). Who knows what impact that's going to have on its state. I actually agree with Chris's review; I don't think that behavior is really acceptable.

That's not accurate; there is code in there specifically to swallow the *first* call to .callback after Deferred.cancel on a Deferred that has no canceller, without raising an exception.

[glyph]

Replying to radix:

That's not accurate; there is code in there specifically to swallow the *first* call to .callback after Deferred.cancel on a Deferred that has no canceller, without raising an exception.

Huh. Maybe I should have read this code before putting it into review.

While it's an interesting solution to the superficial problem I mentioned, I think it still has the same problem at a deeper level where all the code expecting the deferred up until that point is still sitting around waiting for it, forever, potentially in some wacky state because of it. The client code might no longer care about its callbacks, but internal callbacks (say, to execute the next command on a command queue) could still need to be invoked.

Ultimately the problem is that client code should not use a public API to randomly change the internal implementation state of some framework it's calling.

[jknight]

While it's an interesting solution to the superficial problem I mentioned, I think it still has the same problem at a deeper level where all the code expecting the deferred up until that point is still sitting around waiting for it, forever, potentially in some wacky state because of it. The client code might no longer care about its callbacks, but internal callbacks (say, to execute the next command on a command queue) could still need to be invoked.

I think you need to read it again, still. :) The callbacks chain won't be waiting forever, because they'll have gotten an errback from before.

It is a bit odd for someone else to be errbacking your Deferred, which is why the Deferred needs to then not raise an exception if you do .callback/.errback yourself. This might have the potential for a problem, but, my feeling is that it's not actually going to be a problem

[jknight]

FWIW, I think I had both glyph and exarkun that it (the cancellation part) was a good idea back when this was first proposed 2 years ago. I'm sure we discussed it. Of course even if that's the case, it's still no _guarantee_ it's the right thing, of course. :)

[glyph]

I had a long chat with radix about this, and I think we agreed on a solution here.

My main premise here is that one of the most important features of Deferreds is that, at a particular point in the callback/errback chain, you can predict (based on API documentation, mostly) what results and error conditions you can expect when you add a callback.

Preserving this feature is important for basically the same reason that raising random asynchronous exceptions in Python is a bad idea. You can't write correct software if you can't predict both success and failure modes of each thing you invoke. In other words, Deferreds should not violate their contracts. Yes, this is not quite as serious as every program potentially raising random exceptions everywhere, but it is still important.

If you can cause any deferred to raise some kind of cancellation exception, then you can never have a trivial deferred which just has a success path; you've always got to take cancellation into account. Even if we thought forcing that in every case was a good idea (I'm not so sure it is), we have five years of Deferred-using software that wasn't written that way to contend with.

So, I think cancellation always has to be advisory. By that, I mean calling cancel() does not mean "stop what you are doing right now", it means, "stop what you are doing as soon as you can reasonably do so while maintaining correctness". The canonical example of this is cancelling some computation happening in a thread. You can't actually cancel it, you can just ignore its result. As far as calling / client code is concerned, by default, I would like the cancellation to be a no-op. It can't stop the code, so it shouldn't. This makes it safe for anyone who wants to stop an operation to call cancel on a deferred at any time. If it can be cancelled, it will stop, and if it can't be, then it will keep going. In any event, at no point can you cause a Deferred to violate its contract.

There is a tricky case when you get to the issue of chaining. Consider the following code:

def operation():
    A = getPage(...)
    def x(result):
        B = deferToThread(...)
        def y(result2):
            C = storeToDatabase(...)
            return C
        return B.addCallback(y)
    return A.addCallback(x)

A and C can be cancelled; B cannot. If cancellation is, by default, a no-op, then what happens when, after A has fired, but B has not yet, the caller of operation calls cancel() on A?

I propose that the correct behavior is to put A into a "cancelled" state, which will cause it to cancel C immediately as it is returned to A's callback chain as a result. That provides consistent behavior, and relay's the user's intention to cancel the aggregate operation of A, without hanging the callback chain halfway through or raising a spurious and unexpected failure for B, which does not provide a canceller.

In more sophisticated cases, where you know what you really want in a particularly interesting cancellation case, you can always write wrapping deferreds which do exactly what your application wants (discard the results of the threaded computation and immediately appear to have cancelled for the user's benefit, for example). We should write documentation for those as those cases arrive.

[jknight]

My first impression is that the comment above basically makes sense, but I'm going to think about it a little more.

One immediate comment, though:

If you can cause any deferred to raise some kind of cancellation exception, then you can never have a trivial deferred which just has a success path; you've always got to take cancellation into account.

You sure can: defer.succeed() is uncancellable (unless it's blocked waiting for the result of a nested deferred of course), as it's been callbacked before anyone else sees it.

What you can't have is a deferred which hasn't been callbacked yet which has only a success path. I'd argue that if you think you can even without cancel, you're probably wrong, but maybe that's not a very good argument.

[glyph]

Replying to jknight:

My first impression is that the comment above basically makes sense, but I'm going to think about it a little more.

Cool. Hopefully we won't have too much more discussion here :).

One immediate comment, though:

If you can cause any deferred to raise some kind of cancellation exception, then you can never have a trivial deferred which just has a success path; you've always got to take cancellation into account.

You sure can: defer.succeed() is uncancellable (unless it's blocked waiting for the result of a nested deferred of course), as it's been callbacked before anyone else sees it.

True, it's possible to implement something that can't raise a cancellation exception, but (provided a default cancel() implementation that errbacks) you can't specify a Deferred interface that can't raise a cancellation exception; as soon as you're doing something actually deferred, you encounter that possibility.

What you can't have is a deferred which hasn't been callbacked yet which has only a success path. I'd argue that if you think you can even without cancel, you're probably wrong, but maybe that's not a very good argument.

You're right, in the sense that in practice, it's pretty uncommon to have a Deferred which actually does something asynchronous but doesn't raise any errors. The "real world" case I'm concerned about is (ostensibly the vast majority of) legacy code which is ostensibly written to deal with specific types of errors, with no catch-all, or a catch-all which really freaks out.

I think it might be possible to have a default-raise canceller exception and still somehow guarantee particular exception types as your own result by trapping them or something. I'm not going to bother figuring it out though, since it's more work for the application programmer, it doesn't deal with the "legacy" case, and I think my solution satisfies this case just fine :).

[radix]

I've come to think that the "wait around for the next cancellable deferred" behavior is unnecessary and perhaps undesirable.

My proposal is that in the situation glyph describes, the cancel method should simply do nothing (going along with the "advisory" nature of cancellation). Given that the author of 'operation' clearly wants the Deferred that he returns to be cancellable, he can simply wrap B (the threaded Deferred) with something that glyph briefly mentioned in the last paragraph of his second most-recent post.

[radix]

Ok, never mind, go with your original proposal. :)

My problems stemmed from the fact that your proposal will sometimes cause unnecessary work (in non-optimal cases). *my* proposal, on the other hand, causes cancel() to be non-deterministic in buggy situations. I think in the end yours is better, despite ugliness in implementation.

[Peaker]

I have another implementation idea for this, a new Operation class, which has a deferred member, and a cancel() method.

An Operation could be thought of as a CancellableDeferred?, except this distinction is explicit and also recognized by its users.

Advantages:

• Deferred stays simple (for example, cdefer (#2245) can still work).
• Deferred vs Operation is nicer than Deferred vs CancellableDeferred? (represented by internal state and checks in the cancel() method).
• It does not add overhead to ordinary deferreds, only to cancellable ones, and then I suspect that it would add less.

Disadvantages:

• Changing a Deferred to a cancelable Deferred is backwards compatible, but changing a Deferred to an Operation will break using code. (But, if the code does not need to use cancel(), it can be given operation.deferred anyway)

[glyph]

Replying to Peaker:

I have another implementation idea for this, a new Operation class, which has a deferred member, and a cancel() method.

Thanks for the suggestion. I don't like it, and I'm going to argue against it, but this is a tricky area of the code and I really do appreciate ideas.

An Operation could be thought of as a CancellableDeferred?, except this distinction is explicit and also recognized by its users. Advantages: * Deferred stays simple (for example, cdefer (#2245) can still work).

I don't like the idea of hamstringing Deferred in order to support a C implementation. We made a number of really brain-damaged design decisions very early in the implementation of the reactor (for example, not using Deferreds for the client-connection logic) that were at least partially in support of C implementations which didn't really exist. I think that we can still have a C Deferred implementation that has a 'cancel' method.

* Deferred vs Operation is nicer than Deferred vs CancellableDeferred? (represented by internal state and checks in the cancel() method).

You're right about this, in the sense that the behavior of cancel() should be consistent. The last round of discussion here, however, implies that it will be consistent, and any internal state checks will be invisible to the caller.

* It does not add overhead to ordinary deferreds, only to cancellable ones, and then I suspect that it would add less.

I don't think this is a meaningful metric, because I don't know what "overhead" you're talking about, or how you would compare them.

Disadvantages: * Changing a Deferred to a cancelable Deferred is backwards compatible, but changing a Deferred to an Operation will break using code. (But, if the code does not need to use cancel(), it can be given operation.deferred anyway)

This is basically a dealbreaker. I'm really trying to encourage a much stricter interpretation of "compatibility" going forward, and changing the return value of existing functions to something completely incompatible is not going to work. Adding entirely new APIs in every case where some operation can be cancelled is unnecessarily ugly, since I believe that a modified version of cancellable deferreds will address the other issues that you raised.