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: with Transfer-Encoding: chunked, Transport doesn't reuse the tcp connection #15703

Closed
jerome-laforge opened this issue May 16, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@jerome-laforge
Copy link

commented May 16, 2016

  1. With both below versions:

    • go version go1.6.2 linux/amd64
    • tip devel +b66b97e Mon May 16 15:05:04 2016 +0000,
  2. On Arch Linux, go env:

    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="linux"
    GOOS="linux"
    GOPATH="/home/jerome/go"
    GORACE=""
    GOROOT="/usr/lib/go"
    GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="gcc"
    GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
    CXX="g++"
    
  3. With this program https://play.golang.org/p/HHqlKiVR4o

  4. I expect that http client reuses the tcp connection for first and second http request.
    The problem is describe more deeply here:
    go-kit/kit#249

  5. Sometime, the http client doesn't reuse the tcp connection between first and second http request (I check it with Wireshark filter on port given into the log e.g. tcp.port == 35068).

    When the tcp connection is not reused, delve shows me that (https://github.com/golang/go/blob/master/src/net/http/internal/chunked.go#L61) cr.r.Buffered() return 0, and EOF isn't read (https://github.com/golang/go/blob/master/src/net/http/internal/chunked.go#L56), as the last 0 that indicates the end of chunked response isn't read.
    So http client send a tcp reset to http server and open a new tcp connection.

    If I drain the response.Body (with io.Copy(ioutil.Discard, resp.Body)), the tcp connection is always reused by the http client (even is cr.r.Buffered() returns 0)

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 16, 2016

More interesting is that removing the redundant "Connection: keep-alive" header (which is implicit with HTTP/1.1) seems to matter. It usually (also odd) works when you don't set that header.

Related previous stuff: rev 18072ad and #14867 and google/go-github#317

@bradfitz bradfitz self-assigned this May 16, 2016

@bradfitz bradfitz added this to the Go1.7 milestone May 16, 2016

@bradfitz bradfitz changed the title with Transfer-Encoding: chunked, http client doesn't reuse the tcp connection net/http: with Transfer-Encoding: chunked, Transport doesn't reuse the tcp connection May 16, 2016

@bradfitz bradfitz removed this from the Go1.7 milestone May 18, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 18, 2016

Actually, I had misread your repro code the first time. This is working as intended.

The lack of connection of reuse (that I see 40-60% of the time with your repo) isn't due to chunked encoding, but because the handler hasn't yet finished when you call resp.Body.Close().

If you put a time.Sleep(10 * time.Millisecond) or even shorter after your flush and before the handler, you can see that it it doesn't reuse the connection 100% of the time.

And if you move the rw.(http.Flusher).Flush() call to before the JSON write (so you're still sending chunked encoding), then you see it reuses the connection 100% of the time.

The race you're seeing is because depending on goroutine scheduling.

You have two goroutines, S(erver) and C(lient):

  • S: write, flush, send EOF (at handler func return)
  • C: decode, close

You're seeing them sometimes scheduled like this:

  • S: write
  • C: decode
  • C: close (at it hasn't seen EOF yet, so it's right to close the connection and not reuse it)
  • S: flush / send EOF (neither of which matter, since the client has hung up on us anyway)

That is a valid result.

I recommend you remove the flush.

I've added a new test in https://golang.org/cl/23200 to guarantee the existing behavior when the flush is in the right spot.

Please let me know if I misunderstand something.

@bradfitz bradfitz closed this May 18, 2016

@gopherbot

This comment has been minimized.

Copy link

commented May 18, 2016

CL https://golang.org/cl/23200 mentions this issue.

gopherbot pushed a commit that referenced this issue May 18, 2016

net/http: add test confirming a connection reuse case
Verify that for a server doing chunked encoding, with the final data
and EOF arriving together, the client will reuse the connection even
if it closes the body without seeing an EOF. The server sends at least
one non-zero chunk and one zero chunk. This verifies that the client's
bufio reading reads ahead and notes the EOF, so even if the JSON
decoder doesn't read the EOF itself, as long as somebody sees it, a
close won't forcible tear down the connection. This was true at least
of https://golang.org/cl/21291

No code change. Test already passed (even with lots of runs, including
in race mode with randomized goroutine scheduling).

Updates #15703

Change-Id: I2140b3eec6b099b6b6e54f153fe271becac5d949
Reviewed-on: https://go-review.googlesource.com/23200
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Gerrand <adg@golang.org>

@golang golang locked and limited conversation to collaborators May 18, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.