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/httptrace: panic on GotConn #34282

Closed
cuonglm opened this issue Sep 13, 2019 · 17 comments
Closed

net/http/httptrace: panic on GotConn #34282

cuonglm opened this issue Sep 13, 2019 · 17 comments

Comments

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Sep 13, 2019

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

$ go version
go version devel +03f63654c4 Thu Sep 12 21:02:57 2019 +0000 darwin/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="/Users/cuonglm/Library/Caches/go-build"
GOENV="/Users/cuonglm/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/cuonglm/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/cuonglm/sources/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/cuonglm/sources/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/y4/hs76ltbn7sb66lw_6934kq4m0000gn/T/go-build718378058=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
	"context"
	"log"
	"net/http"
	"net/http/httptrace"
	"sync"
)

func gotConn(info httptrace.GotConnInfo) {
	_ = info.Conn.RemoteAddr
}

func doGet() {
	ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
		GotConn: gotConn,
	})

	req, _ := http.NewRequest(http.MethodGet, "https://google.com", nil)
	req = req.WithContext(ctx)

	if _, err := http.DefaultClient.Do(req); err != nil {
		log.Println(err)
	}
}

func main() {
	var wg sync.WaitGroup
	n := 1000
	wg.Add(n)

	for i := 0; i < n; i++ {
		go func() {
			defer wg.Done()
			doGet()
		}()
	}

	wg.Wait()
}

Thanks @tmthrgd for simpler and not racy program.

What did you expect to see?

Run successfully.

What did you see instead?

Panic

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12ad645]

goroutine 72 [running]:
main.gotConn(0x0, 0x0, 0x1a500101, 0xc0ce52)
	/Users/cuonglm/sources/goissues/34282/trace.go:12 +0x5
net/http.(*Transport).getConn(0x15cc420, 0xc0025eec60, 0x0, 0x135530c, 0x5, 0xc0029f1650, 0xe, 0x0, 0x0, 0x0, ...)
	/Users/cuonglm/sdk/go1.13/src/net/http/transport.go:1210 +0x394
net/http.(*Transport).roundTrip(0x15cc420, 0xc00027e900, 0x0, 0xc00020ab20, 0x100e118)
	/Users/cuonglm/sdk/go1.13/src/net/http/transport.go:522 +0x603
net/http.(*Transport).RoundTrip(0x15cc420, 0xc00027e900, 0x15cc420, 0x0, 0x0)
	/Users/cuonglm/sdk/go1.13/src/net/http/roundtrip.go:17 +0x35
net/http.send(0xc00027e900, 0x13be5a0, 0x15cc420, 0x0, 0x0, 0x0, 0xc000282050, 0x203000, 0x1, 0x0)
	/Users/cuonglm/sdk/go1.13/src/net/http/client.go:250 +0x43a
net/http.(*Client).send(0x15d1200, 0xc00027e900, 0x0, 0x0, 0x0, 0xc000282050, 0x0, 0x1, 0x13479a0)
	/Users/cuonglm/sdk/go1.13/src/net/http/client.go:174 +0xfa
net/http.(*Client).do(0x15d1200, 0xc00027e900, 0x0, 0x0, 0x0)
	/Users/cuonglm/sdk/go1.13/src/net/http/client.go:641 +0x3ce
net/http.(*Client).Do(...)
	/Users/cuonglm/sdk/go1.13/src/net/http/client.go:509
main.main.func1(0xc00001c340, 0x13c3a40, 0xc0000ceab0)
	/Users/cuonglm/sources/goissues/34282/trace.go:31 +0x18c
created by main.main
	/Users/cuonglm/sources/goissues/34282/trace.go:25 +0xeb
exit status 2

The program works in go1.12, panic on go1.13 as well as tip.

@cuonglm cuonglm changed the title net/http/httptrace: panic on GoConn net/http/httptrace: panic on GotConn Sep 13, 2019
@agnivade agnivade added this to the Go1.14 milestone Sep 13, 2019
@agnivade
Copy link
Contributor

@agnivade agnivade commented Sep 13, 2019

Want to do a bisect ? 🙂

@bradfitz

@ALTree

This comment has been hidden.

@cuonglm

This comment has been hidden.

@ALTree

This comment has been hidden.

@cuonglm

This comment has been hidden.

@ALTree

This comment has been hidden.

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Sep 13, 2019

Here is a simpler reproducer that shows the same crash (Edit: simpler still):

package main

import (
	"context"
	"log"
	"net/http"
	"net/http/httptrace"
	"sync"
)

func gotConn(info httptrace.GotConnInfo) {
	_ = info.Conn.RemoteAddr // info.Conn == nil here
}

func doGet() {
	ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{
		GotConn: gotConn,
	})

	req, _ := http.NewRequest(http.MethodGet, "https://google.com", nil)
	req = req.WithContext(ctx)

	if _, err := http.DefaultClient.Do(req); err != nil {
		log.Println(err)
	}
}

func main() {
	var wg sync.WaitGroup
	n := 1000
	wg.Add(n)

	for i := 0; i < n; i++ {
		go func() {
			defer wg.Done()
			doGet()
		}()
	}

	wg.Wait()
}
@cuonglm
Copy link
Contributor Author

@cuonglm cuonglm commented Sep 13, 2019

@ALTree The example of @tmthrgd above is not racy and produce the panic.

I updated the issue with that program, thanks @tmthrgd .

@ALTree
Copy link
Member

@ALTree ALTree commented Sep 13, 2019

In general, it's better not to edit the original message. Now people reading the thread from the start will get confused by our initial discussion about code that doesn't exist any more. I'll mark our messages as outdated to hide them.

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Sep 13, 2019

I think the problem might be some kind of race condition in queueForIdleConn around t.idleConn.

This is where the call to GotConn with a nil info.Conn is happening:

go/src/net/http/transport.go

Lines 1196 to 1199 in 98aa978

if delivered := t.queueForIdleConn(w); delivered {
pc := w.pc
if trace != nil && trace.GotConn != nil {
trace.GotConn(pc.gotIdleConnTrace(pc.idleAt))

Nothing touches w before queueForIdleConn, which can only ever return true here:

return delivered

Running the go program above in delve, I'm seeing a persistConn with conn == nil at the call to tryDeliver here:

delivered = w.tryDeliver(pconn, nil)

@cuonglm
Copy link
Contributor Author

@cuonglm cuonglm commented Sep 13, 2019

git bisect points to 43b9fcf

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Sep 13, 2019

After 43b9fcf, dialConn can return a persistConn with conn == nil here:

return &persistConn{t: t, cacheKey: pconn.cacheKey, alt: next(cm.targetAddr, pconn.conn.(*tls.Conn))}, nil

That persistConn is then being put in t.idleConn by tryPutIdleConn and pulled out by queueForIdleConn.

When there aren't any idle connections available, x/net/http2 calls GotConn itself, see here:

go/src/net/http/transport.go

Lines 1217 to 1221 in 98aa978

// Trace success but only for HTTP/1.
// HTTP/2 calls trace.GotConn itself.
if w.pc != nil && w.pc.alt == nil && trace != nil && trace.GotConn != nil {
trace.GotConn(httptrace.GotConnInfo{Conn: w.pc.conn, Reused: w.pc.isReused()})
}

Is the bug here simply that this call is missing the HTTP/2 specific check?

go/src/net/http/transport.go

Lines 1198 to 1200 in 98aa978

if trace != nil && trace.GotConn != nil {
trace.GotConn(pc.gotIdleConnTrace(pc.idleAt))
}

@cuonglm
Copy link
Contributor Author

@cuonglm cuonglm commented Sep 13, 2019

@tmthrgd w.pc.alt == nil guarantees HTTP/1

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 13, 2019

Change https://golang.org/cl/195237 mentions this issue: net/http: fix HTTP/2 idle pool tracing

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Sep 13, 2019

CL 195237 fixes the panic, but is missing a test case.


@cuonglm If you're looking for a temporary workaround, either:

http.DefaultTransport.(*http.Transport).DisableKeepAlives = true

or

http.DefaultTransport.(*http.Transport).MaxIdleConnsPerHost = -1

will stop the problematic persistConn from being put in the idle pool

@cuonglm
Copy link
Contributor Author

@cuonglm cuonglm commented Sep 13, 2019

@gopherbot please consider this for backport 1.13

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 13, 2019

Backport issue(s) opened: #34285 (for 1.13).

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.