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

[exarkun at twistedmatrix.com]

On 09:27 am, kylerzhang11 at gmail.com wrote:
>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. :)
>
>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.

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

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