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/trace: export http.Handlers #20478

Closed
peterbourgon opened this issue May 24, 2017 · 6 comments
Closed

x/net/trace: export http.Handlers #20478

peterbourgon opened this issue May 24, 2017 · 6 comments
Milestone
Unreleased

Comments

@peterbourgon
Copy link

peterbourgon commented May 24, 2017 •
edited

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.
@gopherbot gopherbot added this to the Unreleased milestone May 24, 2017
@zombiezen
Copy link
Contributor

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

@Bo0mer Bo0mer mentioned this issue Jun 16, 2017
Closed
@ahmetb
Copy link

ahmetb commented Jun 16, 2017 •
edited

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
Copy link
Contributor

rakyll commented Jun 16, 2017

@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.

@rakyll rakyll self-assigned this Jun 16, 2017
@zombiezen
Copy link
Contributor

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
Copy link
Contributor

rakyll commented Jun 20, 2017

Closed via golang/net@1816238.

@rakyll rakyll closed this as completed Jun 20, 2017
@roadrunner
Copy link

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.

@golang golang locked as resolved and limited conversation to collaborators Feb 3, 2018
@rsc rsc unassigned rakyll Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@roadrunner @rakyll @peterbourgon @ahmetb @zombiezen @gopherbot