Skip to content

net/http: short writes with FileServer on macos [1.23 backport] #70020

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.

Sign up for GitHub

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

Jump to bottom
Closed
gopherbot opened this issue Oct 24, 2024 · 4 comments
Closed

net/http: short writes with FileServer on macos [1.23 backport] #70020

gopherbot opened this issue Oct 24, 2024 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases
Milestone
Go1.23.3

Comments

@gopherbot
Copy link
Contributor

@ianlancetaylor requested issue #70000 to be considered for backport to the next 1.23 minor release.

@gopherbot Please open backport issues.

This causes inexplicable and difficult to avoid failures on BSD systems. We should backport the fix.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Oct 24, 2024
@gopherbot gopherbot mentioned this issue Oct 24, 2024
Closed
@gopherbot gopherbot added this to the Go1.23.3 milestone Oct 24, 2024
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/622696 mentions this issue: [release-branch.go1.23] internal/poll: keep copying after successful Sendfile return on BSD

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/622697 mentions this issue: [release-branch.go1.23] internal/poll: handle the special case of sendfile(2) sending the full chunk

@neild
Copy link
Contributor

neild commented Oct 25, 2024

Cherrypick should include both https://go.dev/cl/622696 (fix) and https://go.dev/cl/622697 (followup tweak to the fix).

@prattmic prattmic added the CherryPickApproved Used during the release process for point releases label Oct 30, 2024
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Oct 30, 2024
gopherbot pushed a commit that referenced this issue Oct 30, 2024
@neild @prattmic
[release-branch.go1.23] internal/poll: keep copying after successful …
6ba3a8a 
…Sendfile return on BSD

The BSD implementation of poll.SendFile incorrectly halted
copying after succesfully writing one full chunk of data.
Adjust the copy loop to match the Linux and Solaris
implementations.

In testing, empirically macOS appears to sometimes return
EAGAIN from sendfile after successfully copying a full
chunk. Add a check to all implementations to return nil
after successfully copying all data if the last sendfile
call returns EAGAIN.

For #70000
For #70020

Change-Id: I57ba649491fc078c7330310b23e1cfd85135c8ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/622235
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
(cherry picked from commit bd388c0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/622696
gopherbot pushed a commit that referenced this issue Oct 30, 2024
@panjf2000 @prattmic
[release-branch.go1.23] internal/poll: handle the special case of sen…
958f3a0 
…dfile(2) sending the full chunk

CL 622235 would fix #70000 while resulting in one extra sendfile(2) system
call when sendfile(2) returns (>0, EAGAIN).
That's also why I left sendfile_bsd.go behind, and didn't make it line up
with other two implementations: sendfile_linux.go and sendfile_solaris.go.

Unlike sendfile(2)'s on Linux and Solaris that always return (0, EAGAIN),
sendfile(2)'s on *BSD and macOS may return (>0, EAGAIN) when using a socket
marked for non-blocking I/O. In that case, the current code will try to re-call
sendfile(2) immediately, which will most likely get us a (0, EAGAIN).
After that, it goes to `dstFD.pd.waitWrite(dstFD.isFile)` below,
which should have been done in the first place.

Thus, the real problem that leads to #70000 is that the old code doesn't handle
the special case of sendfile(2) sending the exact number of bytes the caller requested.

Fixes #70000
Fixes #70020

Change-Id: I6073d6b9feb58b3d7e114ec21e4e80d9727bca66
Reviewed-on: https://go-review.googlesource.com/c/go/+/622255
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Run-TryBot: Andy Pan <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/622697
@gopherbot
Copy link
Contributor Author

Closed by merging CL 622696 (commit 6ba3a8a) to release-branch.go1.23.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases
Projects
None yet
Milestone
Go1.23.3
Development

No branches or pull requests
3 participants
@neild @prattmic @gopherbot