net/http: EnableFullDuplex without closing Request.Body panics with invalid concurrent Body.Read call

[DavidArchibald]

Go version

go version go1.22.5 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/david/.cache/go-build'
GOENV='/home/david/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/david/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/david/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.5'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/david/Projects/My Price Health/mono/go.mod'
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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build17757478=/tmp/go-build -gno-record-gcc-switches

What did you do?

While I encountered this with a full fledged server I simplified it down to this script:

package main

import "net/http"

func main() {
    s := &http.Server{
        Addr: "localhost:8080",
        Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            err := http.NewResponseController(w).EnableFullDuplex()
            if err != nil {
                panic(err)
            }
        }),
    }

    err := s.ListenAndServe()
    if err != nil {
        panic(err)
    }
}

I then narrowed down the issue to when I make a request like so:

import requests

requests.request("GET", "http://localhost:8080", json={})

I don't have this problem when I try to reproduce the issue in Postman or craft the request myself, even when I make it obstensibly identical. Likely because of differences in client behaviours.

What did you see happen?

The program panicked with this result:

2024/07/23 15:29:42 http: panic serving 127.0.0.1:36822: invalid concurrent Body.Read call
goroutine 20 [running]:
net/http.(*conn).serve.func1()
        /usr/local/go/src/net/http/server.go:1903 +0xbe
panic({0x63b1c0?, 0x6ff850?})
        /usr/local/go/src/runtime/panic.go:770 +0x132
net/http.(*connReader).Read(0xc00011b0b0, {0xc000244000, 0x1000, 0x1000})
        /usr/local/go/src/net/http/server.go:768 +0x22a
bufio.(*Reader).fill(0xc00012a360)
        /usr/local/go/src/bufio/bufio.go:110 +0x103
bufio.(*Reader).Peek(0xc00012a360, 0x4)
        /usr/local/go/src/bufio/bufio.go:148 +0x53
net/http.(*conn).serve(0xc00023e000, {0x702b90, 0xc00011afc0})
        /usr/local/go/src/net/http/server.go:2079 +0x749
created by net/http.(*Server).Serve in goroutine 1
        /usr/local/go/src/net/http/server.go:3290 +0x4b4

What did you expect to see?

I expected no panic. Perhaps there's a reason that enabling EnableFullDuplex is bad in a handler like this but I couldn't find any documentation that suggested this would be harmful.

I could manage to get this panic to go away by moving this handler a bit later into the actual router I have but this seems to indicate to me that it's actually a race condition or something.

[gabyhelp]

Related Issues and Documentation

• net/http: TimeoutHandler panics with concurrent requests #8209 (closed)
• net/http: http.TimeoutHandler rewrites panic stack trace #51587
• net/http: panic "slice bounds out of range" while reading chunked response body #22330 (closed)
• net/http: on panic http.server re-runs the handler multiple times on Windows #57588 (closed)
• net/http: Expect 100-continue panics in httputil.ReverseProxy #34902 (closed)
• x/net/http2: hpack.(*Encoder).WriteField panic errors #47882 (closed)
• net/http: data race in net/http.(*expectContinueReader) #43769 (closed)
• net/http: pc close panics #60862 (closed)
• net/http: panic: runtime error: slice bounds out of range #12339 (closed)
• when use standard http client panic id (14) \<= evictCount (14) #43048 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

Any request with a body will do, e.g. curl http://localhost:8080 -d '{}'. Python isn't necessary to make the request.

Looks like the concurrent read happens when the server tries to drain the body in preparation for reusing the connection. Adding either of the below avoids the panic:

• defer r.Body.Close() to the handler
• marking the connection non-reusable with -H 'connection: close' from the client side

[seankhliao]

cc @neild

[DavidArchibald]

marking the connection non-reusable with -H 'connection: close' from the client side

That makes sense. I can confirm that the other clients I tried were adding connection: close to their headers. This is why I ended up including Python in my attempt at minimum reproduction because I couldn't figure out how to distill it down.

[Tachone]

Any request with a body will do, e.g. curl http://localhost:8080 -d '{}'. Python isn't necessary to make the request.

Looks like the concurrent read happens when the server tries to drain the body in preparation for reusing the connection. Adding either of the below avoids the panic:

• defer r.Body.Close() to the handler
• marking the connection non-reusable with -H 'connection: close' from the client side

@seankhliao When the server has EnableFullDuplex enabled, should it be specified in the documentation that req.Body.Close() needs to be called explicitly, since the transport no longer handles closing it? @neild Could you share your thoughts on this?