Repository: caddyserver/caddy (language: go)

Issue:
Figure out why Caddy isn't using sendfile
Context:
- https://twitter.com/WBaryluk/status/1519019831651180544
- https://twitter.com/WBaryluk/status/1519019570140434432

Caddy should use Unix sendfile when there's no TLS and no compression. (And even for TLS, we should look into kTLS. But maybe unsafe, sigh.)


Discussion / hints:
Found this: https://www.reddit.com/r/golang/comments/b9ewko/comment/hphp294/?utm_source=reddit&utm_medium=web2x&context=3

(Sorry @mohammed90 😅) - We use `http.ServeContent` which calls `io.CopyN`, and that wraps the `src Reader` in a `io.LimitedReader` which _isn't_ an `*os.File`, so `sendfile` doesn't trigger 😢
There was an attempt to fix it here many years ago, went inactive: https://go-review.googlesource.com/c/go/+/7893/3
> so `sendfile` doesn't trigger

It should, ReadFrom will unconditionally* invoke `sendFile` which [unwraps](https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/net/sendfile_linux.go;l=25;bpv=0;bpt=0) the LimitReader.

*However this will only apply if the file is larger than 512 bytes as the first 512 byte chunk is [explicitly handled](https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/net/http/server.go;l=590;bpv=0;bpt=1) before triggering sendfile.

I can reproduce this behaviour with bare net/http so the problem is either in the size of the test files, or something else is missing from the equation.

Looking briefly at the file server I don't see any major red flags that would invalidate this, but it's late I'll have a proper look tomorrow night.
@flga Ohh, thank you for looking into this. This kernel stuff goes a little over my head "kerrantly" :upside_down_face:

I don't know how to debug this in Caddy; specifically, I've never used `strace` for anything more complicated than a hello-world-style C program, so maybe having confirmation that sendfile() _isn't_ being called would be a good start.

A test config might look like this:

```
:80

root ./sendfile_test
file_server
```

That's HTTP-only, and no `encode` directive means no compression, so I would expect sendfile to work here.

Or, without using a config file:

```
$ caddy file-server --root ./sendfile_test
```

I would also expect that to use sendfile when serving up files.

It would be absolutely awesome if you had a chance to dig into this and help us figure out how to get sendfile to be used (even on TLS*).

* We might be able to get sendfile working over TLS with a setsockopt() call from [what I've read](https://news.ycombinator.com/item?id=19071698).
Took a peek during lunch, sendfile is indeed not being called in caddy and the reason is that the response writer received in the file server is not a vanilla http responsewriter, it's an instance of our own ResponseRecorder. This type hides the ReadFrom method from io.Copy so the sendfile behaviour is never even considered. Doing a quick & dirty patch so that it exposes ReadFrom causes sendfile to be used again as expected.

ResponseWriterWrapper suffers from the same issue, it's effectively hiding some upgrade interfaces tho it's not clear to me how impactful that is or if it is even relevant given we support the major ones like Flusher.

Anyway, this was just a quick dirty gut check, I'll need to look at the code a little closer to see where best to spend our efforts.

re TLS: I'm not sure this is worth pursuing, it seems to me we would be fighting against net/http and crypto/tls the whole way so the gains would have to be huge in order to be worth it. A quick search for ktls in the go repo came up with this: https://github.com/golang/go/issues/44506 and a related post from Fillo https://words.filippo.io/playing-with-kernel-tls-in-linux-4-13-and-go/
That said it merits a deeper look, even if it does prove to not be worth it, so I'll try to fit it in soon.
D'oh, I totally forgot that we wrap all the ResponseWriters with our own recorder type so that we can provide logging:

https://github.com/caddyserver/caddy/blob/8cc8f9fddd2ea717633528322e1bef938bee2415/modules/caddyhttp/server.go#L221-L223

> Doing a quick & dirty patch so that it exposes ReadFrom causes sendfile to be used again as expected.

That's actually pretty reasonable, I would be happy to review that patch!

> ResponseWriterWrapper suffers from the same issue, it's effectively hiding some upgrade interfaces tho it's not clear to me how impactful that is or if it is even relevant given we support the major ones like Flusher.

I wish I knew a better way to wrap the response writer without having to implement all these interfaces, but clearly we are missing some. Again, I'd be happy to add it there, as the purpose of this wrapper type is to make it function like the underlying type.

Re: kTLS, I'm OK with it if we defer/shelve that for now, but if you feel like hacking on it just to see a PoC, I won't stop you! Depending on how aggregious it is (haha) we might not merge it, but if you want to have some fun then go ahead. If it turns out reasonable maybe we can even merge it.

Thanks for looking into things!
> I wish I knew a better way to wrap the response writer without having to implement all these interfaces, but clearly we are missing some. Again, I'd be happy to add it there, as the purpose of this wrapper type is to make it function like the underlying type.

You can read more about this problem in https://blog.merovius.de/posts/2017-07-30-the-trouble-with-optional-interfaces/

Fix patch:
diff --git a/modules/caddyhttp/responsewriter.go b/modules/caddyhttp/responsewriter.go
index 374bbfbac68..9820b41bc67 100644
--- a/modules/caddyhttp/responsewriter.go
+++ b/modules/caddyhttp/responsewriter.go
@@ -62,6 +62,16 @@ func (rww *ResponseWriterWrapper) Push(target string, opts *http.PushOptions) er
 	return ErrNotImplemented
 }
 
+// ReadFrom implements io.ReaderFrom. It simply calls the underlying
+// ResponseWriter's ReadFrom method if there is one, otherwise it defaults
+// to io.Copy.
+func (rww *ResponseWriterWrapper) ReadFrom(r io.Reader) (n int64, err error) {
+	if rf, ok := rww.ResponseWriter.(io.ReaderFrom); ok {
+		return rf.ReadFrom(r)
+	}
+	return io.Copy(rww.ResponseWriter, r)
+}
+
 // HTTPInterfaces mix all the interfaces that middleware ResponseWriters need to support.
 type HTTPInterfaces interface {
 	http.ResponseWriter
@@ -188,9 +198,26 @@ func (rr *responseRecorder) Write(data []byte) (int, error) {
 	} else {
 		n, err = rr.buf.Write(data)
 	}
-	if err == nil {
-		rr.size += n
+
+	rr.size += n
+	return n, err
+}
+
+func (rr *responseRecorder) ReadFrom(r io.Reader) (int64, error) {
+	rr.WriteHeader(http.StatusOK)
+	var n int64
+	var err error
+	if rr.stream {
+		if rf, ok := rr.ResponseWriter.(io.ReaderFrom); ok {
+			n, err = rf.ReadFrom(r)
+		} else {
+			n, err = io.Copy(rr.ResponseWriter, r)
+		}
+	} else {
+		n, err = rr.buf.ReadFrom(r)
 	}
+
+	rr.size += int(n)
 	return n, err
 }
 
@@ -251,4 +278,10 @@ type ShouldBufferFunc func(status int, header http.Header) bool
 var (
 	_ HTTPInterfaces   = (*ResponseWriterWrapper)(nil)
 	_ ResponseRecorder = (*responseRecorder)(nil)
+
+	// Implementing ReaderFrom can be such a significant
+	// optimization that it should probably be required!
+	// see PR #5022 (25%-50% speedup)
+	_ io.ReaderFrom = (*ResponseWriterWrapper)(nil)
+	_ io.ReaderFrom = (*responseRecorder)(nil)
 )


Test patch:
diff --git a/modules/caddyhttp/responsewriter_test.go b/modules/caddyhttp/responsewriter_test.go
new file mode 100644
index 00000000000..1913932003d
--- /dev/null
+++ b/modules/caddyhttp/responsewriter_test.go
@@ -0,0 +1,165 @@
+package caddyhttp
+
+import (
+	"bytes"
+	"fmt"
+	"io"
+	"net/http"
+	"strings"
+	"testing"
+)
+
+type responseWriterSpy interface {
+	http.ResponseWriter
+	Written() string
+	CalledReadFrom() bool
+}
+
+var (
+	_ responseWriterSpy = (*baseRespWriter)(nil)
+	_ responseWriterSpy = (*readFromRespWriter)(nil)
+)
+
+// a barebones http.ResponseWriter mock
+type baseRespWriter []byte
+
+func (brw *baseRespWriter) Write(d []byte) (int, error) {
+	*brw = append(*brw, d...)
+	return len(d), nil
+}
+func (brw *baseRespWriter) Header() http.Header        { return nil }
+func (brw *baseRespWriter) WriteHeader(statusCode int) {}
+func (brw *baseRespWriter) Written() string            { return string(*brw) }
+func (brw *baseRespWriter) CalledReadFrom() bool       { return false }
+
+// an http.ResponseWriter mock that supports ReadFrom
+type readFromRespWriter struct {
+	baseRespWriter
+	called bool
+}
+
+func (rf *readFromRespWriter) ReadFrom(r io.Reader) (int64, error) {
+	rf.called = true
+	return io.Copy(&rf.baseRespWriter, r)
+}
+
+func (rf *readFromRespWriter) CalledReadFrom() bool { return rf.called }
+
+func TestResponseWriterWrapperReadFrom(t *testing.T) {
+	tests := map[string]struct {
+		responseWriter responseWriterSpy
+		wantReadFrom   bool
+	}{
+		"no ReadFrom": {
+			responseWriter: &baseRespWriter{},
+			wantReadFrom:   false,
+		},
+		"has ReadFrom": {
+			responseWriter: &readFromRespWriter{},
+			wantReadFrom:   true,
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			// what we expect middlewares to do:
+			type myWrapper struct {
+				*ResponseWriterWrapper
+			}
+
+			wrapped := myWrapper{
+				ResponseWriterWrapper: &ResponseWriterWrapper{ResponseWriter: tt.responseWriter},
+			}
+
+			const srcData = "boo!"
+			// hides everything but Read, since strings.Reader implements WriteTo it would
+			// take precedence over our ReadFrom.
+			src := struct{ io.Reader }{strings.NewReader(srcData)}
+
+			fmt.Println(name)
+			if _, err := io.Copy(wrapped, src); err != nil {
+				t.Errorf("Copy() err = %v", err)
+			}
+
+			if got := tt.responseWriter.Written(); got != srcData {
+				t.Errorf("data = %q, want %q", got, srcData)
+			}
+
+			if tt.responseWriter.CalledReadFrom() != tt.wantReadFrom {
+				if tt.wantReadFrom {
+					t.Errorf("ReadFrom() should have been called")
+				} else {
+					t.Errorf("ReadFrom() should not have been called")
+				}
+			}
+		})
+	}
+}
+
+func TestResponseRecorderReadFrom(t *testing.T) {
+	tests := map[string]struct {
+		responseWriter responseWriterSpy
+		shouldBuffer   bool
+		wantReadFrom   bool
+	}{
+		"buffered plain": {
+			responseWriter: &baseRespWriter{},
+			shouldBuffer:   true,
+			wantReadFrom:   false,
+		},
+		"streamed plain": {
+			responseWriter: &baseRespWriter{},
+			shouldBuffer:   false,
+			wantReadFrom:   false,
+		},
+		"buffered ReadFrom": {
+			responseWriter: &readFromRespWriter{},
+			shouldBuffer:   true,
+			wantReadFrom:   false,
+		},
+		"streamed ReadFrom": {
+			responseWriter: &readFromRespWriter{},
+			shouldBuffer:   false,
+			wantReadFrom:   true,
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			var buf bytes.Buffer
+
+			rr := NewResponseRecorder(tt.responseWriter, &buf, func(status int, header http.Header) bool {
+				return tt.shouldBuffer
+			})
+
+			const srcData = "boo!"
+			// hides everything but Read, since strings.Reader implements WriteTo it would
+			// take precedence over our ReadFrom.
+			src := struct{ io.Reader }{strings.NewReader(srcData)}
+
+			if _, err := io.Copy(rr, src); err != nil {
+				t.Errorf("Copy() err = %v", err)
+			}
+
+			wantStreamed := srcData
+			wantBuffered := ""
+			if tt.shouldBuffer {
+				wantStreamed = ""
+				wantBuffered = srcData
+			}
+
+			if got := tt.responseWriter.Written(); got != wantStreamed {
+				t.Errorf("streamed data = %q, want %q", got, wantStreamed)
+			}
+			if got := buf.String(); got != wantBuffered {
+				t.Errorf("buffered data = %q, want %q", got, wantBuffered)
+			}
+
+			if tt.responseWriter.CalledReadFrom() != tt.wantReadFrom {
+				if tt.wantReadFrom {
+					t.Errorf("ReadFrom() should have been called")
+				} else {
+					t.Errorf("ReadFrom() should not have been called")
+				}
+			}
+		})
+	}
+}