net/http: Request body is not closed by Transport.RoundTrip on some net.Conn errors

[egorbunov]

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

$ go version
go version go1.17.3 linux/amd64

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="/home/eg/.cache/go-build"
GOENV="/home/eg/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/eg/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/eg/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.3"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/usr/lib/go/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 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2074510398=/tmp/go-build -gno-record-gcc-switches"

What did you do?

1. Wrap net.Listener with error injections
2. Run http.Server using such listener
3. Run many concurrent requests targeting such server
4. On every request, at the client side, wait for request body to be closed

Kind of a minimal example to run described steps: https://play.golang.com/p/lku8lEgiPu6
Yeah, I know that reproducer feels big, but the most of it is to create error injections actually, but
main part (see main fn) is straightforward and simple.

What did you expect to see?

I expect that net/http eventually closes Request.Body as it is told in documentation for RoundTripper: https://pkg.go.dev/net/http#RoundTripper

What did you see instead?

Specified example program hangs indefinitely while waiting for Request.Body to be closed.

Investigation

• It is possible that encountered behavior has something to deal with errServerClosedIdle error inside net/http, because we have met described hangs only after receiving that error from Client.Do.
• Also the fact that I am doing Post request may be important because net/http does not retry Post (non-idepotent) request on such error.

In case next assumption is true:

Once request execution got to func (pc *persistConn) roundTrip(req *transportRequest) call, Request.Body closure is expected to be done by func (pc *persistConn) writeLoop().

Then it seems like there is a possibility of not closing body in the writeLoop in case pc is closed concurrently.
Imagine:

                   ---------------------------------------------------------------------------------------> time
gor1               pc.writech <- writeRequest{...}                                   
gor2                   pc.closeWithError(...)
writeLoop                                                              select { case <-pc.closech: return }
                                                                                              

So request was sent successfully into pc.writech (channel has buffer of size 1), but won't be ever read from it because writeLoop exited.

Related places in the code:

1. go/src/net/http/transport.go

   Line 2383 in f659183

   func (pc *persistConn) writeLoop() {

2. go/src/net/http/transport.go

   Line 2595 in f659183

   pc.writech <- writeRequest{req, writeErrCh, continueCh}

[Asuan]

There are workaround:
add line rw.Header().Add("Content-Length", "1") or rw.Write([]byte("ok")) after line 113 it solve problem.
Looks like problem only in case no body in response from server.

[heschi]

cc @neild

[egorbunov]

There are workaround: add line rw.Header().Add("Content-Length", "1") or rw.Write([]byte("ok")) after line 113 it solve problem. Looks like problem only in case no body in response from server.

Not really because adding correct Content-Length with actual response body of Content-Length bytes does not solve anything and reproducer continues to hang.

[ianlancetaylor]

@neild This is in the 1.18 milestone; time to move to 1.19? Thanks.

[odeke-em]

Yes please, looks like this shall be moved to Go1.19 as we haven't had any movement for almost 3 months. I shall move it to the next milestone. Thank you everyone for chiming.

[ianlancetaylor]

CC @neild

Rolling forward to 1.20.