On Wed, May 28, 2014 at 6:18 PM, Jason Tackaberry <[email protected]> wrote:
> On 14-05-28 07:34 PM, Guido van Rossum wrote: > > I have a feeling that what's really going on here is a subtle bug in > Task._step(): when the coroutine raises CancelledError, it *always* assumes > this means the Task's cancellation was effective, and calls > Future.cancel().But it should only do that as a delayed response to a > cancel() call on the Task -- if the Task was never cancelled, it should > treat the CancelledError as any other exception, and call > self.set_exception() instead. (However, the question is, how long to wait > before we assume the task swallowed the cancellation? _step() can't look > inside the code to see whether it is still in a try/except/finally clause > handling the cancellation...) > > > It feels like we're honing in on my objection. My concern is really about > the current behaviour of silent uncaught implicit cancellations than about > silent uncaught explicit ones. That is, if t1 yields from t2, and t2 is > cancelled, I could understand if that fact isn't logged. But t1 being > cancelled as a consequence of t2 is what I believe should be logged if not > caught. > > It took several readings of your comments above but I think I understand > the issue now. Let me see if I've got it: > > Task.cancel() could set a flag that the task was explicitly cancelled, and > if in the next step the coroutine raises CancelledError and that flag is > set, the cancellation was successful and Future.cancel() is called. But if > the coroutine catches CancelledError, it could yield back arbitrarily. > Maybe it reraises, or maybe it swallows the cancellation and continues on, > and some time later on yields a completely unrelated future that gets > cancelled. That second CancelledError is ambiguous: Task._step() doesn't > know if it's related to the original task cancellation (which was in fact > swallowed). > Right, that's the issue. > One idea: CancelledError instances could take an 'origin' attribute that > indicates the Future that originated the cancellation. > That's not a bad idea (though I'm not sure about the "origin" term). > Then, when Task._step() gets a CancelledError from the coroutine, it can > check the origin attribute to see if it's self. If it is, the cancellation > was successful and Future.cancel() can be called. If not, it should call > Future.set_exception() instead. > Sounds about right. > The disadvantage is that this CancelledError instance would need to be > passed around, or at least the origin attribute preserved through new > instances. This probably starts to look a bit like the patch I drafted for > the custom cancellation feature. > Perhaps. Could I impose on you and ask you to (1) file a bug in the Tulip tracker describing the problem, and (2) develop a patch and send it for my review using Rietveld (codereview.appspot.com)? > > > Specifically, if I have coroutines A -> B -> C, if C is "soft >> cancelled" by some outside supervisory task S, then B handles this case and >> continues on working. If C is "hard cancelled" then B should stop and A >> needs to be aware of this and handle it appropriately. >> [...] >> > I'll think about this more once we have agreement on the first issue. > > > I just thought of a related scenario that may be easier to > explain/understand. > > In the A -> B -> C chain, B isn't able to distinguish between C being > cancelled and B itself being cancelled. It's easy to imagine that B would > want to handle those cases differently. > Yup, that's the scenario we identified above. It's not quite the same as wanting to pass a subclass of CancelledError to cancel() though (which I'm still hesitant to accept). -- --Guido van Rossum (python.org/~guido)
