uvicorn icon indicating copy to clipboard operation
uvicorn copied to clipboard

Support zerocopy send

Open abersheeran opened this issue 3 years ago • 11 comments

  • [x] Implement with httptools.
  • [x] Implement with h11: https://h11.readthedocs.io/en/latest/api.html#sendfile

Close #35

abersheeran avatar Oct 05 '21 14:10 abersheeran

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

tomchristie avatar Oct 06 '21 14:10 tomchristie

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

😀Yeah, I joined.

abersheeran avatar Oct 06 '21 14:10 abersheeran

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

I was about to ask that. :+1: Thanks!

Kludex avatar Oct 06 '21 14:10 Kludex

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

tomchristie avatar Oct 07 '21 08:10 tomchristie

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

Kludex avatar Oct 07 '21 08:10 Kludex

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?

abersheeran avatar Oct 07 '21 12:10 abersheeran

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: https://github.com/encode/uvicorn/pull/1159 - More context on the mentioned PRs there.

Kludex avatar Oct 07 '21 14:10 Kludex

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!

abersheeran avatar Oct 07 '21 16:10 abersheeran

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

synodriver avatar Oct 08 '21 05:10 synodriver

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.

tomchristie avatar Dec 06 '21 10:12 tomchristie

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.

abersheeran avatar Dec 06 '21 11:12 abersheeran

Closing this as stale.

Kludex avatar Dec 05 '22 06:12 Kludex