[Django] #32798: StreamingHttpResponse raises SynchronousOnlyOperation in ASGI server
Django
------------------------------------------+------------------------
Reporter: Ralph Broenink | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
When using a ASGI-compliant server, such as Daphne,
StreamingHttpResponse's iterator will be executed in an asynchronous
context, preventing database queries from being performed (raising
SynchronousOnlyOperation). This is not expected behaviour, and appears not
to be documented.
*Steps to reproduce*
1. Create a simple project and app. Set-up for use with Channels. We are
not going to use this component and the bug does seem to originate from
Django itself, but Channels ensures that runserver is ASGI-compliant.
2. Create the following view:
{{{
from django.contrib.contenttypes.models import ContentType
from django.http import StreamingHttpResponse
def test_view(request):
def generate():
yield "hello\n"
list(ContentType.objects.all())
yield "bye\n"
return StreamingHttpResponse(generate(), content_type="text/plain")
}}}
3. Open the page served by test_view
4. Observe the following trace:
{{{
Exception inside application: You cannot call this from an async context -
use a thread or sync_to_async.
Traceback (most recent call last):
File "venv/lib/python3.8/site-packages/channels/staticfiles.py", line
44, in __call__
return await self.application(scope, receive, send)
File "venv/lib/python3.8/site-packages/channels/routing.py", line 71, in
__call__
return await application(scope, receive, send)
File "venv/lib/python3.8/site-packages/django/core/handlers/asgi.py",
line 168, in __call__
await self.send_response(response, send)
File "venv/lib/python3.8/site-packages/django/core/handlers/asgi.py",
line 242, in send_response
for part in response:
File "channelstest/testapp/views.py", line 9, in generate
list(ContentType.objects.all())
File "venv/lib/python3.8/site-packages/django/db/models/query.py", line
287, in __iter__
self._fetch_all()
File "venv/lib/python3.8/site-packages/django/db/models/query.py", line
1303, in _fetch_all
self._result_cache = list(self._iterable_class(self))
File "venv/lib/python3.8/site-packages/django/db/models/query.py", line
53, in __iter__
results = compiler.execute_sql(chunked_fetch=self.chunked_fetch,
chunk_size=self.chunk_size)
File "venv/lib/python3.8/site-
packages/django/db/models/sql/compiler.py", line 1152, in execute_sql
cursor = self.connection.cursor()
File "venv/lib/python3.8/site-packages/django/utils/asyncio.py", line
24, in inner
raise SynchronousOnlyOperation(message)
django.core.exceptions.SynchronousOnlyOperation: You cannot call this from
an async context - use a thread or sync_to_async.
}}}
This error is probably caused by the fact that Django 3 now actively
prevents this kind of error (it would have gone undetected in Django 2)
and the fact that the iterator is called in an asynchronous context in
handlers/asgi.py.
As mentioned above, this issue should at the very least be documented in
the documentation, but preferably should be resolved altogether.
--
Ticket URL: <https://code.djangoproject.com/ticket/32798>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Ralph Broenink | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 3.2
Keywords: ASGI, async | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* cc: Andrew Godwin (added)
* keywords: => ASGI, async
* stage: Unreviewed => Accepted
Comment:
Yes, this won't work. It's not the streaming response (the block this is
raised from explicitly handles that) but rather the ORM call.
> This is not expected behaviour, and appears not to be documented.
I think (currently) it is the expected behaviour. It's covered in the
[https://docs.djangoproject.com/en/3.2/topics/async/#async-safety Async
safety] section of the Asynchronous support docs.
Hmmm 🤔 but yes, it's a limitation on the async wrapping of synchronous
views… I'll accept for now because I can't quite see a better place to pin
it. It's likely this will not be resolved until we have a better async
story for the ORM, but that's not yet at the ticketed stage. (If we had
such a ticket open, I'd say it's a duplicate of that.)
I'll cc Andrew so it crosses his radar as a target use-case.
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:1>
Django
Reporter: Ralph Broenink | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 3.2
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Godwin):
Yeah, this is an unfortunate confluence of the way that generators get
called (and where they get called) in the response stack - they're called
as sync generators, not async, but also are run in an async thread.
I suspect we should probably look at running any response generators
inside sync_to_async, unless we can detect they're an asynchronous
generator somehow and then use `async for` instead of `for`?
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:2>
Django
Reporter: Ralph Broenink | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 3.2
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael Brown):
I suspect we should probably look at running any response generators
inside sync_to_async, unless we can detect they're an asynchronous
generator somehow and then use `async for` instead of `for`?
Even if we detect async vs sync generators at some point, I think response
generators should be called inside `sync_to_async`. Database calls in sync
generators work in WSGIHandler, so it should also work in ASGIHandler. I
can see a usecase for async response generators, but I think that's a
different issue.
As for how to change `for` to `async for`, it would be nice if
`sync_to_async` handled async generators, but according to
[https://github.com/django/asgiref/issues/142 asgiref issue #142] this
isn't the case. The pull request for that issue does something like call
`next` in `sync_to_async`, with a workaround to convert `StopIteration` to
`StopAsyncIteration`:
{{{#!python
async def _sync_to_async_iterator(iterator):
iterator = iter(iterator)
def _next():
try:
return next(iterator)
except StopIteration:
raise StopAsyncIteration
while True:
try:
yield await sync_to_async(_next)()
except StopAsyncIteration:
break
}}}
Would it be more appropriate to use a queue here? Here is an
implementation using a queue, but it's more complicated so I'm less sure
it's correct (although it does work):
{{{#!python
async def _sync_to_async_iterator(iterator):
q = asyncio.Queue(maxsize=1)
def sync_iterator():
for item in iterator:
async_to_sync(q.put)(item)
task = asyncio.create_task(sync_to_async(sync_iterator)())
try:
while not task.done():
next_item = asyncio.create_task(q.get())
done, pending = await asyncio.wait([next_item, task],
return_when=asyncio.FIRST_COMPLETED)
if next_item in done:
yield next_item.result()
elif task in done:
next_item.cancel()
break
finally:
task.cancel()
task.result()
}}}
If one of those options seems good then I can work on a patch with tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:3>
Django
Reporter: Ralph Broenink | Owner: nobody
Type: Bug | Status: new
Component: HTTP handling | Version: 3.2
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Godwin):
Separating these out as two issues:
- Generators should indeed be called within sync_to_async for now, as a
safety measure. This is a patch I'd like to see in Django.
- At some point in future, maybe we can detect async generators and
correctly handle them in both async and sync modes, but that can be an
optional extra for some future time.
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:4>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: assigned
Component: HTTP handling | Version: 3.2
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Brown):
* owner: nobody => Michael Brown
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:5>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: assigned
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Brown):
* has_patch: 0 => 1
Comment:
Created a pull request: https://github.com/django/django/pull/14526
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:6>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: assigned
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by th3hamm0r):
Fyi: I stumbled over this error when running wagtail with asgi, as it is
using `StreamingHttpResponse` for its db-backed CSV-export (see
https://github.com/wagtail/wagtail/blob/c6017abca042031fb2f7931b406a4c12e7a9e8a4/wagtail/admin/views/mixins.py#L201).
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:7>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: assigned
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael Brown):
New pull request for the simple fix of wrapping the streaming response
code in `sync_to_async` and using `sync_to_async(send)`.
https://github.com/django/django/pull/14652
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:8>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: assigned
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_tests: 0 => 1
Comment:
The proposed fix is ''slow'' — but it's not clear exactly how slow. I'm
going to put together a benchmark with locust so we can decide properly
it's fast-enough, or if we'd be better erroring in this case, with a
helpful error messages saying ≈''"Use WSGI here"''.
[https://github.com/django/django/pull/14652#pullrequestreview-773806647
Comment on PR to that effect.]
I'll mark as ''Needs tests'' for the moment.
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:9>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: assigned
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Janzen):
* cc: Jon Janzen (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:10>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Component: HTTP handling | Version: 3.2
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => duplicate
Comment:
To simplify things I'm going to mark this as a duplicate of #33735 (which
should be resolved by adding `__aiter__` to `StreamingHTTPResponse`)
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:11>
Django
Reporter: Ralph Broenink | Owner: Michael Brown
Type: Bug | Status: closed
Keywords: ASGI, async | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carles Pina Estany):
* cc: Carles Pina Estany (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32798#comment:12>