[Twisted-Python] Raising exception from a Deferred canceller.
![](https://secure.gravatar.com/avatar/dcfe4f7a841ea0a06f5dcc6b4313a014.jpg?s=120&d=mm&r=g)
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
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
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 On Thu, Aug 29, 2013 at 10:27 AM, zhang kai <kylerzhang11@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/607cfd4a5b41fe6c886c978128b9c03e.jpg?s=120&d=mm&r=g)
On 10:18 am, terry@jon.es wrote:
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
![](https://secure.gravatar.com/avatar/607cfd4a5b41fe6c886c978128b9c03e.jpg?s=120&d=mm&r=g)
On 09:27 am, kylerzhang11@gmail.com wrote:
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. :)
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).
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
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
JP writes: On Thu, Aug 29, 2013 at 1:00 PM, <exarkun@twistedmatrix.com> wrote:
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
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
JP writes: On Thu, Aug 29, 2013 at 1:00 PM, <exarkun@twistedmatrix.com> wrote:
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
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Thank you, Kai, for a great post describing the issue in detail. On Aug 29, 2013, at 2:27 AM, zhang kai <kylerzhang11@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
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
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 On Thu, Aug 29, 2013 at 10:27 AM, zhang kai <kylerzhang11@gmail.com> wrote:
![](https://secure.gravatar.com/avatar/607cfd4a5b41fe6c886c978128b9c03e.jpg?s=120&d=mm&r=g)
On 10:18 am, terry@jon.es wrote:
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
![](https://secure.gravatar.com/avatar/607cfd4a5b41fe6c886c978128b9c03e.jpg?s=120&d=mm&r=g)
On 09:27 am, kylerzhang11@gmail.com wrote:
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. :)
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).
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
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
JP writes: On Thu, Aug 29, 2013 at 1:00 PM, <exarkun@twistedmatrix.com> wrote:
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
![](https://secure.gravatar.com/avatar/c8f4d4d1a8fc53c13ed05c202e0257fe.jpg?s=120&d=mm&r=g)
JP writes: On Thu, Aug 29, 2013 at 1:00 PM, <exarkun@twistedmatrix.com> wrote:
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
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Thank you, Kai, for a great post describing the issue in detail. On Aug 29, 2013, at 2:27 AM, zhang kai <kylerzhang11@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
participants (4)
-
exarkun@twistedmatrix.com
-
Glyph
-
Terry Jones
-
zhang kai