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

Add ASGI adapter #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ASGI adapter #261

wants to merge 1 commit into from

Conversation

kmichel
Copy link

@kmichel kmichel commented Sep 14, 2020

Hi !

Following discussion in #251, here is an attempt at a native ASGI adapter for WhiteNoise,
without using the WsgiToAsgi adapter.

File reading is still done synchronously but I'm not certain it's a real problem (and I'm not
sure using a threaded async wrapper would really help). The reading is done in small blocks,
which should avoid long blocking times and fairly advance all requests simultaneously.

There are probably many things to improve, I wrote it without altering the existing code
but there might be a better approach.

The new code is tested but I haven't written documentation yet.

An interesting point is that it avoids an issue with Range requests I had with WsgiToAsgi.

@evansd
Copy link
Owner

evansd commented Sep 15, 2020

@kmichel Hey, thanks so much for this. It would be great to get native ASGI support in Whitenoise. Just to warn you that things are pretty busy at $DAYJOB right now so I might not get a chance to look at this straightaway, but I'll try to get to it as soon as I can. Thanks!

@troygrosfield
Copy link

@evansd your contributions to this project are certainly appreciated by all in the community! After reading through some of the issues, it's seems like you don't have as much time for dev work on this repo (I had the same problem with a few open source apps I released). What about moving this repo to JazzBand to make it community owned?

That way the community can get more involved in helping move features like this forward. Obviously, JazzBand would need to agree to take on the project.

Thoughts?

@evansd
Copy link
Owner

evansd commented Apr 22, 2021

@troygrosfield Hey, thanks; and you're absolutely right. I've been a feeling a vague but growing sense of open-source guilt about this project as realistically I just can't devote the time to it that it deserves. I've also not been doing as much web-facing work recently which means I haven't been using it myself as much as I used to.

Jazzband is exactly the right home for it and from a quick glance I think it meets the guidlines for inclusion. What are the next steps?

@troygrosfield
Copy link

troygrosfield commented Apr 22, 2021

@evansd oh trust me, I know the feeling. It's the reason Jazzband was created. Look at the first 2 steps in the FAQ:

In short:

  1. become a Jazzband member which gives you elevated permissions on all the projects in the Jazzband organization:
  2. transfer the project:

Thanks again for all your contributions and creating software that people truly find useful!

@Archmonger
Copy link

Archmonger commented Nov 20, 2021

@evansd Sending you a ping since it's around holiday season, so hopefully you have a couple hours of availability to consider a Jazzband transfer.

As a Jazzband member and someone who loves Whitenoise, I would definitely help out with maintenance/support/updates.

Copy link

@Archmonger Archmonger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job on the tests, seems fairly extensive.

def read_file(file_handle, content_length, block_size):
bytes_left = content_length
while bytes_left > 0:
data = file_handle.read(min(block_size, bytes_left))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use aiofiles instead of the stdlib files.

The current method is going to stall the ASGI the event queue for the duration of the file read, which will result in even worse performance than the WSGI version.

@@ -0,0 +1,74 @@
class AsgiWhiteNoise:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you mentioned in your original post, the docs still need an update to show end-users how to configure whitenoise with ASGI.

@tony
Copy link

tony commented Jan 3, 2022

@evansd @Archmonger

Any updates on moving over? Have you initiated a transfer to the @jazzband organization?

(This is a pretty prolific django package so having extra support behind it would be a big leap)

@Archmonger
Copy link

Archmonger commented Jan 3, 2022

I have not received any word from evansd. If he'd like I could go through the Jazzband transfer process for him, but I'd need his approval first. This repo already fits all criteria, so the transfer process is just a formality.

@tony
Copy link

tony commented Jan 4, 2022

@Archmonger

Also, @evansd can also do it himself just by moving the repo over to @jazzband via project's Settings page, also, right? He can easily do it himself

Either way, you helping him too is fine

@evansd Any availability to send over the repo to jazzband tonight? This week?

@adamchainz
Copy link
Collaborator

adamchainz commented Jan 31, 2022

Hi, I'm helping maintain Whitenoise right now. I'm preparing a release that supports the latest Pythons and Djangos. I'd love to merge this ASGI adapter too. It seems that @Archmonger 's comments have not been addressed, which would be key - docs are important, and use of aiofiles seems critical to performance.

@kmichel would you be able to pick this up? Or @Archmonger if you want to make a second PR that would be good too.

@Archmonger
Copy link

If @kmichel doesn't reply back today I'll open a new PR to address the issues. Both docs and aiofiles are fairly minimal changes.

@Archmonger
Copy link

Archmonger commented Feb 2, 2022

Seems like there's no response back from @kmichel .

I'll have time on Thursday or Friday to generate this PR.

Another thing I'm now noticing is this PR does not address Django middleware. My new PR will address this.

@adamchainz When I develop the Django middleware, how would you feel if I performed async updates to the existing middleware, rather than creating an new async middleware class? That would force the min Django version to be 3.1+, but require no effort from the end-user to obtain ASGI performance benefits.

Additionally, would be significantly less effort to maintain one middleware class over two.

EDIT: I believe it's possible to implement async support in a backwards-compatible way as long as I avoid using django.utils.decorators.sync_and_async_middleware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants