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

timofurrer · 2023-04-12T08:34:01Z

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).

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2023-04-12T16:33:10Z

CC @neild @bradfitz

neild · 2023-04-17T17:13:28Z

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 · 2023-04-18T05:44:06Z

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 · 2023-04-18T16:03:16Z

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 · 2023-04-18T16:03:49Z

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

timofurrer · 2023-04-21T13:52:18Z

@neild thanks for the responsive fix 🎉

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

Prior to Go 1.19 adding support for sending 1xx informational headers with ResponseWriter.WriteHeader, WriteHeader(101) would send a 101 status and disable further writes to the response. This behavior was not documented, but is intentional: Writing to the response body explicitly checks to see if a 101 status has been sent before writing. Restore the pre-1.19 behavior when writing a 101 Switching Protocols header: The header is sent, no subsequent headers are sent, and subsequent writes to the response body fail. For #59564 Change-Id: I72c116f88405b1ef5067b510f8c7cff0b36951ee Reviewed-on: https://go-review.googlesource.com/c/go/+/485775 Reviewed-by: Bryan Mills <bcmills@google.com> Auto-Submit: Damien Neil <dneil@google.com> Run-TryBot: Damien Neil <dneil@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>

dr2chase added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 13, 2023

seankhliao closed this as completed May 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timofurrer commented Apr 12, 2023 •

edited

ianlancetaylor commented Apr 12, 2023

neild commented Apr 17, 2023

timofurrer commented Apr 18, 2023 •

edited

neild commented Apr 18, 2023

gopherbot commented Apr 18, 2023

timofurrer commented Apr 21, 2023

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

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

Comments

timofurrer commented Apr 12, 2023 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

ianlancetaylor commented Apr 12, 2023

neild commented Apr 17, 2023

timofurrer commented Apr 18, 2023 • edited

neild commented Apr 18, 2023

gopherbot commented Apr 18, 2023

timofurrer commented Apr 21, 2023

timofurrer commented Apr 12, 2023 •

edited

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

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

timofurrer commented Apr 18, 2023 •

edited