Have to set current loop myself since Daphne 2.4.1

[Matbj]

Environment

Python 3.6

Problem

Usage of any function that relies on default loop - gotten by running asyncio.get_event_loop()) - such as asyncio.Lock, does not work without grabbing and specifying the loop using

from daphne.server import twisted_loop
lock = asyncio.Lock(loop=twisted_loop)

or by setting it to be the current one

from daphne.server import twisted_loop
asyncio.set_event_loop(twisted_loop)
lock = asyncio.Lock()

Otherwise you get errors looking like this:

RuntimeError: Task <Task pending coro=<MainSocketEndpoint.handle_request() running at ./project-name/websocket/handler.py:49> created at ./project-name/websocket/handler.py:41> got Future <Future pending created at /Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/base_events.py:295> attached to a different loop

Cause

Since version Daphne 2.4.1 the running loop created in
https://github.com/django/daphne/blob/master/daphne/server.py#L7 (twisted_loop = asyncio.new_event_loop())
is no longer fetchable with asyncio.get_event_loop()

This change in 2.4.1 came as a surprise to us and to find all the places the loop has to be specified will be tough considering it is a very common practice to rely on asyncio.get_event_loop(). Many installed packages rely on that. Also making sure tests handle it is difficult. Meaning we have to rely on solution 2 (asyncio.set_event_loop(twisted_loop)).

Solution

Either revert this change:
27f760a

or add
asyncio.set_event_loop(twisted_loop)
after
twisted_loop = asyncio.new_event_loop().

Feedback

Switching from using the default event loop to custom one should probably not be considered to be a minor change.

[carltongibson]

OK, so... first of all it's a bug fix. It's turns out that just calling get_event_loop() doesn't handle running in a separate thread, such as under Django's auto-loader.

Having enquired from the Python core team on this, we should be using get_running_loop() instead. This is documented, but not that clearly I think:

Because this function has rather complex behavior (especially when custom event loop policies are in use), using the get_running_loop() function is preferred to get_event_loop() in coroutines and callbacks. https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.get_event_loop

The long and short of it seemed to be "we would deprecate get_event_loop() but that means we'd have to pull it out in two versions, which is too quick because it's widely used. Use get_running_loop() instead."

Which isn't perfect I know, but, I don't think calling set_event_loop() is correct, since you'd end up (in the same issue of) accessing the event loop from different threads, which is a strict no-no (although where it says that in the docs, I don't know).

SO, can I ask, what are you trying to do, and would get_running_loop() work in your case?

[carltongibson]

Hmmm. asyncio.Lock() is using get_event_loop() itself 🤔

[carltongibson]

OK, so it looks like the adjustment is still in play here.

Via https://bugs.python.org/issue36373, deprecating the loop argument, we have this comment of a PR:

In Python 3.9 I have a plan to add another deprecation warning if not self._loop.is_running() for queues and locks and eventually replace get_event_loop() with get_running_loop() as a final fix. python/cpython#15889 (comment)

So, in the short run you can use Lock(loop=asyncio.get_running_loop()) inside a coroutine, or I guess you'll have to import twsited_loop for the same if you're creating the lock in sync code.

⚠️ Rabbit-hole ahead ⚠️

[Matbj]

asyncio.get_running_loop was introduced in 3.7 so that's not an option for us. But if you're saying set_event_loop() is not an option then we'll have to make sure to specify the loop upon every instantiation of anything the requires the loop. Or do set_event_loop() ourselves instead. We're not using Django, we're using Starlette.

[carltongibson]

But if you're saying set_event_loop() is not an option ...

We can't call this until we're in the thread we're going to use the event loop in, the thread Twisted is running in. (Otherwise we end up accessing the same loop from two threads, which isn't allowed.)

IF you've only got one thread in play, or you know you're already in the correct thread (already in a coroutine running under Daphne) then you can call set_event_loop(). (It's just we can't add it at the import point where we create the twisted_loop.)

...introduced in 3.7 so that's not an option for us...

Grrr. This stuff is moving fast still.

[Matbj]

I want to say thanks for looking into this!

I want to raise the question again. Does it make sense to force users of daphne through Django/Starlette/... to have to import the lock and then specify it when using asyncio.Lock etc? As I mentioned it is a common pattern of many packages to rely on asyncio.get_event_loop. Just to give one example, aiohttp uses this pattern:
https://github.com/aio-libs/aiohttp/blame/master/aiohttp/client.py#L206
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/helpers.py#L274

Even, since this pull request: aio-libs/aiohttp#3580 you are no longer allowed to specify a loop as argument to aiohttp.ClientSession. So only way is to use set_event_loop.

Would it be possible to somewhere in daphne set the event loop with asyncio.set_event_loop to make users not having to think about it?

[carltongibson]

Right, but see aio-libs/aiohttp#3331 — there it's explicitly stated that get_event_loop() will be deprecated, so the aiohttp helper will presumably disappear, once 3.7 is the minimum supported version...

Would it be possible to somewhere in daphne set the event loop ...

I'll have to think about it. It's a tricky one.

[carltongibson]

(If you fancied implementing #264, that would give a good place to add the call at least...)

[carltongibson]

Hi @Matbj — Long time, but now PY36 is end-of-life the prospect here looks slightly clearer...

asyncio.get_running_loop was introduced in 3.7 so that's not an option for us.

This is no longer an issue, and asyncio.get_running_loop() is now more or less universally used, and get_event_loop() is on the way out.

The issue here is doing things — like creating the lock — before we spin up the Server instance.

We already call set_event_loop() when starting the Sever:

daphne/daphne/server.py

Line 139 in 71ba440

asyncio.set_event_loop(reactor._asyncioEventloop)

Anything you do after that point will get the right event loop.

Anything you do before that will need to set the twisted_loop but it's a bit 😬 because this is all necessary in order to run in multi-threaded situations, such as originally with the Django auto-reloader. The bottom line is calling any of these loop requiring functions without a current running loop is liable to go wrong, which is why it's been deprecated.

As such, I'm going to close this as wontfix. I think as asyncio has moved on it's largely become moot. (There are better higher-level APIs available now.) But in any case I think it's easier to address specific cases — saying Defer doing this until ... rather than thinking we can just set the twisted event loop on the main thread at startup, which we already know we can't.

Hope that makes sense. Sorry for the slow resolution, but really there wasn't much to be done until get_running_loop() was used throughout.

Thanks again.