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

Support zerocopy send #1210

Closed
wants to merge 28 commits into from
Closed

Conversation

abersheeran
Copy link
Member

@abersheeran abersheeran commented Oct 5, 2021

Close #35

requirements.txt Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

Hi @abersheeran I've invited you as a member of the Encode org. 👍

@abersheeran
Copy link
Member Author

Hi @abersheeran I've invited you as a member of the Encode org. 👍

😀Yeah, I joined.

@Kludex
Copy link
Member

Kludex commented Oct 6, 2021

Hi @abersheeran I've invited you as a member of the Encode org. +1

I was about to ask that. 👍 Thanks!

@tomchristie
Copy link
Member

Do we know what the deal is with the hanging CI tests?

@Kludex
Copy link
Member

Kludex commented Oct 7, 2021

It should be related with this branch. The CI is fine on uvicorn now.

@abersheeran
Copy link
Member Author

The sendfile cannot be implemented on Windows, so the test coverage cannot reach 95%. I am not familiar with whether it can be selectively skipped in this case?

@Kludex
Copy link
Member

Kludex commented Oct 7, 2021

The sendfile cannot be implemented on Windows, so the test coverage cannot reach 95%. I am not familiar with whether it can be selectively skipped in this case?

Yeah, we were adding pragma: no cover on non Python 3.8, as discussed here and the comment below.
But I wish we could do this: #1159 - More context on the mentioned PRs there.

@abersheeran
Copy link
Member Author

The sendfile cannot be implemented on Windows, so the test coverage cannot reach 95%. I am not familiar with whether it can be selectively skipped in this case?

Yeah, we were adding pragma: no cover on non Python 3.8, as discussed here and the comment below. But I wish we could do this: #1159 - More context on the mentioned PRs there.

Thanks for your reply, it is very helpful!

@synodriver
Copy link

synodriver commented Oct 8, 2021

Thank you for your pr! Now hypercorn supports zerocopysend too.

docs/server-behavior.md Outdated Show resolved Hide resolved
@Kludex Kludex added this to the Version 0.16.0 milestone Oct 9, 2021
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

alos I see a lot of # pragma: no cover is there a reason ?

docs/server-behavior.md Outdated Show resolved Hide resolved
# Sendfile
if self.allow_sendfile: # pragma: no cover
# Set the buffer to 0 to avoid the problem of sending file before headers.
transport.set_write_buffer_limits(0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really comfortable with the fact that this'll change the behaviour throughout, regardless of if http.response.zerocopysend is actually being used or not for any given request/response.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this option is not configured, the headers may be sent after the body is sent.

@tomchristie
Copy link
Member

Okay, so this is clearly a neatly done little bit of work, nonetheless I feel like I ought to be the voice of pushback here.

Personally I think that in the trade-off between functionality vs. increased complexity, increased ongoing maintenance cost, scope for bug introduction, this is a possibly poor trade-off for us.

I'd perhaps be more comfortable with this if it was obvious that it didn't affect any other behaviour other that just for that one message case, but currently the write buffer change makes me uncomfortable, as does the largish footprint of the change within the # Sending response body block.

@abersheeran
Copy link
Member Author

I'd perhaps be more comfortable with this if it was obvious that it didn't affect any other behaviour other that just for that one message case, but currently the write buffer change makes me uncomfortable, as does the largish footprint of the change within the # Sending response body block.

Sendfile has to face the buffer, there seems to be no good way to avoid interference with other types of responses-we have no means to predict this behavior before zerocopysend is sent.

@Kludex Kludex removed this from the Version 0.16.0 milestone Aug 15, 2022
@Kludex
Copy link
Member

Kludex commented Dec 5, 2022

Closing this as stale.

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.

Support for sendfile
5 participants