Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#32798 closed Bug (duplicate)

StreamingHttpResponse raises SynchronousOnlyOperation in ASGI server

Reported by: Ralph Broenink Owned by: Michael Brown
Component: HTTP handling Version: 3.2
Severity: Normal Keywords: ASGI, async
Cc: Andrew Godwin, Jon Janzen, Carles Pina Estany Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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")
  1. Open the page served by test_view
  2. 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.

Change History (12)

comment:1 by Carlton Gibson, 3 years ago

Cc: Andrew Godwin added
Keywords: ASGI async added
Triage Stage: Unreviewed → Accepted

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 ​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.

comment:2 by Andrew Godwin, 3 years ago

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?

comment:3 by Michael Brown, 3 years ago

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 ​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:

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):

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.

comment:4 by Andrew Godwin, 3 years ago

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.

comment:5 by Michael Brown, 3 years ago

Owner: changed from nobody to Michael Brown
Status: new → assigned

comment:6 by Michael Brown, 3 years ago

Has patch: set

comment:7 by Stefan Hammer, 3 years ago

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).

comment:8 by Michael Brown, 3 years ago

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

comment:9 by Carlton Gibson, 3 years ago

Needs tests: set

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". ​Comment on PR to that effect.

I'll mark as Needs tests for the moment.

comment:10 by Jon Janzen, 2 years ago

Cc: Jon Janzen added

comment:11 by Carlton Gibson, 2 years ago

Resolution: → duplicate
Status: assigned → closed

To simplify things I'm going to mark this as a duplicate of #33735 (which should be resolved by adding __aiter__ to StreamingHTTPResponse)

comment:12 by Carles Pina Estany, 2 years ago

Cc: Carles Pina Estany added
Note: See TracTickets for help on using tickets.
Back to Top