Opsdroid eventloop breaks if anyio.run() is called while there is an …

[btierneyc]

…eventloop. According to AnyIO docs, anyio.run() must not have an event loop running in the current thread, which currently happens since loopless in the init function is False. Identified this bug from debugging a slack connector, which pointed to this line as the culprit.

Description

Initially found error from trying to connect to Slack via websockets. Upon configuring Slack websockets as true, Opsdroid hangs. Upon further investigation, the webserver was also down, and figured out that the event loop was not working as intended. Inside the init function there is a loopless parameter that is defaulted to False, and so assume that opsdroid intends to loop and use the event loop. However anyio documentation states that "The current thread must not be already running an event loop." in order to use anyio.run() method. Changing the anyio code back to the original eventloop code fixes this issue and allows slack integration.

Did not revert any of the anyio changes made for the tests, just the instance found where it would affect runtime.

Status

READY

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

• Test A
• Test B

Tested with Slack Websockets configured as true in the configuration and with appropriate tokens.

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

[pmaresca]

Shoot, yeah looks like that does cause problems for socket mode. I attempted to get test suite working utilizing anyio without touching any of actual project code but there was one test scenario in particular that I could not rectify without doing something about the presence of the event loop. If you can come up with a better solution until opsdroid can be converted completely to anyio that loopless option can probably be backed out completely.

___________________________________________________________________________________________ TestConfiguration.test_schema ____________________________________________________________________________________________

self = <tests.test_configuration.TestConfiguration testMethod=test_schema>

    def test_schema(self):
        skill_path = "opsdroid/testing/mockmodules/skills/schema_skill"
        example_config = {
            "connectors": {"websocket": {}},
            "skills": {"test": {"path": skill_path}},
        }
        with OpsDroid(config=example_config, loopless=True) as opsdroid:
>           opsdroid.sync_load()

tests/test_configuration.py:45:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <opsdroid.core.OpsDroid object at 0x143957be0>

    def sync_load(self):
        """Run the load modules method synchronously."""
>       self.eventloop.run_until_complete(self.load())
E       AttributeError: 'NoneType' object has no attribute 'run_until_complete'

opsdroid/core.py:223: AttributeError

I think this test will certainly fail if you run it yourself.

[pmaresca]

so maybe this will do until anyio can be project wide?

    def sync_load(self):
        """Run the load modules method synchronously."""
        # We are trying to run opsdroid without an event loop through anyio
        # for testing purposes
        if self.eventloop is None:
            anyio.run(self.load)
        else:
            # We are running in the normal asyncio context
            self.eventloop.run_until_complete(self.load())

@jacobtomlinson what do you think? Maybe a log warning/documentation that loopless is a gap measure and not recommended? I don't feel awesome about adding this stuff but I don't really have time to push for full anyio right now.

[jacobtomlinson]

I don't have strong feelings here. It would be awesome to move towards anyio everywhere, the async code here is very old so theres a lot of things that could be improved, task groups for a start would be very useful here.

Maybe we should just rever for now?

[pmaresca]

I would be fine reverting it but that still means that sync loading test will need to be addressed somehow or expected to fail until full anyio happens. Whatever smells the best to you works for me. I'm not sure just how much work full anyio is but maybe I'll have a poke around if you want to generate an issue for it.