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

[zhang kai]

Hi,

As itamar mentioned in ticket #6676 <http://tm.tl/#6676>, If a cancellation
function for a Deferred throws an exception(the cancel() method of
Deferred won’t
throw exceptions, but the canceller may), behavior is undefined. If the
cancellation function throws an exception it is currently not caught, and
cancellation does not occur.

We can catch the exception and log it, and fallback to just firing Deferred
withCancelledError. This won’t break any old code. But an exception
raising from the cancellation function often means the cancellation is
failed.

Another option we have is taking this opportunity to make the cancellation
being able to fail. There is the motivation:

There are cases where a Deferred is uncancellable. For example, we can call
twisted.mail.imap4.IMAP4Client.delete to delete a mailbox. When the
operation is waiting in the queue, we can cancel it by removing it from the
queue. However, when the operation is already sent and is waiting for the
response, it becomes uncancellable.

If we allow the canceller(NOT the cancel() method of the Deferred) to raise
an exception, we can tell the user the cancellation is failed and the
Deferredwon’t be fired with a CancelledError.

Raising an exception from cancel() may break the old code. So we can catch
the exception raised by the canceller, then return a False without firing
theDeferred to tell the user that the cancellation is failed.

In order to avoid missing unexpected exceptions, we can create a
CancellationFailedError. When the canceller raises CancellationFailedError,
we catch it and return False. When the canceller raises others exceptions,
we catch it, log it then return False.

Something like this:

def cancel(self):
if not self.called:
canceller = self._canceller
if canceller:
try:
canceller(self)
except CancellationFailedError:
return False
except Exception:
log.err(None, "Unexpected exception from canceller.")
return False
else:
# Arrange to eat the callback that will eventually be fired
# since there was no real canceller.
self._suppressAlreadyCalled = True
if not self.called:
# There was no canceller, or the canceller didn't call
# callback or errback.
self.errback(failure.Failure(CancelledError()))
return True
elif isinstance(self.result, Deferred):
# Waiting for another deferred -- cancel it instead.
return self.result.cancel()
else:
return False

This won’t break any code by raising an exception from cancel(), although
some code may rely on cancel() not returning any value.

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


Regards,

-Kai

[Terry Jones]

Hi Kai

I think it's helpful to keep clear on two different things that cancelation
is intended to do: 1) to fire the original deferred so that things relying
on it can proceed, and 2) to try to terminate an ongoing action that the
deferred might be waiting on.

For 1, I think calling cancel() should *always* result in the deferred
being fired (with, as it currently stands, CancelledError being used if a
provided cancel function does not fire the deferred itself). Always firing
the deferred is very important because the caller of cancel may have set up
many deferreds that rely on each other and their entire program may not be
able to proceed at all until the offending deferred is actually fired. It's
also contractually simple, and easy to document & understand.

For 2, the question is: do we want to also return information to the caller
if 2a) the underlying cancel function detects that it cannot, or can no
longer, stop the operation, or 2b) there is some kind of exception when
cancel calls the cancellation function. I don't think 2a) is really an
exception situation, so it makes sense, as you say, just to return False
from cancel in this case. It's basically the cancel function saying it was
too late to do anything about the underlying operation but not providing
more information than that. Internally raising and catching
CancellationFailedError (as in your code) in that case seems good to me.
In the case of 2b) I would just let the exception bubble up to the calling
code. Agreed, it could break some existing code, but isn't that existing
code already subject to that exact failure? It's just currently
undefined/undocumented.

Terry

Post by zhang kai
Hi,
As itamar mentioned in ticket #6676 <http://tm.tl/#6676>, If a
cancellation function for a Deferred throws an exception(the cancel() method
of Deferred won’t throw exceptions, but the canceller may), behavior is
undefined. If the cancellation function throws an exception it is currently
not caught, and cancellation does not occur.
We can catch the exception and log it, and fallback to just firing
Deferred withCancelledError. This won’t break any old code. But an
exception raising from the cancellation function often means the
cancellation is failed.
Another option we have is taking this opportunity to make the cancellation
There are cases where a Deferred is uncancellable. For example, we can
call twisted.mail.imap4.IMAP4Client.delete to delete a mailbox. When the
operation is waiting in the queue, we can cancel it by removing it from the
queue. However, when the operation is already sent and is waiting for the
response, it becomes uncancellable.
If we allow the canceller(NOT the cancel() method of the Deferred) to
raise an exception, we can tell the user the cancellation is failed and the
Deferredwon’t be fired with a CancelledError.
Raising an exception from cancel() may break the old code. So we can
catch the exception raised by the canceller, then return a False without
firing theDeferred to tell the user that the cancellation is failed.
In order to avoid missing unexpected exceptions, we can create a
CancellationFailedError. When the canceller raises CancellationFailedError,
we catch it and return False. When the canceller raises others
exceptions, we catch it, log it then return False.
canceller = self._canceller
canceller(self)
return False
log.err(None, "Unexpected exception from canceller.")
return False
# Arrange to eat the callback that will eventually be fired
# since there was no real canceller.
self._suppressAlreadyCalled = True
# There was no canceller, or the canceller didn't call
# callback or errback.
self.errback(failure.Failure(CancelledError()))
return True
# Waiting for another deferred -- cancel it instead.
return self.result.cancel()
return False
This won’t break any code by raising an exception from cancel(), although
some code may rely on cancel() not returning any value.
So, what’s your opinion on raising an exception from the canceller?
Regards,
-Kai
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[e***@twistedmatrix.com]

Post by Terry Jones
Hi Kai
[snip]
2b) there is some kind of exception when
cancel calls the cancellation function. I don't think 2a) is really an
exception situation, so it makes sense, as you say, just to return False
from cancel in this case. It's basically the cancel function saying it was
too late to do anything about the underlying operation but not
providing
more information than that. Internally raising and catching
CancellationFailedError (as in your code) in that case seems good to me.
In the case of 2b) I would just let the exception bubble up to the calling
code. Agreed, it could break some existing code, but isn't that
existing
code already subject to that exact failure? It's just currently
undefined/undocumented.

This seems like a bad behavior that we should fix. Allowing arbitrary
exceptions from other application code here makes it a lot harder to
write robust code that uses Deferred cancellation.

Beyond that, how much code has already been written that uses this
feature? If it was written based on the (admittedly, meager)
documentation that exists for the feature, then it won't have exception
handling.

We could leave the behavior as it is and document it and require all of
that code be updated and all future code be written to handle arbitrary
exceptions from a cancel call. Or we could get rid of the undocumented
exception case, make the existing code correct (if it was previously
correct based on documented behavior) and avoid making things more
difficult for all future users of Deferred cancellation.

In my experience, there are usually a lot of subtle concerns and tricky
corner cases when fixing an inconsistency between documentation and
implementation. This case seems like a much more clear-cut win to just
fix the implementation.

Jean-Paul

[e***@twistedmatrix.com]

Post by zhang kai
Hi,
As itamar mentioned in ticket #6676 <http://tm.tl/#6676>, If a
cancellation
function for a Deferred throws an exception(the cancel() method of
Deferred won’t
throw exceptions, but the canceller may), behavior is undefined. If the
cancellation function throws an exception it is currently not caught, and
cancellation does not occur.
We can catch the exception and log it, and fallback to just firing Deferred
withCancelledError. This won’t break any old code. But an exception
raising from the cancellation function often means the cancellation is
failed.

Keep in mind that the Deferred cancellation API is a "best effort" API.
There are no guarantees that anything can be cancelled. Consider the
fact that 90% or more of Deferreds out there don't even have
cancellation implemented for them yet and that before Deferred
cancellation was introduced, 100% of Deferreds were uncancellable. :)

Post by zhang kai
Another option we have is taking this opportunity to make the
cancellation
There are cases where a Deferred is uncancellable. For example, we can call
twisted.mail.imap4.IMAP4Client.delete to delete a mailbox. When the
operation is waiting in the queue, we can cancel it by removing it from the
queue. However, when the operation is already sent and is waiting for the
response, it becomes uncancellable.
If we allow the canceller(NOT the cancel() method of the Deferred) to raise
an exception, we can tell the user the cancellation is failed and the
Deferredwon’t be fired with a CancelledError.
Raising an exception from cancel() may break the old code. So we can catch
the exception raised by the canceller, then return a False without firing
theDeferred to tell the user that the cancellation is failed.

It's true that introducing an exception where previously there was no
exception is likely to break things.

However, *hiding* the failure in a return code that has to be checked
everywhere is not a solution to this problem. The cancellation has
still failed - the only difference returning False instead of raising an
exception makes is that most code won't bother to check the return value
and will miss out on the fact that cancellation has failed.

This mostly just breaks things differently (in a way that's much harder
to track down than a missing exception handler).

Post by zhang kai
In order to avoid missing unexpected exceptions, we can create a
CancellationFailedError. When the canceller raises
CancellationFailedError,
we catch it and return False. When the canceller raises others
exceptions,
we catch it, log it then return False.
canceller = self._canceller
canceller(self)
return False
log.err(None, "Unexpected exception from canceller.")
return False
# Arrange to eat the callback that will eventually be fired
# since there was no real canceller.
self._suppressAlreadyCalled = True
# There was no canceller, or the canceller didn't call
# callback or errback.
self.errback(failure.Failure(CancelledError()))
return True
# Waiting for another deferred -- cancel it instead.
return self.result.cancel()
return False
This won’t break any code by raising an exception from cancel(),
although
some code may rely on cancel() not returning any value.
So, what’s your opinion on raising an exception from the canceller?

What about a third option - if a cancellation function raises an
exception, fail the Deferred with that exception.

This:

1) avoids raising an exception from Deferred.cancel (and avoids
encoding error information in the return value of that method, forcing
application code to start checking for error return values)

2) Satisfies the expectation of the application code that cancelling
the Deferred will cause it to fire "soon" - with roughly the same
quality as if the Deferred had no canceller implemented at all.

3) Probably makes the implementation bug apparent by making the
exception available to errbacks on the Deferred.

I'm not entirely convinced (3) is ideal - it may be that the Deferred
should actually fire with CancelledError in this case, just as it would
without a canceller, and the exception raised by the canceller should
just be logged (somewhat like what your code above does - but after
logging the error the Deferred should actually be cancelled).

This has the same benefits, but puts the information about the
implementation into the application's log file rather than forcing
application-supplied errbacks to handle it somehow.

Jean-Paul

[Terry Jones]

Post by e***@twistedmatrix.com
What about a third option - if a cancellation function raises an
exception, fail the Deferred with that exception.

I really like this idea, but it wont work if the cancel function has
already fired the deferred.

Terry

[Terry Jones]

Post by e***@twistedmatrix.com
Keep in mind that the Deferred cancellation API is a "best effort" API.
There are no guarantees that anything can be cancelled. Consider the fact
that 90% or more of Deferreds out there don't even have cancellation
implemented for them yet and that before Deferred cancellation was
introduced, 100% of Deferreds were uncancellable. :)

I think this is (unintentionally) misleading. Although 90% (or more) of
deferreds don't have an explicit custom cancelation function, 100% of
deferreds can now be canceled.

Terry

[Glyph]

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

Post by zhang kai
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