asgiref: Excessive use of asgiref.local and sync_to_async breaks other context local implementations
I am writing on behalf of my employer, sentry.io.
I am not sure whether this issue should be filed against Django or asgiref. Starting with Django 3.1, signal handlers for got_request_exception are executed with the wrong contextvars context when running as part of an ASGI request. This can be easily demonstrated by wrapping the Django ASGI app in a middleware that stores something on the context, and trying to retreive that same value from that signal handler.
This is how I became aware of Django’s custom context local implementation that is shipped as part of asgiref. I understand why you do the things you do, but consider that this will break a lot of tracing tooling for Python that may only be orthogonal to Django or ASGI. The situation around context locals is already awful enough, it would be terrible if Sentry and tools like it had to implement a codepath specifically for Django 3 applications, or anything that uses asgiref.local.
I can see the following options:
- avoid releasing Django 3.1 with whatever changes just happened that made errorhandlers execute in the wrong
contextvarscontext. Considering that Django 3.1 is already in alpha I would at least consider this as a stopgap measure. - modify asgiref.sync_to_async and its inverse to “drive” contextvars if they are available such that contextvars always behave identically to asgiref.local
- drop asgiref.local entirely and swap it out for the
aiocontextvarspackage on PyPI, as the primary purpose of implementing asgiref.local from scratch seemed to have been to support Python 3.6. One would still have to implement 2) for this to work, but the advantage here is that aiocontextvars seems to be significantly more popular across frameworks than asgiref.local.
Those options are not mutually exclusive.
Thanks for considering.
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 25 (25 by maintainers)
I know, we’ll have to work it out on the Django side. I’m tempted to change the requirement to include a
~=3.2and a>=3.2.8.(also this might encourage me to finally put the Django test suite as part of tests for asgiref on here)
So, the problem is one of backwards-compatibility.
contextvarsdoes not provide quite the same interface asthreading.Localdid; I tried to port things to using onlycontextvarsbut this would basically make it impossible to sensibly call most of the Django APIs across the sync/async boundary, as well as ruining backwards-compatible behaviour expected of connections and other local-backed items by synchronous middleware.Specifically, Django’s ability to run a request context partially in a fresh sync thread, partially in an existing sync thread that is reserved by thread_sensitive, and partially in an async thread is not something I could get contextvars to solve. It’s possible I didn’t stare at the problem long enough, but I did spend six months trying to get everything to work. If we wanted to rewrite half of Django core and break backwards compat we could definitely do it, but that is, of course, not going to happen.
Going back to your original problem - we should be preserving the outside
contextvarscontext from within any sync_to_async / async_to_sync calls, so I’d consider that a bug we can fix. My general goal here is to have Django treat contextvars pretty normally (passing the context around between threads, obviously, but in the way the python module tells you to) and only have to really mess with the threading-style API, and then deprecate use of that over time.