Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #30451 -- Added ASGI handler and coroutine-safety. #11209

Merged
merged 2 commits into from Jun 20, 2019

Conversation

@andrewgodwin
Copy link
Member

commented Apr 13, 2019

This adds in an AsgiHandler handler class, get_asgi_application, and an asgi.py file into the default project template. It serves Django over an ASGI interface, but does not yet allow asynchronous views; get_response is where the context changes from asynchronous to synchronous.

It also swaps out Django's threading-based safety for asgiref.local, which provides hybrid thread- and async-task-based safety.

The goal would be to land this in master, and then start work on getting async through the middleware layers and down to views if they are declared as async def. That way we can stabilise this, make sure it's OK, and hopefully ship it in 3.0, and then see how far we can get async view handling as another PR so it's not all tied up in one big patch.

This will be squashed down to a single commit for merge.

Show resolved Hide resolved docs/spelling_wordlist Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
@tomdyson
Copy link
Contributor

left a comment

Tiny docstring and documentation fixes

Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved docs/howto/deployment/asgi/daphne.txt Outdated
Show resolved Hide resolved docs/howto/deployment/asgi/index.txt Outdated
Show resolved Hide resolved docs/howto/deployment/asgi/uvicorn.txt Outdated

@andrewgodwin andrewgodwin force-pushed the andrewgodwin:asgi branch from 92912b6 to 4967156 Apr 24, 2019

@andrewgodwin andrewgodwin marked this pull request as ready for review Apr 24, 2019

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

The MariaDB test appears to be consistently failing for all PRs today, so I am going to say this is otherwise ready for review.

Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved docs/howto/deployment/asgi/daphne.txt Outdated
@pope1ni

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

So I'm fairly ignorant of async stuff at moment, but I see two things crop up a lot here:

  • django.utils.asyncio.async_unsafe()
  • asgiref.local.Local() in place of threading.local()

I presume that third-party apps I use may need to make use of these to support running under ASGI?

In the case of the latter, is asgiref.local.Local() expected to replace all uses of threading.local()?

Given these questions, will there be some documentation on implementing minimal support for ASGI for apps?

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2019

@pope1ni If you want to be fully async-safe, then yes, it's required that you account for coroutines in a third-party app. async_unsafe is a general protection against this occurring if you have code you know is unsafe for sure, while Local is a helpful implementation if you want threadlocal-like behaviour with async coroutines.

Neither is required for writing ASGI apps at all, but we will need a guide that is "adapting Django apps to work under async" which mentions both of these, as well as a slew of other factors like "don't use normal, sync requests in async code" and "don't use normal sockets". I did not intend to include that in this patch, since it's far more extensive than this basic safety. I'll add a warning to the ASGI deployment docs that are in here, though.

(Moreover, in ASGI mode Django still runs all client code as sync, so this would only be a problem after this patch if a user was overriding the AsgiHandler)

@pope1ni

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

@andrewgodwin Thanks for the explanation. Looking forward to seeing how this develops.

@andrewgodwin andrewgodwin force-pushed the andrewgodwin:asgi branch from fa20913 to 58a2dba May 1, 2019

Show resolved Hide resolved django/core/handlers/asgi.py Outdated
body += message["body"]
# Limit the maximum request data size that will be handled in-memory.
# TODO: Stream the body to temp disk instead?
# (we can't provide a file-like with direct reading as we would not be async)

This comment has been minimized.

Copy link
@apollo13

apollo13 May 1, 2019

Contributor

This probably cannot use the upload handlers for similar reasons?

This comment has been minimized.

Copy link
@andrewgodwin

andrewgodwin May 2, 2019

Author Member

I haven't looked into that yet, but probably not. File input in general has been a pain in Channels and I expect the same here.

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

I have added a Trac ticket to match this here: https://code.djangoproject.com/ticket/30451

@felixxm felixxm changed the title Implement ASGI handler and coroutine-safety Fixed #30451 -- Implemented ASGI handler and coroutine-safety. May 6, 2019

@carltongibson carltongibson force-pushed the andrewgodwin:asgi branch from 1b66029 to 0054a59 May 7, 2019

@carltongibson carltongibson force-pushed the andrewgodwin:asgi branch from 0054a59 to 490a99f May 8, 2019

@carltongibson
Copy link
Member

left a comment

Hey @andrewgodwin. This looks like a good starting point. I agree we should merge in and allow pushing forwards.

I made a small adjustment pulling up get_response(). Please review and squash in if you're happy.

I have a curious test collection issue on macOS, which isn't occurring elsewhere, with the async_to_sync wrapper on the test cases...

  File "/Users/carlton/.pyenv/versions/3.7.2/lib/python3.7/unittest/loader.py", line 235, in getTestCaseNames
    testFnNames = list(filter(shouldIncludeMethod, dir(testCaseClass)))
  File "/Users/carlton/.pyenv/versions/3.7.2/lib/python3.7/unittest/loader.py", line 232, in shouldIncludeMethod
    fullName = '%s.%s' % (testCaseClass.__module__, testFunc.__qualname__)
AttributeError: 'functools.partial' object has no attribute '__qualname__'

Where:

ipdb> testCaseClass                                                                                                            
<class 'asgi.tests.ASGITest'>
ipdb> testFunc                                                                                                                 
functools.partial(<bound method AsyncToSync.__call__ of <asgiref.sync.AsyncToSync object at 0x10b8ed4e0>>, None)

I'm guessing some use of wraps somewhere but I haven't dug fully yet.

@felixxm
Copy link
Member

left a comment

@andrewgodwin Thanks for this patch 🚀 I left some comments and questions. Sorry for some of them but I'm not an async expert.

Can you generally use single quotes and wrap comments at 79 chars? 🙏 Thanks.

Show resolved Hide resolved django/core/exceptions.py
Show resolved Hide resolved django/core/exceptions.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/utils/asyncio.py Outdated
Show resolved Hide resolved docs/howto/deployment/asgi/daphne.txt Outdated
Show resolved Hide resolved docs/howto/deployment/asgi/daphne.txt
Show resolved Hide resolved docs/howto/deployment/asgi/uvicorn.txt Outdated
Show resolved Hide resolved docs/releases/3.0.txt Outdated
@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@carltongibson The get_response thing looks fine, happily pulled in. I'll re-squash this all down once the review changes are done.

The qualname thing is strange, I've not seen that here or in the Jenkins tests. I'll go looking for what it is, but without a Mac to replicate on, I may be a bit stuck.

@felixxm I have changed the quotes to single-quotes where it matches (e.g. the signals file was double-quotes already). Also reasonably sure I got all the comments under 79 but let me know if you spot any more.

@andrewgodwin andrewgodwin force-pushed the andrewgodwin:asgi branch from db200aa to e5470c9 May 22, 2019

@felixxm felixxm force-pushed the andrewgodwin:asgi branch from e5470c9 to b08eb5d May 24, 2019

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented May 26, 2019

There, I've updated it so it'll correctly handle large POST bodies and buffer them to disk once they go over FILE_UPLOAD_MAX_MEMORY_SIZE, which is the best we can do until maybe we prototype async-ingestion-hidden-behind-a-threaded-file-object over in Channels and see how stable it is in everyday usage. In the meantime, this pattern is well-known and what Django does currently anyway.

I did notice we have literally no tests for pushing up large uploaded files, mostly as we have very few tests that go through the actual Handlers rather than through the test Client (and its fake handler). I ran a full set of manual tests on this locally; I'm trying to figure out if there's a way to sensibly test this in the test suite without actually spinning up a server but it seems hard for both the WSGI and the ASGI case.

@felixxm
Copy link
Member

left a comment

@andrewgodwin Thanks again for your hard work 🚀 I left few more small comments, sorry for partial reviews, but I'm not really comfortable in async-area.

Show resolved Hide resolved django/core/handlers/asgi.py
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py
Show resolved Hide resolved docs/releases/3.0.txt Outdated

@carltongibson carltongibson self-requested a review Jun 7, 2019

@andrewgodwin andrewgodwin force-pushed the andrewgodwin:asgi branch from 3b7a4d4 to 0e89338 Jun 7, 2019

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

I have re-squashed after the latest round of tweaks requested by @felixxm - I feel like we're getting ready to land, but I'm going to wait until I get an explicit "yes" from both of the Fellows :)

@carltongibson
Copy link
Member

left a comment

Hey @andrewgodwin.

I have a couple of comments on asgi.py.

(The tempfile.SpooledTemporaryFile approach to the body handling is super. 🙂)

Show resolved Hide resolved django/core/handlers/asgi.py
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
Show resolved Hide resolved django/core/handlers/asgi.py Outdated
@carltongibson

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

OK, this looks good to me. It's the start of a long road. 🙂

I kind of think that maybe we can tweak doc-here or doc-there but that it's pretty clear for the stage we're at. I think we should merge as-is and come back as the rest falls into place, and people start using it, particularly towards the end of the year.

Outstanding question: do we want a merge #11471, and abstract that bit. I don't really mind either way. DRY, yes, but sometimes these things are just better inlined. @felixxm you can decide. 🙂 And I assume you'll want to give it one last look over for remaining cosmetic changes?

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2019

I merged #11471 as @felixmm and I both agreed it should be spun out like that (and it's not like Request doesn't do that a lot already). Will rework this so it matches.

@andrewgodwin andrewgodwin force-pushed the andrewgodwin:asgi branch from 91b2fcc to 3e573db Jun 15, 2019

@felixxm

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@andrewgodwin I pushed some minor edits to comments and tests, added asgiref to the list of required packages for running all tests, and added two extra tests.

I'm going to check docs tomorrow and maybe add few more tests (if you prefer we can push them in a separate commit).

I have also one question about ASGIStaticFilesHandler, because currently it is unused. Can you give me some advice about it? How can I use it? 🤔

@felixxm

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

@andrewgodwin Can you check asgi.tests.ASGITest.test_file_response failure on Windows?

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

@felixxm Do we need to add asgiref to the guide for running tests since the patch also introduces it as a Django dependency in setup.py?

Also, I'm looking into the test failure on Windows, it's an apparent lack of a system magic-numbers database. Will fix so it accepts both as valid mimetypes.

@andrewgodwin

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

Also, ASGIStaticFilesHandler would in theory be used either in an asgi.py that was used to serve it in production, or an ASGI-enabled runserver (where the current static files handler is used).

@carltongibson

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

...ASGIStaticFilesHandler...

Good yes. 🙂

So you'd do something like:

application = ASGIStaticFilesHandler(get_asgi_application())
@felixxm

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

Do we need to add asgiref to the guide for running tests since the patch also introduces it as a Django dependency in setup.py?

Yes, we have all required packages there, also packages included in the setup.py like sqlparse or pytz.

@felixxm felixxm changed the title Fixed #30451 -- Implemented ASGI handler and coroutine-safety. Fixed #30451 -- Added ASGI handler and coroutine-safety. Jun 18, 2019

@felixxm felixxm force-pushed the andrewgodwin:asgi branch from c792649 to 7e94c57 Jun 18, 2019

@felixxm

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I squashed commits, pushed minor edits to docs and added more tests.

@felixxm felixxm force-pushed the andrewgodwin:asgi branch from 7e94c57 to cc16386 Jun 20, 2019

andrewgodwin and others added some commits Apr 12, 2019

Fixed #30451 -- Added ASGI handler and coroutine-safety.
This adds an ASGI handler, asgi.py file for the default project layout,
a few async utilities and adds async-safety to many parts of Django.

@felixxm felixxm force-pushed the andrewgodwin:asgi branch from cc16386 to 7f19e37 Jun 20, 2019

@felixxm felixxm merged commit 7f19e37 into django:master Jun 20, 2019

19 checks passed

docs Build #515 ended
Details
flake8 Build #515 ended
Details
isort Build #520 succeeded in 27 sec
Details
pr-mariadb/database=mysql,label=mariadb,python=python3.7 Build #516 ended
Details
pr-mariadb/database=mysql_gis,label=mariadb,python=python3.7 Build #516 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.6 Build #513 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.7 Build #513 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.6 Build #513 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.7 Build #513 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.6 Build #513 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.7 Build #513 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.6 Build #513 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.7 Build #513 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.6 Build #513 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.7 Build #513 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.6 Build #513 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.7 Build #513 ended
Details
pull-requests-javascript Build #517 ended
Details
pull-requests-windows/database=sqlite3,label=windows,python=Python37 Build #515 ended
Details
@felixxm

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Many thanks Andrew 🚀

@PetterS

This comment has been minimized.

Copy link

commented Jun 20, 2019

Whohoo! Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.