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

x/net/http2: Transport ignores net/http.Transport.Proxy once connected #25793

Open
plambein opened this Issue Jun 8, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@plambein
Copy link

plambein commented Jun 8, 2018

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

go1.10.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="..."
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="..."
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_j/6wqhr8r57g34l96w0vt17bww0000gp/T/go-build671837991=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I've set up an HTTP2 client and server. The client does a few concurrent requests to the sever. The client's http.Transport has a Proxy function set which alters the requests's context. The http.Transport.DisableKeepAlives flag is also set to true.

Upon receiving the response, the contents of the context gets printed out.
My code in playground: https://play.golang.org/p/bx4GvEZ-suw
Can't be run in playground though, because of the HTTP/2 package and needed cert and key.
To get the cert + key run: openssl req -x509 -newkey rsa:4096 -keyout server.key -out server.pem -days 365 -nodes in the directory of the file.

What did you expect to see?

The context of every request altered, like so: (what happens if the http.Transport doesn't get configured for HTTP2)

context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)
context.Background.WithValue("Proxy", 1)

What did you see instead?

Almost none of the requests passed through the http.Transport.Proxy function, even though the no keepalives flag has been set:

context.Background.WithValue("Proxy", 1)
context.Background
context.Background.WithValue("Proxy", 1)
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background
context.Background

Not using HTTP2 on the client side seems to solve the issue completely. Playing with the 10ms processing time on the server and 10ms waiting time between starting goroutines either resolves the issue or make it worse. However this is not something you (should) have control over in a production environment.

This might be related to: #25620, however that one is on TCP level (where the http.Transport.Proxy is called, but not respected), while here the function isn't called at all.

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Jun 8, 2018

(CC: @bradfitz and @tombergan for x/net/http2.)

@bcmills bcmills added this to the Unreleased milestone Jun 8, 2018

@meirf

This comment has been minimized.

Copy link
Contributor

meirf commented Jun 10, 2018

tl;dr net/http executes Proxy whenever RoundTrip isn't delegated to alternate round tripper (altRT). When that altRT is from x/net/http2, doesn't appear that Proxy is ever being called (no reference to Proxy), which I think needs to be fixed.

If you use GODEBUG=http2debug=1, the number of times you see "http2: no cached connection was available" should be equal to the number of times you see the desired "context.Background.WithValue("Proxy", 1)". When there is no cached h2 connection match yet, net/http will executeconnectMethodForRequest before it creates the connection. connectMethodForRequest is the one and only place I see the Proxy being called - I don't see the Proxy being called anywhere in x/net/http2. If the h2 connection pool does have a connection match, it will run its own RoundTrip.

See for more details: Slide A, Slide B, Video.

Also I don’t think DisableKeepAlive has any effect on http2 which uses long lived connections.

@plambein

This comment has been minimized.

Copy link

plambein commented Jun 11, 2018

@meirf

Also I don’t think DisableKeepAlive has any effect on http2 which uses long lived connections.

Actually this has been implemented here, although the connection closes "as soon as idle": #14008

@meirf

This comment has been minimized.

Copy link
Contributor

meirf commented Jun 16, 2018

cc: @odeke-em

@bradfitz bradfitz changed the title x/net/http2: http.Transport.Proxy not always called on http2 clients x/net/http2: Transport ignores net/http.Transport.Proxy once connected Jul 13, 2018

@bradfitz bradfitz added the NeedsFix label Jul 13, 2018

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Jul 13, 2018

This might be tricky to fix with the current interface between the two packages. It'll need some thinking. Worst case we could document it at least, but I'd rather fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment