Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
egorbunov opened this issue Nov 16, 2021 · 11 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@egorbunov
Copy link

egorbunov commented Nov 16, 2021

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. func (pc *persistConn) writeLoop() {
  2. pc.writech <- writeRequest{req, writeErrCh, continueCh}
@egorbunov egorbunov changed the title Request body is not closed by Transport.RoundTrip upon underlying net.Conn errors http.Request body is not closed by Transport.RoundTrip upon underlying net.Conn errors Nov 16, 2021
@egorbunov egorbunov changed the title http.Request body is not closed by Transport.RoundTrip upon underlying net.Conn errors http.Request body is not closed by Transport.RoundTrip on some net.Conn errors Nov 16, 2021
@egorbunov egorbunov changed the title http.Request body is not closed by Transport.RoundTrip on some net.Conn errors net/http: Request body is not closed by Transport.RoundTrip on some net.Conn errors Nov 16, 2021
@Asuan
Copy link
Contributor

Asuan commented Nov 17, 2021

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 heschi added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 22, 2021
@heschi heschi added this to the Go1.18 milestone Nov 22, 2021
@heschi
Copy link
Contributor

heschi commented Nov 22, 2021

cc @neild

@egorbunov
Copy link
Author

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
Copy link
Contributor

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

@odeke-em
Copy link
Member

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.

@odeke-em odeke-em modified the milestones: Go1.18, Go1.19 Feb 12, 2022
@ianlancetaylor
Copy link
Contributor

CC @neild

Rolling forward to 1.20.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
Aoang added a commit to Aoang/go that referenced this issue Dec 20, 2022
When there is still a request in writech after
writeLoop exits, the request body will not be
closed and will not be retried.
The request is not sent to the server, so it can
be safely retried without regard to idempotence.

Fixes golang#49621
@gopherbot
Copy link

Change https://go.dev/cl/458437 mentions this issue: net/http: fix when writeLoop exited still has request in writech

@gopherbot
Copy link

Change https://go.dev/cl/461675 mentions this issue: net/http: close Request.Body when pconn write loop exits early

@twmb
Copy link
Contributor

twmb commented May 9, 2023

Potentially related to #26408 and #26409.

@Nytonecombek
Copy link

Potentially related to #26408 and #26409.

1 similar comment
@Nytonecombek
Copy link

Potentially related to #26408 and #26409.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Development

Successfully merging a pull request may close this issue.

8 participants