django / asgiref

ASGI specification and utilities
https://asgi.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
1.49k stars 212 forks source link

Re-evaluate "http.response.zerocopysend" #423

Open Archmonger opened 1 year ago

Archmonger commented 1 year ago

Right now, none of the existing ASGI webservers support zerocopysend. This seems to be due to the additional code complexity needed to support this extension.

This lack of support has prevented me from adding zerocopysend to whitenoise, which would have been a big win for the Python web development community.

I'm opening this issue as a place to discuss what, if anything, can be done to encourage adoption for zerocopysend.

Here's a few questions to get this discussion started:

  1. Is a zerocopysend2 interface needed to encourage adoption?
  2. Does asgiref need some helper utilities to simplify adoption of the current extension?
  3. It seems that adding zerocopysend would require facing the buffer. Is it worthwhile (or feasible) to engineer a way of avoiding this?
Archmonger commented 1 year ago

cc: @pgjones @Kludex @tomchristie

Kludex commented 1 year ago

The ones who know more about it are @synodriver and @abersheeran.

abersheeran commented 1 year ago

The reason for facing buffers is because of the design of asyncio. It seems possible to solve this problem if we modify the zerocopysend standard so that it can only be sent at the end of the message.

Archmonger commented 11 months ago

@abersheeran

In terms of what this would like in practice, do you have a proposed interface?

Trying to work backwards and see how/if to push through this issue.

Lack of zero copy send is pretty painful for Whitenoise and ReactPy.

synodriver commented 11 months ago

I have a fork of hypercorn with this extension, but it will only work under http1.1, since h2 doesn't provide this feature. As for facing the buffer, I personally think there's no problem because mine has been running for a long time.

Archmonger commented 11 months ago

@synodriver

Would an interface change allow h2 support without waiting on library support?

synodriver commented 11 months ago

@synodriver

Would an interface change allow h2 support without waiting on library support?

I don't think it's possible. hypercorn relys on those sans-io projects to parse raw data. You can send data directly, but that will break h2 library's internal state machine.

Archmonger commented 3 months ago

cc: @synodriver @pgjones @gi0baro

As I have recently adopted the maintenance burden of WhiteNoise via my approved fork ServeStatic, file streaming via ASGI webservers is now something I need to focus on.

Before I start shooting PRs into the dark, I'd like the SW leads of the most popular webservers to help brainstorm on what the path forward is here.

After revisiting this issue, my first thought was to effectively clone the WSGI FileWrapper extension within Uvicorn/Hypercorn/Granian. For example:

async def application(scope, receive, send):
    FileWrapper = scope["extensions"]["file_wrapper"]

    # Returning a FileWrapper allows the webserver to handle the file with whatever it feels is the best solution.
    return FileWrapper(open("example.css", "rb"), BLOCK_SIZE)
  1. Does this FileWrapper interface make integration of this file handling solution any easier?
    • I'm assuming the answer is yes, since it could allow detection of file transmissions further down the stack & HTTP pipeline.
    • Perhaps I'm missing some knowledge here on why the zerocopysend interface was selected over this. This legacy interface seems to also allow for graceful fallbacks to pure-python file handling.
  2. Are there any other ideas on adding file handling to ASGI frameworks in a way that is both performant and isn't painful to maintain for webservers?
  3. Is it fair to assume the zerocopysend extension is dead to Uvicorn/Hypercorn/Granian due to ASGI webserver maintenance burden?
tomchristie commented 3 months ago

Is it fair to assume the zerocopysend extension is dead to Uvicorn/Hypercorn/Granian due to ASGI webserver maintenance burden?

Not necessarily, the previous PR on uvicorn wasn't going to be acceptable due to the behaviour changes it introduced. Probably just needs someone to step up to it again.

gi0baro commented 3 months ago
  1. Is it fair to assume the zerocopysend extension is dead to Uvicorn/Hypercorn/Granian due to ASGI webserver maintenance burden?

Not really, but I can only speak in regards of Granian here.

Also I'd keep Granian out of the topic here, as:

Given these points, and also the fact Granian is quite young compared to other servers, I don't think zerocopysend will be part of the development priorities in the near future (but I'm open to any PRs for this).

In terms of the extension itself: I don't have any particular objection in how it was designed; I'd probably would only change the fact zerocopysend and response.body messages can be mixed, as it is not super clear to me what's the intended use-case for that, and it will probably simplify the overall state flow in servers willing to support this.

tomchristie commented 2 months ago

I'd probably would only change the fact zerocopysend and response.body messages can be mixed, as it is not super clear to me what's the intended use-case for that, and it will probably simplify the overall state flow in servers willing to support this.

Second guessing, I'd assume that the intent there was for zero-copy-send to be usable with Range requests.

gi0baro commented 2 months ago

I'd probably would only change the fact zerocopysend and response.body messages can be mixed, as it is not super clear to me what's the intended use-case for that, and it will probably simplify the overall state flow in servers willing to support this.

Second guessing, I'd assume that the intent there was for zero-copy-send to be usable with Range requests.

Isn't that the purpose of offset and count items in http.response.zerocopysend message? Why would you need to mix bytes from FDs with anything else? 🤔

Archmonger commented 2 months ago

Yes the zerocopysend user API does work for range requests, but unfortunately that ends up being a moot point. The problem is that the spec for zerocopysend is too invasive to integrate with existing webservers, which has resulted in nearly all of them not supporting this extension.

Worst case, I'd be okay with each webserver creating webserver-specific extensions outside the ASGI spec while the community decides what the best path forward is.

tomchristie commented 2 months ago

The problem is that the spec for zerocopysend is too invasive to integrate with existing webservers, which has resulted in nearly all of them not supporting this extension.

Yes.

while the community decides what the best path forward is.

I can't speak for "here's how it should be supported", tho I would suggest dropping or otherwise sidelining the extension as currently designed.

Archmonger commented 2 months ago

I'm realizing it's likely more computationally efficient to continue down the route of Path Send.

I know the Path Send spec has already been formalized, but is it technologically feasible to add offset and count as an addendum to the spec?

andrewgodwin commented 2 months ago

If there's some way to add it backwards-compatibly, then we can do that, but I can't immediately think of a perfect way to.

Archmonger commented 2 months ago

If you're okay with cluttering the ASGI extension namespace, it could be implemented as a http.response.pathsend2 extension.

gi0baro commented 2 months ago

I'm realizing it's likely more computationally efficient to continue down the route of Path Send.

I know the Path Send spec has already been formalized, but is it technologically feasible to add offset and count as an addendum to the spec?

If you're okay with cluttering the ASGI extension namespace, it could be implemented as a http.response.pathsend2 extension.

@Archmonger maybe it's just me, but I still miss the point here.. If I got your need correctly, you'd like people using the project you maintain to take advantage of some offload strategy to send files in place of the standard Python buffering. Now, count and offset in zerocopysend are AFAIK designed to deal with range requests, which is something nice to have, but IMHO kinda goes beyond your point, unless you have some numbers stating the vast majority of your users do in fact range requests. So to me, before actually changing the spec, and having 0 servers supporting it, I'd rather invest my energies into making more servers adopting the existing pathsend spec. Then we can probably open a discussion about supporting ranges in pathsend or in a new extra spec designed for that.

septatrix commented 2 months ago

...unless you have some numbers stating the vast majority of your users do in fact range requests.

Pretty much all browsers use range requests when loading large media like audio and video. Safari/WebKit even goes further and fails to load videos if range requests are not supported.

gi0baro commented 2 months ago

Pretty much all browsers use range requests when loading large media like audio and video. Safari/WebKit even goes further and fails to load videos if range requests are not supported.

Those are not numbers from the project though. Browsers' behaviour doesn't really take a role in ASGI design. And again, I still think splitting the discussion in the two topics (zerocopysend/pathsend support, and range support in pathsend) would be benefical.

septatrix commented 2 months ago

Pretty much all browsers use range requests when loading large media like audio and video. Safari/WebKit even goes further and fails to load videos if range requests are not supported.

Those are not numbers from the project though. Browsers' behaviour doesn't really take a role in ASGI design.

You were asking for numbers how many users use range requests (unless I misunderstood your question). I just wanted to note that for some scenarios 100% of users issue range requests (and many would see errors if the server would not offer range requests).

And again, I still think splitting the discussion in the two topics (zerocopysend/pathsend support, and range support in pathsend) would be benefical.

That seems like a good idea :+1:

Archmonger commented 2 months ago

Now, count and offset in zerocopysend are AFAIK designed to deal with range requests, which is something nice to have, but IMHO kinda goes beyond your point, unless you have some numbers stating the vast majority of your users do in fact range requests.

@gi0baro The relevant repo(s) I'm currently maintaining are production-grade static file servers built in Python. Currently, WSGI file transfers can be handled via WSGI's FileWrapper in a performant manner. While ASGI currently requires buffering the file in chunks within Python. Ideally, I'd like all file requests (including range requests) to be handled by more performant options, if available.

So to me, before actually changing the spec, and having 0 servers supporting it, I'd rather invest my energies into making more servers adopting the existing pathsend spec.

Yup I agree that more ASGI webservers need to support the path send spec. To be fair, the spec is fairly new so it's not really surprising that all the webservers don't support it yet.


All further replies related to Path Send should be made in the new issue.

Archmonger commented 2 months ago

Now circling back around to zerocopysend, I think it's fair to say that there isn't a strong interest by the most popular ASGI webservers to add support for it.

My personal opinion is that the extension spec can/should be deprecated when an alternative exists that can be used for HTTP range requests, but of course the final call goes to the Django leads.

I will keep this issue open in case any of the Django leads want to give any thoughts/remarks.

gi0baro commented 2 months ago

My personal opinion is that the extension spec can/should be deprecated when an alternative exists that can be used for HTTP range requests, but of course the final call goes to the Django leads.

To be fair, pathsend – which I personally introduced into ASGI spec – was not intended to be a replacement for zerocopysend. Even if it might be used for the same purpose, it was designed explicitly as a port of a RSGI feature.

And this is also why it won't probably produce any performance gain in a pure Python ASGI server: when sending files back the main bottleneck is to read from the file descriptor, especially given there are no real unblocking I/O implementation out there except for uring, but it really depends on a specific Linux kernel version only. pathsend will be generally faster if you can avoid the GIL, but if your code is Python based, there's no way to avoid that. Which is also the reason – probably – this is not supported by many servers out there.

andrewgodwin commented 2 months ago

My approach to accepting extensions has been relatively open; I deliberately did not deconflict pathsend and zerocopysend as they do serve somewhat similar purposes,. zerocopysend is in theory easier to make perform well, given it can be offloaded to the OS, but it also relies on the ASGI server code being in the same process as the application.

I'd like to see us move more towards pathsend personally; file descriptors and the ability to use os.sendfile is unix-platform specific, and I think the large majority of "send a lot of data" cases are going to be files on disk, even if they're just buffered there.

Archmonger commented 2 months ago

zerocopysend is in theory easier to make perform well, given it can be offloaded to the OS.

Is it not possible for web servers to offload pathsend traffic to the OS?

I have been under the impression that the "backend design" of pathsend is open to interpretation, and using os.sendfile on Linux is likely what most web servers would do.

gi0baro commented 2 months ago

zerocopysend is in theory easier to make perform well, given it can be offloaded to the OS, but it also relies on the ASGI server code being in the same process as the application.

I have been under the impression that the "backend design" of pathsend is open to interpretation, and using os.sendfile on Linux is likely what most web servers would do.

To be clear: my point about performance was about the fact ASGI servers are async by nature, so even if you offload disk I/O to the OS, that call will be still blocking and thus potentially block the the event-loop (eg: Granian still reads from disk and writes to network in the user-space, it simply does that outside of the Python interpreter and thus the GIL). So regardless of the ASGI spec/extension involved, in order to truly "offload" disk I/O in Python async servers the only possible way would be to have a separated thread executor and be sure whatever primitive gets used it will release the GIL; otherwise the all thing will either block the loop or be 0-concurrent due to GIL locks.

So @Archmonger you should probably take into account that, independently of the specific extension, this will probably need efforts in handling some complexity on the servers maintainer's side to be implemented, given your true aim here is increase the general I/O performance on files.

Archmonger commented 2 months ago

Yep, on a related note - in my tests using a simple asyncio.to_thread wrapper around stdlib file IO calls, I saw performance improvements in the range of 40% in high concurrency workloads.

Ironically, this solution ended up being more performant than any of the async file IO libraries I found on PyPi so that solution is functionally indentical to what ServeStatic currently uses.

I would assume similar or greater performance improvements when using threading alongside os.sendfile.

There is a really nasty workaround to have an os.sendfile equivalent on Windows, but it's not the end of the world to simply wait for Python to add support for it.