x/net/trace: export http.Handlers

[peterbourgon]

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

go version go1.8.1 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/peter"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.8.1/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.8.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/m1/g2lm29h90y1dw2gllcb2mk200000gn/T/go-build364343606=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

My program exposes an admin HTTP API, for a variety of internal routes, as well as pprof and expvar. For this it uses its own http.ServeMux (actually a gorilla/mux.Router, but it's irrelevant) into which I register routes explicitly:

r := mux.NewRouter()
r.Handle("/debug/vars", expvar.Handler())
r.HandleFunc("/debug/pprof/", pprof.Index)
r.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
r.HandleFunc("/debug/pprof/profile", pprof.Profile)
r.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
r.HandleFunc("/debug/pprof/trace", pprof.Trace)
r.Handle("/debug/pprof/block", pprof.Handler("block"))
r.Handle("/debug/pprof/goroutine", pprof.Handler("goroutine"))
r.Handle("/debug/pprof/heap", pprof.Handler("heap"))
r.Handle("/debug/pprof/threadcreate", pprof.Handler("threadcreate"))
myFoo.Register(r.PathPrefix("/admin/foo/").Subrouter())
myBar.Register(r.PathPrefix("/admin/bar/").Subrouter())
myBaz.Register(r.PathPrefix("/admin/baz/").Subrouter())
r.Handle("/", rootHandler(r))
return http.Serve(adminListener, r)

I'm now wanting to expose the x/net/trace handlers (specifically /debug/events, for go-conntrack).

What did you expect to see?

I expected to be able to do something like

r.HandleFunc("/debug/requests", trace.Requests)
r.HandleFunc("/debug/events", trace.Events)

What did you see instead?

No such Handler functions are exposed by the trace package. Instead, the trace package assumes its users will always want it to register its routes into the http.DefaultServeMux, and performs those registrations as an import side effect, as part of package init.

Please expose the code in func init as individual handlers, so users who don't use the DefaultServeMux can still have access to x/net/trace functionality.

[zombiezen]

@rakyll @hyangah Seems fine to me, thoughts? Are there implications on other /debug/ handlers? net/http/pprof seems to allow this.

[ahmetb]

Furthermore, having init() registering handlers causes multiple registration issues (panic below) when some of the code uses x/net/trace from $GOPATH/src/golang.org/x/net/trace, and other parts of the code use it from vendor/golang.org/x/net/trace. Both imports run their init, and since they are separate packages due to their import path, the program panics:

panic: http: multiple registrations for /debug/requests

goroutine 1 [running]:
net/http.(*ServeMux).Handle(0x18a7780, 0x15d8e89, 0xf, 0x185a8e0, 0x15f0948)
	/usr/local/Cellar/go/1.8.3/libexec/src/net/http/server.go:2254 +0x610
net/http.(*ServeMux).HandleFunc(0x18a7780, 0x15d8e89, 0xf, 0x15f0948)
	/usr/local/Cellar/go/1.8.3/libexec/src/net/http/server.go:2286 +0x55
net/http.HandleFunc(0x15d8e89, 0xf, 0x15f0948)
	/usr/local/Cellar/go/1.8.3/libexec/src/net/http/server.go:2298 +0x4b
github.com/ahmetb/gcb-dashboard/vendor/golang.org/x/net/trace.init.1()
	/Users/ahmetb/workspace/gopath-gcb-dashboard/src/github.com/ahmetb/gcb-dashboard/vendor/golang.org/x/net/trace/trace.go:121 +0x42
github.com/ahmetb/gcb-dashboard/vendor/golang.org/x/net/trace.init()
	/Users/ahmetb/workspace/gopath-gcb-dashboard/src/github.com/ahmetb/gcb-dashboard/vendor/golang.org/x/net/trace/trace_go17.go:22 +0x1cd
github.com/ahmetb/gcb-dashboard/vendor/google.golang.org/grpc.init()
...

[rakyll]

@zombiezen, I'd like to survey all global registrations and export handlers where we don't. I am strongly +1ing to export handlers from x/net/trace.

@ahmetb, are you vendoring in libraries? Unfortunately, libraries vendoring their dependencies is not a properly supported feature, we suggest only the main packages should vendor dependencies. I'd love to avoid global registration on the default mux, but these inits have been put in place in the past and I am not sure if we can break the behavior -- especially the standard library's.

[zombiezen]

Note that this bug is for golang.org/x/net/trace handlers, not the net/http/pprof ones. No strict compatibility promise, but silent breakage would be bad.

[rakyll]

Closed via golang/net@1816238.

[roadrunner]

considering a popular golang library like this; it's most likely to get imported by multiple different packages.
so, i think it should allow us to handle errors safely.
i just can't understand the point of making panicable calls in the very init func?
please, at least, consider wrapping it up with sync once or something-else fits the idiomatic way of go.