django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.47k stars 209 forks source link

Compatibility with gevent monkey-patching? #443

Closed tildedave closed 8 months ago

tildedave commented 8 months ago

Hello, we run a gevent + channels project in production and were running infrequently into a combination of the following issues:

Their root cause is the same; the asgiref async_to_sync thread doesn't get spawned as a new thread. Instead, the gevent monkey-patching process makes it so asgiref spawns a greenlet instead of a native thread, and the event loop is added to the current thread.

When the event loop's been created in the current thread a bunch of code that's sanity checking for existing event loops fails. The django error above is because Django checks the current thread for a running event loop during ORM statements and throws an error if it finds one. async_to_sync itself throws an error if it finds a running event loop in the current thread.

Here's a self-contained script that reproduces the problem. I'm using the newest asgiref (3.7.2) and gevent (24.2.1), under Python 3.11.5.

import gevent
from gevent import monkey

monkey.patch_all()

from asgiref import sync

async def async_fn_that_yields():
    gevent.sleep(1)

def g():
    print('Running greenlet')
    sync.async_to_sync(async_fn_that_yields)()
    print('Done running greenlet')

gevent.joinall([
    gevent.spawn(g),
    gevent.spawn(g),
])

This errors out with a bunch of errors like:

Traceback (most recent call last):
  File "src/gevent/greenlet.py", line 908, in gevent._gevent_cgreenlet.Greenlet.run
  File "/Users/dking/atlassian/chartio-castle/gevent_and_asgiref.py", line 15, in g
    sync.async_to_sync(async_fn_that_yields)()
  File "/Users/dking/.virtualenvs/py3.11.4/lib/python3.11/site-packages/asgiref/sync.py", line 257, in __call__
    loop_future.result()
  File "/usr/local/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/concurrent/futures/_base.py", line 449, in result
    return self.__get_result()
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/local/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/concurrent/futures/_base.py", line 401, in __get_result
    raise self._exception
  File "/usr/local/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dking/.virtualenvs/py3.11.4/lib/python3.11/site-packages/asgiref/sync.py", line 297, in _run_event_loop
    loop.run_until_complete(gather())
  File "/usr/local/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/base_events.py", line 629, in run_until_complete
    self._check_running()
  File "/usr/local/Cellar/python@3.11/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/base_events.py", line 590, in _check_running
    raise RuntimeError(
RuntimeError: Cannot run the event loop while another loop is running

Changing asgiref to use the gevent.threadpool.ThreadPoolExecutor - which always spawns in a native thread - fixes my script, though of course I don't think asgiref should make a change like this, and it also blocks forever when I run it with our actual application code :-)

I don't think it's particularly fair to ask asgiref to have some gevent-specific logic, but I'm interested to understand what you'd recommend - running under gevent monkey patching seems like a pretty common deployment pattern. Thanks!

andrewgodwin commented 8 months ago

I'm not sure there's really a lot we can do here - gevent's monkeypatching mode has always been a bit of a nightmare for compatibility and it would seem this is no exception. If you can figure out a reliable way to detect that we're running inside gevent without depending or importing gevent and have a fix we can apply in that situation, that's something we could consider adopting, otherwise... maybe see if gevent will consider adding a monkeypatch for asgiref alongside everything else they patch? 😄