net/http: permit splice from socket to ResponseWriter

[PaulForgey]

What version of Go are you using (go version)?

$ go version
go version go1.15 darwin/amd64

(my complaint actually helps linux and is a non-issue on darwin, but I target linux hosts too)

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/pforgey/Library/Caches/go-build"
GOENV="/Users/pforgey/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/pforgey/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/pforgey/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/pforgey/goroot"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/pforgey/goroot/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="0"
GOMOD="/Users/pforgey/goroot/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ck/hxb66hs93wd58w5ylk4v0vfxk_3ncr/T/go-build576313242=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Assumed a simple io.Copy() between a socket and an http.ResponseWriter would go through splice()

What did you expect to see?

To be treated like io.Copy() between two spliceable FDs

What did you see instead?

A surprising, but not unreasonable workaround to issue #5660 caused by the fact we start writing headers immediately before the source reader is readable.

Instead of going through the trouble of determining if the source is a file before proceeding along the presumed sendfile (but maybe splice!) route, I have successfully tested a local patch (attached) which does a small read instead. Regardless of needsSniff(), this satisfies the sniff step, but also waits for the source to be actually readable.

A small body of < 512 (sniffLen) bytes would have little benefit from sendfile/splice, and we were already doing a syscall round trip. Would the extra and and write of 512 bytes from kernel->user->kernel have a significant impact in the overall case of the larger bodies? I'm not too sure it would, but it is a legitimate point to object with, but it wouldn't be the first example of zero copy network code slopping a little bit of the initial data portion like that. For the smaller bodies, it may actually have slightly less setup time.

(removed gist link to patch and submitting a PR instead)

[davecheney]

@PaulForgey please remove the link to your patch so it does not taint the reviewers. Please see https://golang.org/doc/contribute.html or send a PR to this repository and a robot will convert it into a gerrit change list.

[gopherbot]

Change https://golang.org/cl/249238 mentions this issue: net/http: improve response.ReadFrom()

[ianlancetaylor]

CC @bradfitz

[odeke-em]

@PaulForgey thank you for the issue and thanks Dave and Ian for the issue triage and responses.
@PaulForgey, *net.TCPConn implements io.ReadFrom already https://golang.org/pkg/net/#TCPConn.ReadFrom.
I don't have a Linux computer on hand, but can you please confirm that strace with a simple hello world server that sends back a bunch of content in Go, doesn't respond to a request from cURL with splice?
If it doesn't, perhaps let's start a process of elimination to try to figure out what's going wrong.

Does the following use splice or nah? If it does, let's then slowly move that into an HTTP handlerfunc, ensuring that splice still shows up

package main

import (
	"bufio"
	"bytes"
	"fmt"
	"io"
	"net"
	"net/http"
	"net/http/httputil"
	"strings"
)

func main() {
	ln, err := net.Listen("tcp", ":0")
	if err != nil {
		panic(err)
	}

	doneCh := make(chan bool)
	go func() {
		defer ln.Close()
		for {
			select {
			case <-doneCh:
				return

			default:
				conn, err := ln.Accept()
				if err != nil {
					panic(err)
				}
				tc, ok := conn.(*net.TCPConn)
				if !ok {
					panic(fmt.Sprintf("got %T, expected *net.TCPConn", conn))
				}
				req, err := http.ReadRequest(bufio.NewReader(tc))
				if err != nil {
					panic(err)
				}
				blob, _ := httputil.DumpRequest(req, false)
				println("Request in:", string(blob))
				sb := new(bytes.Buffer)
				sb.WriteString("HTTP/1.1 200 OK\r\nConnection: close\r\nContent-Length: 4096\r\n\r\n")
				sb.WriteString(strings.Repeat("bdfg", 1<<10))
				sb.WriteString("\n")
				n, err := io.Copy(tc, sb)
				tc.SetLinger(0)
				println("DONE", n, err)
				tc.Close()
			}
		}
	}()
	defer close(doneCh)

	buf := strings.NewReader(strings.Repeat("abcd", 1<<10))
	req, _ := http.NewRequest("POST", "http://"+ln.Addr().String(), buf)
	res, err := http.DefaultClient.Do(req)
	if err != nil {
		panic(err)
	}
	blob, err := httputil.DumpResponse(res, true)
	if err != nil {
		panic(err)
	}
	println(string(blob))
}

and if it doesn't it might be a problem in the related packages. Thank you.

[PaulForgey]

@odeke-em the issue is happening at a higher level. The ResponseWriter implements its ReaderFrom to allow the underlying writer's ReaderFrom to work under allowable circumstances. Remember, the ResponseWriter is the writer used in the http handler, not directly the underlying socket.

The problem in the original issue 5660 was response.ReadFrom() would immediately send the response header regardless of the source blocking, thus introducing an unexpected side effect if the response is written from a go routine copying from a pipe intended to send the response when ready, such as the main routine consuming the request body first.

To fix this, response.ReadFrom was further restricted to only go through its sendfile/splice path using the underlying writer's ReaderFrom interface if the source is a regular file, thus sabotaging the opportunity for this to happen from a socket (or even a pipe for that matter, which Linux can splice from).

This introduces the behavior I am complaining about in this bug. My PR for it attempts to fix the side effect in the first place so the source is unimportant and it is the underlying writer's ReaderFrom decision how and whether to handle it.

[odeke-em]

The red herring in this issue is that it broadly claims that splice won’t work, but perhaps we could say that we are fixing the problem of responding before the source is ready. I think the prior one was a bold claim which threw me off. I’ll review your CL shortly but will need to confirm that splice gets invoked. Thank you for working on this.

[PaulForgey]

The red herring in this issue is that it broadly claims that splice won’t work, but perhaps we could say that we are fixing the problem of responding before the source is ready. I think the prior one was a bold claim which threw me off. I’ll review your CL shortly but will need to confirm that splice gets invoked. Thank you for working on this.

if you are coming from a regular file, splice will work