Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#34062 closed Bug (fixed)

object HttpResponseNotAllowed can't be used in 'await' expression

Reported by: Antoine Lorence Owned by: Antoine Lorence
Component: Generic views Version: 4.1
Severity: Release blocker Keywords: async
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When defining a simple View subclass with only an async "post" method, GET requests to this view cause the following exception:

[29/Sep/2022 07:50:48] "GET /demo HTTP/1.1" 500 81134
Method Not Allowed (GET): /demo
Internal Server Error: /demo
Traceback (most recent call last):
  File "/home/alorence/.cache/pypoetry/virtualenvs/dj-bug-demo-FlhD0jMY-py3.10/lib/python3.10/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/home/alorence/.cache/pypoetry/virtualenvs/dj-bug-demo-FlhD0jMY-py3.10/lib/python3.10/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/alorence/.cache/pypoetry/virtualenvs/dj-bug-demo-FlhD0jMY-py3.10/lib/python3.10/site-packages/asgiref/sync.py", line 218, in __call__
    return call_result.result()
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 451, in result
    return self.__get_result()
  File "/usr/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
  File "/home/alorence/.cache/pypoetry/virtualenvs/dj-bug-demo-FlhD0jMY-py3.10/lib/python3.10/site-packages/asgiref/sync.py", line 284, in main_wrap
    result = await self.awaitable(*args, **kwargs)
TypeError: object HttpResponseNotAllowed can't be used in 'await' expression

This can be easily reproduced with an empty project (no external dependencies) started with Django 4.1.1 and python 3.10.6.

Basic view to reproduce the bug:

from django.views import View
from django.http import HttpResponse

class Demo(View):
    """This basic view supports only POST requests"""
    async def post(self, request):
        return HttpResponse("ok")

URL pattern to access it:

from django.urls import path

from views import Demo

urlpatterns = [
    path("demo", Demo.as_view()),
]

Start the local dev server (manage.py runserver) and open http://127.0.0.1:8000/demo in the browser.
Server crash with 500 error with the given traceback.

Attachments (1)

Capture d’écran du 2022-09-29 09-54-36.png (162.8 KB ) - added by Antoine Lorence 20 months ago.

Download all attachments as: .zip

Change History (14)

by Antoine Lorence, 20 months ago

comment:1 by Carlton Gibson, 20 months ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Yes, looks right.

http_method_not_allowed() needs to be adjusted to handle both sync and async cases in the same way as options()

Do you have capacity to do a patch quickly? (Otherwise I'll take it on.)

Thanks for the report!

Regression in 9ffd4eae2ce7a7100c98f681e2b6ab818df384a4.

comment:2 by Antoine Lorence, 20 months ago

Thank you very much for your confirmation.

I've never contributed to Django codebase, but the fix for this issue seems obvious. I think this is a good occasion to contribute for the first tme.
I will follow contributions guide and try to provide a regression test and a patch for that issue.

comment:3 by Antoine Lorence, 20 months ago

Owner: changed from nobody to Antoine Lorence
Status: newassigned

comment:4 by Carlton Gibson, 20 months ago

Great, welcome aboard Antoine!

I pushed a draft here https://github.com/django/django/compare/main...carltongibson:django:4.1.2/ticket-34062 that you can use for inspiration.

If you open a PR on GitHub, I'm happy to advise.

Normally there's no rush here, but we have releases due for the beginning of next week, and as a regression this needs to go in. As such, if you hit any barriers please reach out so I can help out. Thanks 🏅

comment:5 by Antoine Lorence, 20 months ago

Wow, your draft is almost exactly what I already wrote in my local fork.

I ended with the exact same modification in View.http_method_not_allowed() (to the letter).
I also written the almost same test in "async/tests" (I didn't use RequestFactory, then your version is better).

I was currently looking into "asgi/tests" to check if I can add a full request-response lifecycle test in such case, but this appear to be more challenging. Do you think this is feasible / required ?

Last edited 20 months ago by Antoine Lorence (previous) (diff)

comment:6 by Carlton Gibson, 20 months ago

It's feasible, but I don't think it's required.

(We already test the full dispatch in many places elsewhere. What we're looking for here is the http_method_not_allowed() is correctly adapted when the view is async (i.e. has async handlers).

Make sense?

comment:7 by Antoine Lorence, 20 months ago

That totally makes sense. What is the next step now ?

Your draft is perfect, more complete than mine (I didn't write into release notes).
I think you can merge your branch django:4.1.2/ticket-34062.

comment:8 by Carlton Gibson, 20 months ago

I'd like to give you the chance to contribute if you'd like to do that![0]

You've made the report. You've investigated.
If you'd like to make the PR, please do!

You can pull any parts from my branch. If you'd like you can add Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es> at the bottom of the commit message (but that's not necessary 🙂)

[0]: This is entirely selfish on my part — once you've done 1 PR, you're ever so much more likely to do another 🙂

Version 1, edited 20 months ago by Carlton Gibson (previous) (next) (diff)

comment:9 by Antoine Lorence, 20 months ago

Thank for your support. The PR is now open: https://github.com/django/django/pull/16136

comment:10 by Antoine Lorence, 20 months ago

Has patch: set

comment:11 by Carlton Gibson, 20 months ago

Triage Stage: AcceptedReady for checkin

comment:12 by Carlton Gibson <carlton@…>, 20 months ago

Resolution: fixed
Status: assignedclosed

In 9b0c9821:

Fixed #34062 -- Updated View.http_method_not_allowed() to support async.

As with the options() methods, wrap the response in a coroutine if
the view is async.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

comment:13 by Carlton Gibson <carlton.gibson@…>, 20 months ago

In ecf6506f:

[4.1.x] Fixed #34062 -- Updated View.http_method_not_allowed() to support async.

As with the options() methods, wrap the response in a coroutine if
the view is async.

Co-authored-by: Carlton Gibson <carlton.gibson@…>

Backport of 9b0c9821ed4dd9920cc7c5e7b657720d91a89bdc from main

Note: See TracTickets for help on using tickets.
Back to Top