Cancellation of the handlers set to the "ready" event of an asyncio component are not properly handled

[rdebroiz]

After cancelling the handlers associated with the "ready" event a _GatheringFuture exception holding the CancelledError is raised but never retrieved.

@comp.on_ready
async def go(*args, **kwargs):
    while True:
        await asyncio.sleep(1)

run(comp)

gives, after the user initiated a shutdown:

2019-02-22T16:54:47 connecting once using transport type "websocket" over endpoint "tcp"
2019-02-22T16:54:50 Shutting down due to SIGINT
2019-02-22T16:54:50 _GatheringFuture exception was never retrieved
future: <_GatheringFuture finished exception=CancelledError()>
concurrent.futures._base.CancelledError

Just await the handlers solves the issue:

# in autobahn.util.ObservableMixin.fire
for handler in self._listener
    future = txaio.as_future(
    
    def consume_result(fut):
        return fut.result()
    
    future.add_callbacks(consume_result)
    res.append(future)

Related to #1000

[rdebroiz]

Actually I made a mistake and type add_callbacks instead of add_done_callback this raised an Exception which prevented the gather of all the handlers at the end of the function to be called.

Ironically what I did by mistake is exactly what I thought was the problem.

I don't understand what's the cause of the _GatheringFuture exception not being handled. It seems to be specific to the "ready" event.

[sprocket-9]

Looks like the problem is in the on_* handlers callback chain in autobahn.wamp.protocol.ApplicationSession and the differences in behaviour of Twisted's Deferred and asyncio Future objects which using txaio can't entirely abstract away.

Deferred's allows switching back and forth between callback and errback chains so any Exception raised later in the chain from user code is taken care of by the current code.

Once a Future is done with a successful result as happens here https://github.com/crossbario/autobahn-python/blob/master/autobahn/wamp/protocol.py#L514 only the callback chain on this Future will run. An Exception raised later in the chain when firing the 'ready' event ' https://github.com/crossbario/autobahn-python/blob/master/autobahn/wamp/protocol.py#L535 will not be handled by the next errback so the Exception from the GatheringFuture is never retrieved.

[rdebroiz]

Thanks for your clarifications.

So if I understand well, a solution could be to add add a callback directly to the future returned by self.fire('ready', self) to handle its errors ?

something more or less like:

# this instance is now "ready"...
def create_ready_future(_):
    fut = self.fire('ready', self)
    if txaio.using_asyncio:
        def handle_errors(f):
            try:
                return f.result()
            except asyncio.CancelledError:
                pass  # maybe handle it as any other exception
            except Exception as e:
                self._swallow_error(txaio.create_failure(e), "While notifying 'ready'")
        fut.add_done_callback(handle_errors)
    return fut

txaio.add_callbacks(
    d,
    create_ready_future,
    None
)

[sprocket-9]

It's not just the 'ready' event but the whole callback chain that needs broken up. Using _swallow_error in the middle of an errback chain causes twisted and asyncio to follow different paths in the chain. I've made an attempt to workaround this problem https://github.com/spr0cketeer/autobahn-python/tree/session-lifecycle-futures-mod

There may well be a better way than this as I found it quite tricky to come up with a working solution - possibly finding an alternative to error handling and not using _swallow_error is the answer - it does go against the warning in txaio around errbacks https://txaio.readthedocs.io/en/latest/api.html#txaio.add_callbacks although to be fair its docstring does come with its own warning "Note that it cancels the error, so use with care!" but this only applies to twisted.

Anyway, please try the fix out and see if it solves your problem. If it does I'll make it a PR.

[meejah]

Hmm "different paths for Twisted vs asyncio" sounds bad. Are you saying that the Autobahn _swallow_error helper doesn't cancel the error in asyncio?

Also, yeah _swallow_error is intended to be an internal helper hence the _ prefix .. it's sort of the "last resort" thing. My intent there was that errors that otherwise would be completely unhandled are at least logged. That is, I don't see anything else that Autobahn can do here (and the "real" solution is that the user-code in question should be handling the error sensibly, if there's something better than "log it and carry on" to be done).

[meejah]

@spr0cketeer that branch looks like it moves a chunk of code around too, can you point me to the relevant changes..?

[rdebroiz]

@spr0cketeer I tested your branch, it does not solves the problem, the _GatheringFuture exceptions are still not handled. but the behavior is now the same for both join and ready events

@meejah (maybe not really related but not sure an issue is needed):
When one stop a component the callback attached to component.start is called as soon as the handler attached to the leave event have been completed but not the ones attached to the disconnect event. This might be a problem because if you want to properly stop the components on shutdown for instance there is no way to guarantee that the disconnect handlers have been completed before you stop the loop.

Is it the the expected behavior or is it a bug in the asyncio callbacks chain?

[sprocket-9]

@meejah:

Hmm "different paths for Twisted vs asyncio" sounds bad. Are you saying that the Autobahn _swallow_error helper doesn't cancel the error in asyncio?

Yes - try raising an Exception in user code on_join handler and you'll see it logged 3 times by _swallow_error in the errback chain.

@spr0cketeer that branch looks like it moves a chunk of code around too, can you point me to the relevant changes..?

It's a rewrite of this:
https://github.com/crossbario/autobahn-python/blob/master/autobahn/wamp/protocol.py#L469-L550

To this:
https://github.com/spr0cketeer/autobahn-python/blob/session-lifecycle-futures-mod/autobahn/wamp/protocol.py#L472-L570

[sprocket-9]

@rdebroiz:

@spr0cketeer I tested your branch, it does not solves the problem, the _GatheringFuture exceptions are still not handled. but the behavior is now the same for both join and ready events

Hmm weird I'm not seeing the _GatheringFuture problem with my fix. Are you sure you've checkout out the session-lifecycle-futures-mod branch in my repo? A common mistake I often make is cloning the repo but forgetting to switch to the branch containing the actual fix!

If you are in the correct branch then let me know your test for showing it so I can reproduce. Mine is raising an Exception on on_join handler and pressing ctrl-c.