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.

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

cannot use sendfile() in a http handler: #6546

Open
terrywh opened this issue Aug 28, 2024 · 5 comments · May be fixed by #6565
Open

cannot use sendfile() in a http handler: #6546

terrywh opened this issue Aug 28, 2024 · 5 comments · May be fixed by #6565
Labels
bug 🐞 Something isn't working

Comments

@terrywh
Copy link

terrywh commented Aug 28, 2024 •
edited
Loading

cannot use sendfile() in a http middleware handler:

below code skipped some code about defining / registering modules

func (m *Middleware) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) (error) {
  file, _ := os.Open("file")
  w.(io.ReaderFrom).ReadFrom(file) 
  return nil
}

OR

func (m *Middleware) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) (error) {
  file, _ := os.Open("file")
  file.WriteTo(w)
  return nil
}

either way the sendfile / splice will not be called for the following reason:

  1. caddyhttp.ResponseRecorder is using io.Copy() (caddyhttp/responsewriter.go:47) instead of http.response.ReadFrom()

this is where the problem resides; io.Copy first use WriterTo, not ReadFrom

  1. *http.response does not implement syscall.Conn interface;
  2. file *os.Filegot wrapped in os.fileWithoutWriteTo (file.go:269)
  3. (*http.response) ReadFrom()/sendfile() try to use TCPConn.ReadFrom but src is not a *os.File (sendfile_linux_go:20)
  4. fallback to genericReadFrom() (tcpsock_posix.go:54)
    (caddy/2.8.4/go/1.22.5)

Originally posted by @terrywh in #4731 (comment)

Maybe change responsewriter.go:48 from io.Copy to directly call *http.response.ReadFrom ?
@mholt
Copy link
Member

mholt commented Sep 2, 2024

Interesting; wouldn't io.Copy call ReadFrom for us?

if dst implements ReaderFrom, the copy is implemented by calling dst.ReadFrom(src).

@mholt mholt added help wanted 🆘 Extra attention is needed bug 🐞 Something isn't working labels Sep 2, 2024
@terrywh
Copy link
Author

terrywh commented Sep 3, 2024 •
edited
Loading

Interesting; wouldn't io.Copy call ReadFrom for us?

if dst implements ReaderFrom, the copy is implemented by calling dst.ReadFrom(src).

it dit, but after *os.File had been wrapped in fileWithoutWriteTo ( in os.File.WriteTo )

io.Copy try to call WriteTo in dst followed by try to call ReadFrom in src;

@WeidiDeng
Copy link
Member

Actually stdlib file server uses io.CopyN instead of io.Copy, which uses io.LimitedReader under the hood.

http.response is for http1.1 only. We can't use that. Also, when ReadFrom, assuming the underlying connection is plain tcp or unix, it will handle the unwrapping of io.LimitedReader. For example, raw tcp conn tries splice first here.

I guess we can test if the ResponseWriter implements ReadFrom, which was actually the case when it was first introduced. It was simplified in cd486c2.

@WeidiDeng WeidiDeng removed the help wanted 🆘 Extra attention is needed label Sep 7, 2024
@WeidiDeng
Copy link
Member

I'll make the necessary change later. You can reimplement that part and create a pr if you feel like it.

@WeidiDeng WeidiDeng linked a pull request Sep 7, 2024 that will close this issue
Open
@WeidiDeng
Copy link
Member

I see you also created a issue at stdlib. For now I suggest you to use http.ServeContent to serve *os.File. Not only will it handle Range requests and If-Modified etc. According to this issue, it turns out io.LimitedReader will help the performance in this case. Also, you don't need to wait for that patch to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug 🐞 Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http: ReponseWriter prefer ReadFrom if available caddyserver/caddy
3 participants
@terrywh @mholt @WeidiDeng