Opened 10 years ago

Closed 2 years ago

Last modified 2 years ago

#4632 enhancement closed fixed (fixed)

ability to cascade canceling inlineCallbacks's deferred

Reported by: tracktor Owned by: Kyle Altendorf
Priority: normal Milestone:
Component: core Keywords:
Cc: tracktor, Tom Prince, sda@…, Kyle Altendorf Branch: branches/cascade-4632
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

This example shows that when we cancel inlineCallbacks's deferred, there is no cascade cancelling performed

from twisted.internet.defer import Deferred, inlineCallbacks, returnValue
from twisted.internet import reactor

import threading
import time


def log(s):
    print time.strftime('%H:%M:%S'), s

def deferred_work():
    cancel_flag = [False]
    def my_canceller(d):
        cancel_flag[0] = True
    res = Deferred(canceller=my_canceller)
    def work():
        log('start work')
        cnt = 7
        while cnt:
            cnt -= 1
            time.sleep(1)
            if cancel_flag[0]:
                return
        log('finish work, work not cancelled!!!')
        return res.callback(7)
    threading.Thread(target=work).start()
    return res

@inlineCallbacks
def test1():
    log('start test1')
    res = yield deferred_work()
    log('finish test1, test1 not cancelled!!!')
    returnValue(res)

@inlineCallbacks
def test2():
    log('start test2')
    res = yield test1()
    log('finish test2, test2 not cancelled!!!')
    returnValue(res)

def ok(result):
    log('ok ' + repr(result))

def error(result):
    log('error' + repr(result))

deferred = test2()
deferred.addCallbacks(ok, error)

reactor.callLater(1, deferred.cancel)

reactor.callLater(10, reactor.stop)
reactor.run()

Output

11:42:53 start test2
11:42:53 start test1
11:42:53 start work
11:42:54 error<twisted.python.failure.Failure <class 'twisted.internet.defer.CancelledError'>>
11:43:00 finish work, work not cancelled!!!
11:43:00 finish test1, test1 not cancelled!!!
11:43:00 finish test2, test2 not cancelled!!!

Solution (idea) - some changes in defer.py

def _inlineCallbacks(result, g, deferred):

    ... skipped ...

        if isinstance(result, Deferred):
            # a deferred was yielded, get the result.
            def gotResult(r):
                if waiting[0]:
                    waiting[0] = False
                    waiting[1] = r
                else:
                    deferred._canceller = None # CHANGES: clear canceller
                    _inlineCallbacks(r, g, deferred)

            result.addBoth(gotResult)
            if waiting[0]:
                # Haven't called back yet, set flag so that we get reinvoked
                # and return from the loop
                waiting[0] = False
                # CHANGES: set canceller
                deferred._canceller = lambda d, result=result: result.cancel()
                return deferred

    ... skipped ...

Output after changes

11:41:54 start test2
11:41:54 start test1
11:41:54 start work
11:41:55 error<twisted.python.failure.Failure <class 'twisted.internet.defer.CancelledError'>>

Attachments (4)

twisted-4632.patch (6.9 KB) - added by tracktor 10 years ago.
twisted-4632.2.patch (7.7 KB) - added by tracktor 10 years ago.
twisted-4632.3.patch (11.5 KB) - added by tracktor 10 years ago.
twisted-4632.4.patch (11.4 KB) - added by tracktor 10 years ago.

Download all attachments as: .zip

Change History (58)

Changed 10 years ago by tracktor

Attachment: twisted-4632.patch added

comment:1 Changed 10 years ago by tracktor

Cc: tracktor added
Keywords: review added
Owner: Glyph deleted

See this disscussion about this patch: http://twistedmatrix.com/pipermail/twisted-python/2010-August/022726.html

I have tested this patch on Windows

One test (twisted.news.test.test_database.NewsShelfTests.test_postApproved) was failed to me. But it was failed without my changes too.

Changed 10 years ago by tracktor

Attachment: twisted-4632.2.patch added

comment:2 Changed 10 years ago by tracktor

Found some bad behaviour, fixed in twisted-4632.2.patch

Bad bahaviour was:

When cascade cancellation occurs... and when some child exception occurs when cancellation... this child exception was trapped (consumed).

It is bad.

Fixed in twisted-4632.2.patch

Now only CancelledError will be trapped.

Corresponding test case added.

comment:3 Changed 10 years ago by Glyph

Keywords: review removed
Owner: set to tracktor

Thanks for the patch.

  1. For the test docstrings:
    1. the preferred style in Twisted now is to say "When inlineCallbacks (some condition), it will (something).", rather than "Ensure that", or "Test that" or "Verify that". Especially, avoid saying "Correct" or "erroneous" in test docstrings, because the test needs to specify what "correct" or "erroneous" about a result is in its particular context.
    2. _genExpectCancelling needs a docstring.
    3. startInlineCallbacks's docstring should say what its purpose is. If it just says "see inlineCallbacks" then it should be deleted and replaced with just inlineCallbacks :). (I know you just copied this from _inlineCallbacks, but it set a bad example.)
  2. startInlineCallbacks should start with an underscore: it's a private, internal implementation detail of inlineCallbacks and should not be exposed.
  3. There should be 2 blank lines between methods.
  4. New tests should be named test_fooBarBaz, not testFooBarBaz. (Again, I know that the tests you were looking at were from older code, and therefore did not follow this pattern. Sorry about that.)
  5. The 'state' parameter to inlineCallbacks really needs some documentation.

Because the test docstrings are a bit vague, I'm not quite sure how to evaluate whether all the changes to the inlineCallbacks implementation are really necessary. Hopefully on the second review your improvements to those to specify the behavior more clearly will make it easier to review those.

Again, thanks a lot for your contribution!

comment:4 Changed 10 years ago by tracktor

What about docstring like this?

    def test_cascadeCancelling(self):
        """
        Let's
            _g_ - generator (decorated with inlineCallbacks)
            _d_ - L{defer.Deferred} returned by generator _g_
            _c_ - L{defer.Deferred} awaited by generator _g_ with yield

        When _d_ callbacked or errbacked (or cancelled due to cancel calls
        errback), _c_ will be cancelled immediately.
        """

It is valid?

comment:5 Changed 10 years ago by Jean-Paul Calderone

Can you update the patch with this docstring and address the other review comments as well? :) Also, we use epytext markup, so if you want to reformat it using that, it would be great. Finally, I think "or" appears in that docstring too many times. A test method should test one thing, not three. Perhpas the test needs to be split into three tests.

Thanks!

Changed 10 years ago by tracktor

Attachment: twisted-4632.3.patch added

comment:6 Changed 10 years ago by tracktor

Keywords: review added
Owner: tracktor deleted

Fixed all review comments but "startInlineCallbacks should start with an underscore".

startInlineCallbacks is public see docstring of startInlineCallbacks

New patch attached - twisted-4632.3.patch

comment:7 in reply to:  6 Changed 10 years ago by Glyph

Keywords: review removed
Owner: set to tracktor

Replying to tracktor:

Fixed all review comments but "startInlineCallbacks should start with an underscore".

startInlineCallbacks is public see docstring of startInlineCallbacks

New patch attached - twisted-4632.3.patch

If it's public, then there needs to be a test that calls it directly, rather than via inlineCallbacks. But, I don't think that it is actually public; there doesn't appear to be any reason for any other code to call it. The docstring doesn't make much sense to me, because it doesn't describe what startInlineCallbacks does, or what its parameters are, or what its return value is; it just says that it 'helps you' do something. How does it help?

comment:8 Changed 10 years ago by Jean-Paul Calderone

Also, whether startInlineCallbacks is useful on its own or not, this is a ticket about making inlineCallbacks Deferreds cancellable. Adding other unrelated inlineCallbacks functionality belongs on another ticket.

comment:9 in reply to:  8 ; Changed 10 years ago by tracktor

Replying to glyph:

The docstring ... doesn't describe what startInlineCallbacks does

What about this docstring for startInlineCallbacks?

def startInlineCallbacks(g, deferred):
    """
    WARNING: this function will not work in Python 2.4 and earlier!

    This functions helps you write L{inlineCallbacks}-like decorators
    with some additional behaviour.

    C(startInlineCallbacks) starts generator processing. See L{inlineCallbacks}
    for details about this generator.

    @param g: generator to be processed

    @param deferred: L{Deferred} to be callbacked with generator result

    For example your C(inlineCallbacks)-like decorator may register resulting
    L{Deferred} in some registry. This allows you to control this L{Deferred}s
    afterwards. You may cancel them all when connection lost for example.

    Example code::

        def inlineCallbacksWithRegistry(registry):
            def _decorator(f):
                def unwindGenerator(*args, **kwargs):
                    deferred = Deferred()
                    registry.register(deferred)
                    return startInlineCallbacks(f(*args, **kwargs), deferred)
                return mergeFunctionMetadata(f, unwindGenerator)
            return _decorator
    """
    ...

Replying to exarkun:

Also, whether startInlineCallbacks is useful on its own or not, this is a ticket about making inlineCallbacks Deferreds cancellable. Adding other unrelated inlineCallbacks functionality belongs on another ticket.

Agree. But startInlineCallbacks is a part of inlineCallbacks implementation.

comment:10 in reply to:  9 ; Changed 10 years ago by Glyph

Replying to tracktor:

Replying to glyph:

The docstring ... doesn't describe what startInlineCallbacks does

What about this docstring for startInlineCallbacks?

(...)

OK, that almost explains why someone might ever want to use this function, but it appears that the point is to control the way that Deferreds are generated. I would much prefer to explicitly have control over that by having inlineCallbacks examine the decorated function for an attribute. Then you could accomplish this same thing like this:

def inlineCallbacksWithRegistry(registry):
    def _decorator(f):
        def deferredFactory():
              d = Deferred()
              registry.register(d)
              return d
        f.inlineCallbacksDeferredFactory = deferredFactory
        return inlineCallbacks(f)
    return _decorator

But I'm not entirely convinced that even this much is necessary; you can implement roughly the same thing today (with no added features) by doing:

def inlineCallbacksWithRegistry(registry):
    def decorator(f):
        inlineified = inlineCallbacks(f)
        def unwindGenerator(*a, **kw):
            result = inlineified(*a, **kw)
            registry.register(result)
            return result
        return mergeFunctionMetadata(f, unwindGenerator)
    return decorator

This isn't exactly the same; it will iterate the generator once, so you can't add callbacks to it before it potentially gets called back, but if all you want is asynchronous cancellation, you can ignore that.

If you're still convinced that it should be exposed, you should explain why, but you'd also need to add some unit tests for doing customizations like this.

Replying to exarkun:

Also, whether startInlineCallbacks is useful on its own or not, this is a ticket about making inlineCallbacks Deferreds cancellable. Adding other unrelated inlineCallbacks functionality belongs on another ticket.

Agree. But startInlineCallbacks is a part of inlineCallbacks implementation.

It could easily be used in the implementation first, then exposed later, in a separate ticket, if that's a good idea.

comment:11 in reply to:  10 Changed 10 years ago by tracktor

You are right, I want to control the way that Deferreds are generated.

Replying to glyph:

I would much prefer to explicitly have control over that by having inlineCallbacks examine the decorated function for an attribute.

I like this!

And what about minor changes in this idea - add (*a, kw) in deferredFactory to provide full information to deferredFactory?

Then for example we may implement inlineCallbacks-like decorator for methods of some class that implements some special interface and may using methods of instance (self) during Deferred creation.

def inlineCallbacksMethodWithRegistration(f):
    def deferredFactory(self, *a, **kw):
        d = Deferred()
        self.registerInlineCallbacksMethodDeferred(d)
        return d
    f.inlineCallbacksDeferredFactory = deferredFactory
    return inlineCallbacks(f)

If it is Ok, what should be done?

  1. rename startInlineCallbacks to _startInlineCallbacks
  1. correct docstring of startInlineCallbacks
  1. inlineCallbacks must examine the decorated function for an attribute inlineCallbacksDeferredFactory

Right?

comment:12 Changed 10 years ago by tracktor

  1. add test case of using inlineCallbacksDeferredFactory

comment:13 Changed 10 years ago by tracktor

And yes, this should be another ticket about inlineCallbacksDeferredFactory

Than in this ticket:

  1. rename startInlineCallbacks to _startInlineCallbacks
  1. correct docstring of startInlineCallbacks

It's all.

Right?

Changed 10 years ago by tracktor

Attachment: twisted-4632.4.patch added

comment:14 Changed 10 years ago by tracktor

Keywords: review added
Owner: tracktor deleted
  1. startInlineCallbacks renamed to _startInlineCallbacks
  1. docstring of startInlineCallbacks changed

see twisted-4632.4.patch

comment:15 Changed 9 years ago by Glyph

Owner: set to Glyph
Status: newassigned

Reviewing.

comment:16 Changed 9 years ago by Glyph

Author: glyph
Branch: branches/cascade-4632

(In [30472]) Branching to 'cascade-4632'

comment:17 Changed 9 years ago by Glyph

Keywords: review removed
Owner: changed from Glyph to tracktor
Status: assignednew

I've put the code into a branch, cascade-4632; please base future patches off of this branch.

I added some documentation in an effort to better understand what's going on here, which is committed to that branch.

Now that I've had the opportunity to understand this feature in depth, there are a couple of remaining points that I think should be addressed:

  1. "Let's" is an odd way of saying that. If you want the comments to read like a mathematical proof, I'd say that these comments should instead be formatted like: "Let: - A be a Foo, - B be a Bar" (Would I be correct in guessing that English is not your first language, or is this jargon in some scientific field that I'm not familiar with?)
  2. Making the 'state' parameter be a 2-list is very, very confusing. It should be an object with some attributes and its own docstring to explain why it has the information that it has The 'waiting' variable sets a bad example, but its saving grace is that it is purely a local variable, not passed between functions.
  3. The way that _startInlineCallbacks determines that the Deferred is being cancelled is incorrect. It should create a Deferred with a canceller that explicitly sets the "cancelling in progress" flag. The way that it is currently implemented, external code might also invoke the callback or errback method of a Deferred returned from an inlineCallbacks function. This should not be supported behavior: it would be extremely confusing to have the logic in one's inlineCallbacks method suddenly stop, and the awaited-upon Deferred get cancelled, if it were callbacked or errbacked for some unrelated reason.

(Although I'm reassigning this to you, I will try to address some of these points myself during the remainder of the sprint today, on the assumption that you're not going to have the time to look at it immediately. If the ticket is still out of review by tomorrow, I'm probably not going to be working on it any more, feel free to respond.)

comment:18 Changed 9 years ago by Glyph

(In [30508]) More idiomatic 'let' (re: #4632 review point 1)

comment:19 Changed 9 years ago by Glyph

(In [30509]) More idiomatic 'let's (in tests) (re: #4632 review point 1)

comment:20 Changed 9 years ago by Glyph

(In [30514]) Expand the cancellation status into an instance. re #4632 review point 2

comment:21 Changed 9 years ago by Glyph

(In [30520]) Remove support for .callback() and .errback() by giving the Deferred produced by @inlineCallbacks a cancellation function, thereby simplifying the implementation somewhat. Also, delete some redundant tests which verified this (undesirable) behavior. re #4632 review point 3

comment:22 Changed 9 years ago by Glyph

Okay, actually, I think the semantics implemented by this branch were wrong. Cancelling the Deferred returned by inlineCallbacks ought to cancel the Deferred being waited upon, and let the chips fall where they may; otherwise, you can't really handle errors correctly (perhaps cancellation produced a specific error type, perhaps the error occurred a couple of reactor reactions later).

comment:23 Changed 9 years ago by Glyph

Owner: changed from tracktor to Glyph
Status: newassigned

comment:24 Changed 9 years ago by <automation>

Owner: Glyph deleted

comment:25 in reply to:  22 Changed 9 years ago by tracktor

Would I be correct in guessing that English is not your first language

exactly

Okay, actually, I think the semantics implemented by this branch were wrong. Cancelling the Deferred returned by inlineCallbacks ought to cancel the Deferred being waited upon, and let the chips fall where they may; otherwise, you can't really handle errors correctly (perhaps cancellation produced a specific error type, perhaps the error occurred a couple of reactor reactions later).

On the other hand, let us imagine a situation. Example from telephony.

User calls to our IVR server built on twisted.

Our server must automatically speak to user some information.

It uses some text-to-speach server for doing this (third-party solution that can speak to some RTP connection)

Our server must acquire text-to-speach server connection (number of this connections is limited by purchased license).

Link this connection with user.

Send command to text-to-speach server to start speaking.

All this process (as many other processes too) I like to do as inlineCallbacks generator. This is really great thing!

What if user hangs up soon after I started this process (inlineCallbacks generator)?

I just want to cancel this process. And I want generator to stop and release resources immediately. Other users may call soon and they really need text-to-speach resources (that are limited by purchased license :))

Why I can not stop inlineCallbacks process after I started it?

Nothing is needed this process to continue working any more. But it works and burdens reactor queue.

comment:26 Changed 9 years ago by tracktor

What to do?

Proposal

Let inlineCallbacks marks own Deferred with some marker (deferred.is_inline_callbacks = True) and register canceller.

When inlineCallbacks Deferred cancelled we

  1. stop generator
  1. check Deferred being waited upon about marker is_inline_callbacks == True. And if is_inline_callbacks == True then cancel this Deferred too; otherwise do nothing and "let the chips fall where they may".

comment:27 Changed 9 years ago by Jean-Paul Calderone

I just want to cancel this process. And I want generator to stop and release resources immediately.

It would help if you gave a code example of this. English is ambiguous. My wild guess is that you're talking about some case like this:

@inlineCallbacks def something():

resource = acquireResource() try:

value = yield doSomething()

finally:

resource.release()

d = something()

And you want resource.release() to run as soon as d.cancel() happens, whereas glyph is suggesting that d.cancel() just cancel the Deferred returned by doSomething and rely on that Deferred firing soon to let the inlineCallbacks-decorated generator continue.

Glyph's suggestion makes more sense to me, because doSomething may actually be using that resource, and it may be impossible to release it until it has been cancelled.

Just my wild guess, though. Please provide more details if I'm mistaken.

comment:28 Changed 9 years ago by Jean-Paul Calderone

sigh...

@inlineCallbacks
def something():
    resource = acquireResource()
    try:
        value = yield doSomething()
    finally:
        resource.release()
d = something()

comment:29 Changed 9 years ago by tracktor

Thank you, exarkun! Now I understood what glyph have said :)

glyph is suggesting that d.cancel() just cancel the Deferred returned by doSomething and rely on that Deferred firing soon to let the inlineCallbacks-decorated generator continue

Yes, this is what I need.

This is much better than what I propose all time during this ticket.

Should I realize this solution and produce patch based on this branch?

comment:30 Changed 9 years ago by Jean-Paul Calderone

Should I realize this solution and produce patch based on this branch?

That would be excellent! :) Thank you.

comment:31 in reply to:  30 ; Changed 9 years ago by Glyph

Replying to exarkun:

Should I realize this solution and produce patch based on this branch?

That would be excellent! :) Thank you.

Actually, I believe r30534 (in the branch) already implements the change in semantics that I suggested. Unfortunately I don't remember why I left off without putting this branch into review! Have a look over the code first and see if there's something that I missed.

comment:32 in reply to:  31 Changed 9 years ago by tracktor

Replying to glyph:

Actually, I believe r30534 (in the branch) already implements the change in semantics that I suggested. Unfortunately I don't remember why I left off without putting this branch into review! Have a look over the code first and see if there's something that I missed.

1.

I think this peace of code should be "restored" as just one line "_inlineCallbacks(r, g, deferred, status)"

http://twistedmatrix.com/trac/browser/branches/cascade-4632/twisted/internet/defer.py?rev=30520#L1141

And therefore "status.cancelled" flag become redundant (unnecessary)

2.

Maybe "def deferredGenerator(f)" should have the cascade cancelling semantic too

3.

Maybe DeferredList should overload "def cancel" as cancelling all owned child Deferreds

comment:33 Changed 9 years ago by tracktor

I was watching defer.py in this branch "cascade-4632"

I found this:

    def unwindGenerator(*args, **kwargs):
        g = f(*args, **kwargs)
        def cancel(it):
            it.callbacks, tmp = [], it.callbacks
            it.addErrback(handleCancel)
            it.callbacks.extend(tmp)
            it.errback(_PeculiarError())
        deferred = Deferred(cancel)
        status = _CancellationStatus(deferred)
        def handleCancel(result):
            result.trap(_PeculiarError)
            status.deferred = Deferred(cancel)
            awaited = status.waitingOn
            awaited.cancel()
            return status.deferred
        _inlineCallbacks(None, g, status)
        return deferred
    return mergeFunctionMetadata(f, unwindGenerator)

Why so complex?

I think we was talking about this solution:

    def unwindGenerator(*args, **kwargs):
        g = f(*args, **kwargs)
        def cancel(it):
            awaited = status.waitingOn
            if awaited:
                awaited.cancel()
        deferred = Deferred(cancel)
        status = _CancellationStatus(deferred)
        _inlineCallbacks(None, g, status)
        return deferred
    return mergeFunctionMetadata(f, unwindGenerator)

This works perfect for me

comment:34 Changed 9 years ago by Jonathan Marcus

Sorry for coming to this report a little late.

I'd also like to be able to cancel an inlineCallback, but I have cases where I want to have special cleanup code if I get canceled. How about the following?

@defer.inlineCallbacks
def foo():
    try:
        yield longRunningProcess()
    except defer.CancelledError:
        releaseResource()
        return
    else:
        doOtherStuff()

If my inlineCallbacks Deferred gets canceled, throw a CanceledError into the generator. This way I can explicitly set up my own cancel-handler in a manner consistent with how I already use inlineCallbacks. Right now, "try... except" corresponds to errbacks, "try... else" to callbacks, and "try... finally" to addBoth.

I realize that this isn't perfect - you shouldn't be able to catch a cancellation and continue on with your work. But maybe it is a starting point?

Thanks, Jonathan

comment:35 Changed 8 years ago by tracktor

Hi, Jonathan

I think we should replace defer.CancelledError in this case with other Exception that inherits GeneratorExit.

class InlineCallbacksCancelled(GeneratorExit):
    pass

So we got

@defer.inlineCallbacks
def foo():
    try:
        yield longRunningProcess()
    except defer.InlineCallbacksCancelled:
        releaseResource()
        raise
    else:
        doOtherStuff()

Now we can't catch InlineCallbacksCancelled and continue on with our work because InlineCallbacksCancelled inherits GeneratorExit.

comment:36 Changed 8 years ago by tracktor

My little resume

When we cancel InlineCallbacks Deferred we should perform (in canceller):

  1. cancel awaited deferred - status.waitingOn.cancel()
  1. stop generator by throwing special exception InlineCallbacksCancelled inherited from GeneratorExit. Generator can't catch such exception. We should catch GeneratorExit.
  1. return None from canceller (Deferred will be errbacked with CancelledError)

comment:37 in reply to:  35 ; Changed 8 years ago by Glyph

Replying to tracktor:

Hi, Jonathan

I think we should replace defer.CancelledError in this case with other Exception that inherits GeneratorExit.

No.

This is not consistent with the way that cancelled Deferreds normally handle errors. You should be able to catch CancelledError just as you could add a callback for CancelledError anywhere in a callback chain.

Jonathan - you should be able to catch a cancellation and continue; this is an explicit part of cancel()'s design.

Think of a Deferred cancellation like a SIGTERM. It's not polite to just handle it and keep going, but it's possible, and it's actually important that it be possible, because sometimes you absolutely need to do cleanup for your code to be correct. (We can't have an equivalent to SIGKILL because there's shared state involved. Deferreds are inherently cooperative multitasking.)

comment:38 in reply to:  37 ; Changed 8 years ago by tracktor

Replying to glyph:

This is not consistent with the way that cancelled Deferreds normally handle errors. You should be able to catch CancelledError just as you could add a callback for CancelledError anywhere in a callback chain.

Agree.

Therefore we cancel InlineCallbacks deferred with CancelledError and stop generator by throwing InlineCallbacksCancelled(GeneratorExit) into it.

Jonathan - you should be able to catch a cancellation and continue; this is an explicit part of cancel()'s design.

Think of a Deferred cancellation like a SIGTERM. It's not polite to just handle it and keep going, but it's possible, and it's actually important that it be possible, because sometimes you absolutely need to do cleanup for your code to be correct. (We can't have an equivalent to SIGKILL because there's shared state involved. Deferreds are inherently cooperative multitasking.)

I like these words.

What about stop generator by throwing InlineCallbacksCancelled inherited from common Exception (not GeneratorExit)?

Why I need this?

I want distinguish different cases inside generator:

@defer.inlineCallbacks
def foo():
    try:
        yield longRunningProcess()
    except SomeError:
        # case 1: longRunningProcess() errbacked with SomeError.
        pass
    except CancelledError:
        # case 2: longRunningProcess() cancelled.
        #         But foo() is alive (not cancelled).
        #         foo() may continue on.
        pass
    except InlineCallbacksCancelled:
        # case 3: foo() cancelled.
        #         longRunningProcess() is cancelled too by inlineCallbacks but this
        #           is not our ​​responsibility, inlineCallbacks will care about this.
        #         foo() should clean up and exit here.
        pass

comment:39 in reply to:  38 Changed 8 years ago by Glyph

Replying to tracktor:

I want distinguish different cases inside generator:

You can't actually distinguish these cases though; and there's no reason that you would need to.

First, notice how longRunningProcess doesn't get returned anywhere but into the yield? That Deferred can effectively only be cancelled by the caller of foo. There's no handle to that Deferred to cancel it, except internally to longRunningProcess, which would be indistinguishable from just any other random errback.

Second, .cancel() will not necessarily raise CancelledError at all. If you cancel a multi-step operation halfway through, it may translate that error into something more suitable for a different layer of the stack. Some APIs may want guarantees that don't involve throwing CancelledError up through the whole errback chain. (This is the whole point of an errback, after all.)

Third, there's no way to cancel() an inlineCallbacks function on its own: such a function can't do something I/O bound, it can only wait on other Deferreds. Therefore the only reasonable thing for .cancel() to do is to cascade (hence this ticket) and there's no particular reason to want to be able to tell whether the user clicked a button that happened to be hooked up to the caller of the inlineCallbacks method that you're writing or something further down the stack.

In some cases, letting code tell the difference between two cases which absolutely should be the same is just inviting more bugs. I'm pretty sure this is one of those cases. Plus, mixing in the bonus semantics of GeneratorExit, which is extra hard to catch, seems to violate the spirit of inlineCallbacks's operation, which generally avoids playing tricks with the generator-ness of it all, with the sole exception of returnValue.

Finally, anyone really interested in having these semantics could wrap their @inlineCallbacks'd function up in another decorator which has a Deferred with a custom canceler. This is subtle and tricky, but you shouldn't really want to do it, so that's good :).

If you have a more detailed use-case that involves an actual application, and not just "I'd like to trap this type of exception differently", please feel free to correct me, though.

comment:40 Changed 8 years ago by tracktor

Ok, I got it! I think I can solve all my use-cases in this semantic. Thank you for educating.

Simple example of use-cases.

We have some event (incoming message from net). We can wait for this event. Event may occur. We can cancel to wait event by timeout.

# function to wait for the event, returns new Deferred
def wait_event():
    ...

# function to emit event (when event occurs), callbacks all awaiting Deferreds
def on_event_occurs(value):
    ...

# function to set timeout if we don't want to wait for the event infinitely
def set_waiter_timeout(waiter, timeout):
    ...

Wrong solution:

# global registry of waiters (who awaits the event)
_waiters = set() # set of (Deferred)s

# canceller for waiters
def _waiter_canceller(waiter):
    _waiters.remove(waiter)

# function to wait for the event, returns new Deferred
def wait_event():
    waiter = Deferred(_waiter_canceller)
    _waiters.add(waiter)
    return waiter

# function to set timeout if we don't want to wait for the event infinitely
def set_waiter_timeout(waiter, timeout):
    reactor.callLater(timeout, waiter.cancel)
    return waiter

# function to emit event (when event occurs)
def on_event_occurs(value):
    while _waiters:
        waiter = _waiters.pop()
        waiter.callback(value)
        # we should also cleanup timeouts here but omit this for simplicity

Usage

@defer.inlineCallbacks
def foo():
    try:
        yield set_waiter_timeout(wait_event(), 10)
    except CancelledError:
        # Who is cancelled?
        # foo() or wait_event()?

Problems of this solution

  1. some third party can cancel Deferred (set_waiter_timeout in this example)
  2. we can't distinguish different cases. But we need it in foo().
  3. there is a wish to cancel Deferred with some other exception than CancelledError. With exception like TimeoutError. And we want to do this not in canceller but in set_waiter_timeout. Third party wants this.

Right solution:

# global registry of waiters (who awaits the event)
_waiters = set() # set of (Deferred)s

# global registry of timeout timers
_timeout_timers = {} # Deferred -> timer

# cleanup for waiters
def _waiter_cleanup(waiter):
    _waiters.remove(waiter)
    timer = _timeout_timers.pop(waiter, None)
    if timer is not None and timer.active:
        timer.cancel()

# function to wait for the event, returns new Deferred
def wait_event():
    waiter = Deferred()
    waiter.addBoth(_waiter_cleanup, waiter)
    _waiters.add(waiter)
    return waiter

# function to set timeout if we don't want to wait for the event infinitely
def set_waiter_timeout(waiter, timeout):
    timer = _timeout_timers.pop(waiter, None)
    if timer is not None:
        timer.cancel()
    timer = reactor.callLater(timeout, waiter.errback, TimeoutError)
    _timeout_timers[waiter] = timer
    return waiter

# function to emit event (when event occurs)
def on_event_occurs(value):
    while _waiters:
        waiter = _waiters.pop()
        waiter.callback(value)

Usage

@defer.inlineCallbacks
def foo():
    try:
        yield set_waiter_timeout(wait_event(), 10)
    except TimeoutError:
        # wait_event() is cancelled
    except CancelledError:
        # foo() is cancelled

Now set_waiter_timeout is not a third party. It is a part of whole mechanism.

Am I right?

comment:41 Changed 8 years ago by tracktor

Sorry, I made mistake in "Right solution"

result is forgotten in _waiter_cleanup

def _waiter_cleanup(result, waiter):
    ...
    return result

comment:42 Changed 7 years ago by Tom Prince

Cc: Tom Prince added

comment:43 Changed 3 years ago by Benoit Gagnon

I was bitten by this issue after discovering what my carefully implemented Deferred canceller logic was being ignored deep down in an @inlineCallbacks generator.

I want to bring attention to the txcoroutine [1] library which has been of great help in providing a drop-in solution to this problem.

I also tested the patch [2] from this ticket. I can confirm that it propagates the cancel() call to the currently yielded Deferred, just like @coroutine from [1].

In addition to wishing Twisted supported this natively, I also wish it would have complained on trying to invoke cancel() on the @inlineCallbacks top-level Deffered, and offered some documentation.

Is there interest in reviving this old ticket and/or absorbing the ideas from [1]? Would a patch documenting the @inlineCallbacks limitations be of service?

[1] https://github.com/eallik/txcoroutine [2] https://github.com/twisted/twisted/compare/trunk...cascade-4632

Last edited 3 years ago by Benoit Gagnon (previous) (diff)

comment:44 Changed 3 years ago by Glyph

Owner: set to Benoit Gagnon
Status: assignednew

Is there interest in reviving this old ticket and/or absorbing the ideas from [1]? Would a patch documenting the @inlineCallbacks limitations be of service?

Yes! There is absolutely interest. Thank you for volunteerIng! If you have any other questions about contributing, feel free to ask.

comment:45 Changed 2 years ago by Kyle Altendorf

Cc: sda@… added
Keywords: review added
Owner: Benoit Gagnon deleted

https://github.com/twisted/twisted/pull/1001

Thanks to Glyph for giving me a branch to resurrect.

comment:46 Changed 2 years ago by Kyle Altendorf

Keywords: review removed
Owner: set to Kyle Altendorf

Sorry, jumped ahead. Time to fix the py3 failures.

comment:47 Changed 2 years ago by Kyle Altendorf

Keywords: review added
Owner: Kyle Altendorf deleted

https://github.com/twisted/twisted/pull/1001

Ready for a first pass review I think.

comment:49 Changed 2 years ago by Kyle Altendorf

Cc: Kyle Altendorf added
Keywords: review added
Owner: set to mark williams

I think I addressed all of the requests. For the big one about adding the explanation I wasn't sure how much you wanted added so I reviewed all the logic you explained and then added a small comment. Let me know if you want a lot more explained in the docstrings.

comment:51 Changed 2 years ago by mark williams

Owner: changed from mark williams to Kyle Altendorf

comment:53 Changed 2 years ago by Mark Williams <mrw@…>

Resolution: fixed
Status: newclosed

In 9d6dbd7e:

Merge pull request #1001 from altendky/4632-altendky-catchup_cascade_4632

Author: tracktor, glyph, altendky

Reviewers: markrwilliams

Fixes: ticket:4632

Cascading cancellation for inlineCallbacks

comment:54 Changed 2 years ago by Glyph

This is an EPIC ticket, thank you SO MUCH to everyone who participated!

The next release is going to be amazing!

🎉

Note: See TracTickets for help on using tickets.