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

net/http/httptest: 'Server.CloseClientConnections' triggers panic in 'Server.Close` call #19729

Closed
gyuho opened this issue Mar 27, 2017 · 2 comments
Closed

net/http/httptest: 'Server.CloseClientConnections' triggers panic in 'Server.Close` call #19729

gyuho opened this issue Mar 27, 2017 · 2 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.9

Comments

@gyuho
Copy link
Contributor

gyuho commented Mar 27, 2017 •
edited

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

go master branch as of today

go version devel +ecc6a81 Sat Mar 25 00:35:35 2017 +0000 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/gyuho/go"
GORACE=""
GOROOT="/home/gyuho/go-master"
GOTOOLDIR="/home/gyuho/go-master/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build055947270=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
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"

What did you do?

etcd(https://github.com/coreos/etcd/blob/master/integration/cluster.go#L717-L720) calls

for _, hs := range m.hss {
	hs.CloseClientConnections()
	hs.Close()
}

where hs is *httptest.Server

What did you expect to see?

It should not panic from nil pointer dereference in the following httptest.Server.Close

What did you see instead?

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

goroutine 26 [running]:
testing.tRunner.func1(0xc42017cf70)
	/home/gyuho/go-master/src/testing/testing.go:624 +0x25d
panic(0xc7f6c0, 0x1266d40)
	/home/gyuho/go-master/src/runtime/panic.go:489 +0x26a
net/http/httptest.(*Server).Close(0xc4200679d0)
	/home/gyuho/go-master/src/net/http/httptest/server.go:212 +0x2a2
github.com/coreos/etcd/integration.(*member).Close(0xc4201a8240)
	/home/gyuho/go/src/github.com/coreos/etcd/integration/cluster.go:719 +0xc3
github.com/coreos/etcd/integration.(*member).Terminate(0xc4201a8240, 0xc42017cf70)
	/home/gyuho/go/src/github.com/coreos/etcd/integration/cluster.go:773 +0x11b
github.com/coreos/etcd/integration.(*cluster).Terminate(0xc4201c01a0, 0xc42017cf70)
	/home/gyuho/go/src/github.com/coreos/etcd/integration/cluster.go:317 +0x56
github.com/coreos/etcd/integration.(*ClusterV3).Terminate(0xc4201e83c0, 0xc42017cf70)
	/home/gyuho/go/src/github.com/coreos/etcd/integration/cluster.go:897 +0x90
github.com/coreos/etcd/clientv3/naming.TestGRPCResolver(0xc42017cf70)
	/home/gyuho/go/src/github.com/coreos/etcd/clientv3/naming/grpc_test.go:84 +0x801
testing.tRunner(0xc42017cf70, 0xdde678)
	/home/gyuho/go-master/src/testing/testing.go:659 +0x98
created by testing.(*T).Run
	/home/gyuho/go-master/src/testing/testing.go:701 +0x2d5
exit status 2

I confirm that after CloseClientConnections, s.client in Close is nil--so panic-ed.

// Also close the client idle connections.
if t, ok := s.client.Transport.(closeIdleTransport); ok {
	t.CloseIdleConnections()
}

It was added from fbf4dd9#diff-4d34e6d5bd560e4ac6fd23ee5d9bda5fR209

Or are we doing anything wrong here?

hs.CloseClientConnections()
hs.Close()

Thanks a lot in advance!
@bradfitz
Copy link
Contributor

I see. You're creating your own httptest.Server values and not using the constructors. Okay, we can make this work again.

@bradfitz bradfitz self-assigned this Mar 27, 2017
@bradfitz bradfitz added this to the Go1.9 milestone Mar 27, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 27, 2017
@gopherbot
Copy link

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

lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
@bradfitz @lparth
net/http/httptest: don't panic on Close of user-constructed Server value
08ce1a2 
If the user created an httptest.Server directly without using a
constructor it won't have the new unexported 'client' field. So don't
assume it's non-nil.

Fixes golang#19729

Change-Id: Ie92e5da66cf4e7fb8d95f3ad0f4e3987d3ae8b77
Reviewed-on: https://go-review.googlesource.com/38710
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Kevin Burke <kev@inburke.com>
@golang golang locked and limited conversation to collaborators Mar 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
3 participants
@bradfitz @gyuho @gopherbot