Cancel pending tasks

[FabioRosado]

Description

This is not the right way that the issue 723 suggests we should approach this issue. But I was unsure how to get running tasks because asyncio.Task.all_tasks seems to return all tasks not only pending but also the ones that have been set as done or finished.

I tried to search how to get only the pending tasks but couldn't figure out the best way to do this, since I couldn't figure out I couldn't create a condition to make opsdroid sleep for x amount of seconds before cancelling the task.

This solution does cancel the task but I'm not sure if this is the best way to do things like I said before. Would like to get some more feedback on how to approach the issue in a better way 👍

Fixes #723

Status

READY | UNDER DEVELOPMENT | ON HOLD

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

• Tox passes
• Ran opsdroid, killed it and no exception was raised

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[codecov]

Codecov Report

Merging #729 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #729   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          28     28           
  Lines        1851   1857    +6     
=====================================
+ Hits         1851   1857    +6

Impacted Files	Coverage Δ
opsdroid/core.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea1ed8a...b9ca23a. Read the comment docs.

[jacobtomlinson]

Yeah I also struggled with this. We need to cancel all tasks after a certain timeout (maybe 30 seconds), however this function is being called from within a task so we need to ignore that one. Sleeping for 30 seconds isn't great either as it would keep the thread open.

I think we should be counting the number of running tasks (not including the current task) every 0.1 seconds. If we get to 300 checks then they should be cancelled, if they all complete on their own then we should break out of the loop.

As you said counting the number of running tasks is tricky though. Might be worth a stack overflow question?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[FabioRosado]

I was giving some thought about this issue and started looking into how other projects seem to close/cancel tasks, do you think is it possible to do something like in the aioslacker?

It seems they are adding all tasks(ensure_future creates tasks right?) into a set, with lines 103-105

fut = ensure_future(coro, loop=self.loop)

self.futs.add(fut)

Then when we try to close the slacker the method close uses:

        if self.futs:
            yield from asyncio.gather(*self.futs, loop=self.loop)

        yield from self.session.close()

Perhaps we could add the connector.listen method (this is where most of the issues happen) into a sort of set/list and on the disconnect just disconnect that task?

Another thing we could do is change the connector.disconnect method from being passed to raise NotImplementedError and then when we try to disconnect, if that exception is raised we can just cancel that task?

[jacobtomlinson]

Yeah that all sounds good.

The only other thing to consider is that if someone's skill enters an infinite (or at least long running) loop that task will need to be cancelled too. Perhaps we should track called skill tasks as well.

[petri]

You can just nicely cancel the tasks that are not yet done like this:

        # The extensions have been given a chance to clean up
        # gracefully in disconnect(). We can now cancel what we started.
        pending = asyncio.Task.all_tasks()
        for p in list(pending):
            if not p.done() and p is not asyncio.current_task():
                p.cancel()

[jacobtomlinson]

That was my original plan @petri, however the function is called within a task and it cancels itself. I haven't managed to find a way of not cancelling itself.

[petri]

@jacobtomlinson recheck the code above. It's not canceling itself due to checking for p is not asyncio.current_task().

[jacobtomlinson]

Totally missed that! Nice!

[jacobtomlinson]

@FabioRosado could you please update this?

[FabioRosado]

Just updated the PR, we will probably have an issue with testing as I need to test for task.cancel() but I haven't managed to figure out how to do this properly. I attempted to mock this method in order to see if it was called, but that didn't work out

[FabioRosado]

So in the end testing this was way easier than I was thinking. Should I merge this now @jacobtomlinson ?