Skip to content

Support zerocopy send#1210

Closed
abersheeran wants to merge 28 commits into
Kludex:masterfrom
abersheeran:zerocopy-send
Closed

Support zerocopy send#1210
abersheeran wants to merge 28 commits into
Kludex:masterfrom
abersheeran:zerocopy-send

Conversation

@abersheeran

@abersheeran abersheeran commented Oct 5, 2021

Copy link
Copy Markdown
Contributor

Close #35

@abersheeran abersheeran marked this pull request as ready for review October 5, 2021 14:46
Comment thread uvicorn/protocols/http/httptools_impl.py Outdated
Comment thread requirements.txt Outdated
@lovelydinosaur

Copy link
Copy Markdown

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

@abersheeran

Copy link
Copy Markdown
Contributor Author

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

😀Yeah, I joined.

@Kludex

Kludex commented Oct 6, 2021

Copy link
Copy Markdown
Owner

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

I was about to ask that. 👍 Thanks!

@lovelydinosaur

Copy link
Copy Markdown

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

@Kludex

Kludex commented Oct 7, 2021

Copy link
Copy Markdown
Owner

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

@abersheeran

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

Comment thread uvicorn/protocols/http/httptools_impl.py Outdated
@Kludex

Kludex commented Oct 7, 2021

Copy link
Copy Markdown
Owner

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

synodriver commented Oct 8, 2021

Copy link
Copy Markdown

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

Comment thread docs/server-behavior.md Outdated
@Kludex Kludex added this to the Version 0.16.0 milestone Oct 9, 2021

@euri10 euri10 left a comment

Copy link
Copy Markdown
Contributor

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 ?

Comment thread docs/server-behavior.md Outdated
# 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
Copy Markdown

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

Comment thread uvicorn/protocols/http/h11_impl.py
@lovelydinosaur

Copy link
Copy Markdown

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

Kludex commented Dec 5, 2022

Copy link
Copy Markdown
Owner

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