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
AsgiWhiteNoise
and async WhiteNoiseMiddleware
#359
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
…into asgi-compat
whitenoise/asgi.py
Outdated
content_length = int(dict(response.headers)["Content-Length"]) | ||
for block in read_file(response.file, content_length, block_size): | ||
await send( | ||
{"type": "http.response.body", "body": block, "more_body": True} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So ASGI has a "zero copy send" extension: https://asgi.readthedocs.io/en/latest/extensions.html#zero-copy-send
This is much more efficient than chunking and recombining the file in Python
That said, it looks like support isn't there yet:
- daphne - none
- hypercorn - none
- uvicorn - pending Support zerocopy send encode/uvicorn#1210
So it might be worth creating a follow-up issue for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's also a PR for hypercorn, jfyk: https://gitlab.com/pgjones/hypercorn/-/merge_requests/62
in any case, none of the ASGI servers supports it yet 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has anything changed in the "zero copy send" landscape?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has anything changed in the "zero copy send" landscape?
I've created a fork of hypercorn to add all extension support here, you can have a try, currently it's under development and should work with all asgi extension listed in asgi spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even when zero copy send gets merged into most ASGI webservers, there's still the question of Windows compatibility.
Ref: https://discuss.python.org/t/support-for-os-sendfile-for-windows/25020
This reverts commit 8590e82.
for more information, see https://pre-commit.ci
This reverts commit f4931e3.
This reverts commit 8a0930e.
Set up pre-commit locally - install as per https://pre-commit.com/ , then |
Co-authored-by: Adam Johnson <me@adamj.eu>
Hi, sorry this has gone unreviewed for so long. Just snowed under with other things at the moment. I am going to try to take a look at this as soon as I can. I think it's an important feature for Whitenoise. But it's a big change and I don't want to just merge it without proper consideration. |
Since I don't want to get into a cat-and-mouse game of constantly re-merging with main, hopefully this PR can get reviewed soon. |
@evansd could you please review this? |
Could @adamchainz approve this pull request if @evansd is too busy? |
@Archmonger, I'm running your branch in production in a Django app. I'll let you know if I have any issues, but so far so good. |
Can we get this merged in for the 6.7 release @evansd ? |
@Archmonger, when Whitenoise returns a 304 response for a file that is already cached on the client, it gives the following warning:
Is there something I should be doing to avoid this? |
What Django version are you running? |
@Archmonger, I'm running Django 5.0b1. I just tried it with 4.2.7 with the same result. I though it might be |
Can you send me your middleware and installed apps? Also let's pull this discussion to the side so that we don't spam everyone with this conversation. Discord: totoboto |
Hey @evansd could you please review and merge this in? This branch has been working just fine for me for months! |
Bumping this since it's been a couple more months. Can we get this merged and released? I'd love to get rid of the errors and sync penalty in my code. |
Hey @evansd any update on you looking at this? |
Would like to see this! |
Can this PR be merged? |
Any update on this PR? |
@jordanorc This PR is complete, but has been stuck in an "awaiting review" stage due to the WhiteNoise maintainers largely not having time. In order to avoid continuously re-merging with main, I've been waiting for a maintainer to review and/or approve. I have offered to help as a maintainer but haven't heard anything back yet. |
Also would like to see this merged ASAP. |
Co-authored-by: James Ostrander <11338926+jlost@users.noreply.github.com>
Co-authored-by: James Ostrander <11338926+jlost@users.noreply.github.com>
Co-authored-by: James Ostrander <11338926+jlost@users.noreply.github.com>
@Archmonger @adamchainz How can we get this one across the finish line? If it’s just resolving conflicts, would it be merged? |
I totally understand that maintaining open source software is hard work and I don't want to criticize anyone. WhiteNoise is great software and I have been using it happily for years in many projects, so thanks for that. That being said, I really wanted to move ahead and therefore have thrown together an ASGI middleware for static file serving here https://github.com/matthiask/blacknoise . It doesn't integrate with Django (middleware) making the implementation much smaller and simpler. It is running in production for a few weeks now and seems to work well. It uses the proven Starlette under the hood, uses anyio for reading files and supports compression using gzip and brotli. Full API compatibility with WhiteNoise isn't the goal, but PRs are welcome. I'm sorry for plugging my own project here, but maybe it helps someone else. If you're interested let's discuss it elsewhere, not here in this PR. Thanks. |
On a related note... Given that there's been no major I will wait a few days in case Dave Evans responds back, but otherwise I'll close this PR and fork the repository. |
Hi @Archmonger, Sorry to have been unresponsive. I'm not running ASGI anywhere in my day job at the moment, and so I've been reluctant to take on responsibility for a significant quantity of code which I don't use and therefore don't fully understand. I've also been reluctant to hand out maintainer status to people I don't know personally. And I feel like the whole So I actually think the best option at this point probably is to fork; not in an acrimonious spirit, but just as an acknowledgement that different people have different needs and priorities when it comes to this library. You're welcome to call it It's certainly possible that I will end up with a personal need to support ASGI in the future, in which case maybe I'll end up using your fork or we can look at re-merging the projects or something. Again, sorry for not making a call on this sooner: I've been prevaricating and somehow hoping that more spare time would drop out of the sky at some point – open-source life, eh? |
Tasks
aiofiles
for asynchronous file handlingSummary
I've created async variants for any methods/functions/classes that can benefit from
aiofiles
. For IO operations that currently do not have an async equivalent (such as the OS calls withinWhiteNoise.find_file(...)
), I've converted to async viaasyncio.to_thread(...)
(if available).Django middleware is now sync and async compatible. Thus, the middleware will provide Django an async
FileResponse
when possible.WhiteNoise
whitenoise.base.BaseWhiteNoise
WhiteNoise
class, but__call__
is now a stub.whitenoise.wsgi.WhiteNoise
WhiteNoise
class. UsesBaseWhiteNoise
as a subclass to obtain most methods.whitenoise.asgi.AsgiWhiteNoise
WhiteNoise
, but using the ASGI interface (async/threading)Responders
whitenoise.responders.StaticFile
aget_response
(async variant).aget_range_response
(async variant).aget_range_not_satisfiable_response
(async variant).whitenoise.responders.AsyncSlicedFile
SlicedFile
, but using theaiofiles
context manager interface.whitenoise.responders.SlicedFile
AsyncSlicedFile
.whitenoise.responders.Redirect
aget_response
(async variant).Middleware
whitenoise.middleware.WhiteNoiseMiddleware
whitenoise.middleware.AsyncWhiteNoiseFileResponse
whitenoise.middleware.AsyncFileIterator
__aiter__
helper that allow Django file responses to useaiofiles
.whitenoise.middleware.AsyncToSyncIterator
__aiter__
to__iter__
converter, since Django cannot use__aiter__
within WSGI.Design Considerations
Code Architecture
WhiteNoise
, rather than creating a whole separateAsgiWhiteNoise
class. However, this would either rely on heuristics or an initialization arg. This seemed more error-prone than an explicit class.BaseWhiteNoise
to async, since event loops perform best with as much yielding viaawait
as possible. However, that's a really high maintenance burden that we shouldn't commit to in this PR.Django Integration
aiofiles
is now a mandatory requirement due to Django middleware being async compatible. The alternative would have been a completely separate middleware class for ASGI, but that would negate a lot of the benefits of Django's middleware's context-aware sync/async auto-selection. Additionally, would have been a significant amount of extra code that would not have been worth the maintenance burden.8192
byte chunks of file data (same aswsgiref.FileWrapper
).Feature Compatibility
asgiref.compatibility.guarantee_single_callable
to retain compatibility for both ASGI v2 and ASGI v3.Test Environment:
AsgiWhiteNoise
pip install git+https://github.com/Archmonger/whitenoise@asgi-compat
# bash uvicorn example:app
Test Environment:
WhiteNoiseMiddleware
pip install git+https://github.com/Archmonger/whitenoise@asgi-compat
The existing middleware has been replaced with an async compatible variant. Therefore, follow the normal documentation for installing
WhiteNoiseMiddleware
.Issues
fix #251
fix #179
close #261
related #181