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

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

Open
Archmonger opened this issue Nov 29, 2023 · 25 comments
Open

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

Archmonger opened this issue Nov 29, 2023 · 25 comments

Comments

@Archmonger
Copy link

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
Copy link
Author

cc: @pgjones @Kludex @tomchristie

@Kludex
Copy link
Contributor

Kludex commented Nov 29, 2023

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

@abersheeran
Copy link
Contributor

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
Copy link
Author

Archmonger commented Dec 10, 2023

@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
Copy link

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
Copy link
Author

@synodriver

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

@synodriver
Copy link

@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
Copy link
Author

Archmonger commented Aug 30, 2024

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
Copy link
Member

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
Copy link
Contributor

gi0baro commented Aug 30, 2024

  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:

  • due to the huge difference in architecture when compared to other servers, moving file descriptors between the app and the server would be quite hard, and it's unclear what's the performance gain (if any)
  • it already supports pathsend (introduced also because the inability to support zerocopysend), which even if less powerful in terms of features, produces a nice gain in performance (which it seems to be what you're looking for) that is probably higher of what you could get with Python anyway (and os.sendfile is only available on Unix).

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
Copy link
Member

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
Copy link
Contributor

gi0baro commented Sep 10, 2024

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@gi0baro
Copy link
Contributor

gi0baro commented Sep 23, 2024

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
Copy link

...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
Copy link
Contributor

gi0baro commented Sep 23, 2024

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
Copy link

septatrix commented Sep 23, 2024

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 👍

@Archmonger
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

gi0baro commented Sep 25, 2024

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
Copy link
Member

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.

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

No branches or pull requests

8 participants