net/http: flush after writing 101 header shouldn't send headers again

[timofurrer]

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

$ go version
go version go1.20 darwin/arm64

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="arm64"
GOBIN=""
GOCACHE="/Users/timo/Library/Caches/go-build"
GOENV="/Users/timo/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/timo/.asdf/installs/golang/1.20/packages/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/timo/.asdf/installs/golang/1.20/packages"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/timo/.asdf/installs/golang/1.20/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/timo/.asdf/installs/golang/1.20/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/dn/652s0ll15hl3_3nsnrbfm3c00000gn/T/go-build303266259=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

The change set https://go-review.googlesource.com/c/go/+/269997 (#26089, #36734) for Go 1.19 introduced that for all 1xx status codes the headers are persisted for subsequent writes - my assumption is that this is mostly for 100 Continue.

However, when we do a w.WriteHeader(http.StatusSwitchingProtocols) (status code is 101 here) the internal w.wroteHeader isn't set to true (due to the early return in the if for 1xx responses.:

go/src/net/http/server.go

Line 1176 in bc5b194

w.wroteHeader = true

... this leads to another w.WriteHeader(http.StatusOK) in a subsequent Flush:

go/src/net/http/server.go

Line 1713 in bc5b194

w.WriteHeader(StatusOK)

... which is not my expectation here. A subsequent flush for (at least) a 101 response should just flush / no-op without sending that http.StatusOK + headers.

Note: if I have some time I'll try to create a unit test ..., but here is an exploratory example:

package main

import (
        "log"
        "net/http"
)

func RequestHandler(w http.ResponseWriter, r *http.Request) {
        w.Header().Add("X-Test", "true")
        w.WriteHeader(http.StatusSwitchingProtocols)
        f := w.(http.Flusher)
        f.Flush()
}

func main() {
        http.HandleFunc("/", RequestHandler)
        log.Fatal(http.ListenAndServe(":3030", nil))
}

Then e.g. run curl -v localhost:3030 and observe the verbose output, or use telnet localhost 3030 to make a manual request and check the raw TCP output:

$ telnet localhost 3030
Trying ::1...
Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.1
Host: localhost:3030

HTTP/1.1 101 Switching Protocols
X-Test: true

HTTP/1.1 200 OK
X-Test: true
Date: Wed, 12 Apr 2023 10:52:36 GMT
Transfer-Encoding: chunked

0

Connection closed by foreign host.

... as you can see the second HTTP/1.1 200 OK response is not expected.

What did you expect to see?

That a subsequent flush after w.WriteHeader(http.StatusSwitchingProtocols) doesn't do a w.WriteHeader(http.StatusOk).

What did you see instead?

A subsequent flush after w.WriteHeader(http.StatusSwitchingProtocols) does a w.WriteHeader(http.StatusOk).

[ianlancetaylor]

CC @neild @bradfitz

[neild]

Writing a 101 Switching Protocols header indicates that what follows on the connection is no longer HTTP. The net/http server doesn't have any specific handling for WriteHeader(101), but perhaps it should refuse to write any further data to the connection (headers or otherwise).

Prior to Go 1.19, WriteHeader explicitly did not support writing 1xx headers, including 101. (Although I believe WriteHeader(101) would actually send a 101 header, contrary to the documentation.) When switching protocols, I'd recommend either writing the 101 header manually after hijacking the connection, or hijacking immediately after calling WriteHeader.

[timofurrer]

The net/http server doesn't have any specific handling for WriteHeader(101),

I wonder if it should 🤔 Which basically would mean that a subsequent Write() / Flush() after the WriteHeader(101) wouldn't implicitly send headers (with an OK status code).

but perhaps it should refuse to write any further data to the connection (headers or otherwise).

Just to clarify for me, with "refuse to write any further data to the connection" you mean without having hijacked it?

(Although I believe WriteHeader(101) would actually send a 101 header, contrary to the documentation.)

I believe this to be true, because it "used to work" for us.

or hijacking immediately after calling WriteHeader.

That seems like a good workaround for now (at the moment we just don't flush after a WriteHeader(101)).

EDIT: We actually do hijack after calling WriteHeader - but inside some proxy code which is the same for upgraded and non-upgraded connection except that we now have to conditionally flush after WriteHeader() because a flush after WriteHeader(101) would send that not-wanted response.

When switching protocols, I'd recommend either writing the 101 header manually after hijacking the connection,

That or conditionally flush are our options for a workaround. Both some suboptimal and I think it would be great for net/http to support 101 Switching Protocols in a sense that after the 101 response has been sent, it won't send any response data (headers and others) implicitly on a flush. WDYT?

[neild]

Digging into this some more, while the pre-1.19 WriteHeader documentation claims no support for writing 1xx headers, there is special handling of a 101 response in two places. We normally strip user-defined Connection headers from the response, but not when sending a 101 response (this was issue #36381), and writes to the response body would return an error after writing a 101 header.

Since the previous behavior was clearly intentional (and pretty much exactly what we seem to agree is reasonable), I think this is a regression even though that behavior wasn't documented.

[gopherbot]

Change https://go.dev/cl/485775 mentions this issue: net/http: handle WriteHeader(101) as a non-informational header

[timofurrer]

@neild thanks for the responsive fix 🎉

I think that's it and I guess this issue can be closed?