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

Jump to bottom

x/net/http2: setting Request.Close doesn't close TCP connections #49375

Closed
tatsuhiro-t opened this issue Nov 5, 2021 · 8 comments
Closed

x/net/http2: setting Request.Close doesn't close TCP connections #49375

tatsuhiro-t opened this issue Nov 5, 2021 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@tatsuhiro-t
Copy link

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

$ go version
go version go1.16.10 linux/amd64

Does this issue reproduce with the latest release?

go1.16.10, which is the latest release of go1.16, is affected.

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

go env Output
$ go env
GOHOSTARCH="amd64"
GOHOSTOS="linux"

What did you do?

go run the source code below:

package main

import (
        "context"
        "fmt"
        "io"
        "net"
        "net/http"
)

func main() {
        tr := http.DefaultTransport.(*http.Transport).Clone()

        dialContext := tr.DialContext

        tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
                conn, err := dialContext(ctx, network, addr)
                if err != nil {
                        return conn, err
                }

                fmt.Printf("%v -> %v\n", addr, conn.RemoteAddr())

                return conn, nil
        }

        c := http.Client{
                Transport: tr,
        }

        for i := 0; i < 2; i++ {
                req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://example.com", nil)
                if err != nil {
                        panic(err)
                }

                req.Close = true

                resp, err := c.Do(req)
                if err != nil {
                        panic(err)
                }
                defer resp.Body.Close()

                io.ReadAll(resp.Body)

                fmt.Println("body read")
        }
}

With go1.16.9, it outputs:

example.com:443 -> 93.184.216.34:443
body read
example.com:443 -> 93.184.216.34:443
body read

IP address lookup is done per request and connection is not reused. Note that connection is HTTP/2.

With go1.16.10, it outputs:

example.com:443 -> 93.184.216.34:443
body read
body read

Notice that IP address lookup is done once and the connection is reused for the second request.
This contradicts the documentation of Request.Close:

	// Close indicates whether to close the connection after
	// replying to this request (for servers) or after sending this
	// request and reading its response (for clients).
	//
	// For server requests, the HTTP server handles this automatically
	// and this field is not needed by Handlers.
	//
	// For client requests, setting this field prevents re-use of
	// TCP connections between requests to the same hosts, as if
	// Transport.DisableKeepAlives were set.
	Close bool

If Transport.DisableKeepAlives is set to true, go1.16.10 does not reuse connection.

So go1.16.10 has a different behaviour between Transport.DisableKeepAlives and Request.Close.

What did you expect to see?

go1.16.10 should not reuse connection if Request.Close is set to true.

What did you see instead?

go1.16.10 reuses connection even if Request.Close is set to true.
@seankhliao seankhliao changed the title go1.16.10 does not close http2 connection even if Request.Close is set to true x/net/http2: setting Request.Close doesn't close TCP connections Nov 5, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 5, 2021
@seankhliao
Copy link
Member

cc @neild @tombergan

@neild
Copy link
Contributor

neild commented Nov 5, 2021

Missed because our HTTP/2 tests for connection reuse call ("http2".Transport).RoundTrip directly, and don't exercise the more common path where net/http manages creating new connections.

Sorry about that.

@neild
Copy link
Contributor

neild commented Nov 5, 2021

I think there's a preexisting bug in here as well.

Request.Close states that setting it prevents re-use of TCP connections. However, for HTTP/2 connections (even prior to the introduction of this bug), a request with Close set can be sent on a connection with in-flight requests on it.

@gopherbot
Copy link

Change https://golang.org/cl/361498 mentions this issue: http2: close conns after use when req.Close is set

@neild
Copy link
Contributor

neild commented Nov 12, 2021

@gopherbot Please open backport issues for 1.16 and 1.17

This was referenced Nov 12, 2021
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #49560 (for 1.16), #49561 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

Change https://golang.org/cl/368375 mentions this issue: [internal-branch.go1.17-vendor] http2: close conns after use when req.Close is set

@gopherbot
Copy link

Change https://golang.org/cl/368374 mentions this issue: [internal-branch.go1.16-vendor] http2: close conns after use when req.Close is set

gopherbot pushed a commit to golang/net that referenced this issue Dec 1, 2021
@neild @mknyszek
[internal-branch.go1.17-vendor] http2: close conns after use when req…
e108c19 
….Close is set

Avoid reusing connections after receiving a response to a request
for which cc.Close is set or a "Connection: close" header is present.

Adjust the tests for connection reuse to test both the case where
new conns are created by the http2 package and when they are
created by the net/http package.

For golang/go#49375
Fixes golang/go#49561

Change-Id: I58594972c832a20b66a3910c17acb51a98a9f7a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/361498
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 69e39ba)
Reviewed-on: https://go-review.googlesource.com/c/net/+/368375
Reviewed-by: Michael Knyszek <mknyszek@google.com>
gopherbot pushed a commit to golang/net that referenced this issue Dec 1, 2021
@neild @mknyszek
[internal-branch.go1.16-vendor] http2: close conns after use when req…
26ec667 
….Close is set

Avoid reusing connections after receiving a response to a request
for which cc.Close is set or a "Connection: close" header is present.

Adjust the tests for connection reuse to test both the case where
new conns are created by the http2 package and when they are
created by the net/http package.

For golang/go#49375
Fixes golang/go#49560

Change-Id: I58594972c832a20b66a3910c17acb51a98a9f7a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/361498
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 69e39ba)
Reviewed-on: https://go-review.googlesource.com/c/net/+/368374
Reviewed-by: Michael Knyszek <mknyszek@google.com>
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
@neild
http2: close conns after use when req.Close is set
ed8823d 
Avoid reusing connections after receiving a response to a request
for which cc.Close is set or a "Connection: close" header is present.

Adjust the tests for connection reuse to test both the case where
new conns are created by the http2 package and when they are
created by the net/http package.

Fixes golang/go#49375

Change-Id: I58594972c832a20b66a3910c17acb51a98a9f7a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/361498
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
fedosgad pushed a commit to fedosgad/oohttp that referenced this issue Jun 22, 2022
@neild @mknyszek
[internal-branch.go1.17-vendor] http2: close conns after use when req…
2bb3bba 
….Close is set

Avoid reusing connections after receiving a response to a request
for which cc.Close is set or a "Connection: close" header is present.

Adjust the tests for connection reuse to test both the case where
new conns are created by the http2 package and when they are
created by the net/http package.

For golang/go#49375
Fixes golang/go#49561

Change-Id: I58594972c832a20b66a3910c17acb51a98a9f7a5
Reviewed-on: https://go-review.googlesource.com/c/net/+/361498
Trust: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
(cherry picked from commit 69e39bad7dc2bbb411fa35755c46020969029fa7)
Reviewed-on: https://go-review.googlesource.com/c/net/+/368375
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@golang golang locked and limited conversation to collaborators Dec 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@neild @tatsuhiro-t @gopherbot @seankhliao