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

x/net/trace: init registers an endpoint on the http.DefaultServeMux, causing a panic if vendored in some cases #24137

Closed
jadekler opened this issue Feb 26, 2018 · 23 comments

Comments

@jadekler
Copy link
Member

x/net registers an endpoint in the http global. When this package is imported multiple times (because other packages I use import it) causes:

panic: http: multiple registrations for /debug/requests
@gopherbot gopherbot added this to the Unreleased milestone Feb 26, 2018
@dgryski
Copy link
Contributor

dgryski commented Feb 26, 2018

Duplicate of #20478

@jadekler
Copy link
Member Author

Thanks @dgryski. I'll leave this open unless maintainers decide to close, since it remains an issue.

@andybons
Copy link
Member

Duplicate of #20478

@andybons andybons marked this as a duplicate of #20478 Mar 26, 2018
@jadekler
Copy link
Member Author

jadekler commented Mar 27, 2018

@andybons @davecheney Can I have some clarification on why #20478 is closed and locked? The thread is extremely unsatisfactory. This is an issue which causes unwanted panics when I import several packages (as in, it's not my program's fault) - this feels strongly like something that should be discussed, or at least something that should have guidance for how to deal with.

@e-nikolov
Copy link

e-nikolov commented Jul 26, 2018

#20478 Can't be both a duplicate of this and resolved since the issue discussed here is very much not resolved at this time.

The panic also happens for similar reasons when an application dynamically loads 2 different plugins if both have vendored /x/net/trace, since they will both try to register the same route.

@andybons
Copy link
Member

Spoke with @jadekler about this. Apologies for closing as a duplicate when it was a separate issue.

The issue is almost certainly with vendored dependencies, since init is only called one for a package no matter how many times it's imported within a program. If x/net is in vendor, though, it is treated as an entirely different package, and this behavior could happen.

The solution would be not to register these on http.DefaultServeMux.

It's not clear to me why the previous conversation was locked. @davecheney maybe you can provide context?

@bradfitz @mikioh

@andybons andybons reopened this Jul 26, 2018
@andybons andybons changed the title x/net: trace init registers an endpoint in the http global x/net/trace: init registers an endpoint on the http.DefaultServeMux, causing a panic if vendored in some cases Jul 26, 2018
@bradfitz
Copy link
Contributor

To some extent this problem should go away with Go modules in Go 1.11 and later, as we won't have duplicate copies of packages loaded and fighting with each other.

In some way the panic from ServeMux duplicate registration is almost a feature in that it alerts you that you've loaded two copies of x/net/trace when you almost certainly want just one in your program.

@andybons
Copy link
Member

To some extent this problem should go away with Go modules in Go 1.11 and later, as we won't have duplicate copies of packages loaded and fighting with each other.

It's still an issue if one were to have two versions of /x/net in their program (say if they were incrementally upgrading in a very large project). There are likely more possibilities of duplicate packages fighting with each other as a consequence of this.

@bradfitz
Copy link
Contributor

It's still an issue if one were to have two versions of /x/net in their program (say if they were incrementally upgrading in a very large project).

Like I said above, in that case the panic is a feature: it told you accidentally introduced a copy.

@jadekler
Copy link
Member Author

I wish I had thought ahead enough to document a repro, so that we had something concrete to discuss :(

In some way the panic from ServeMux duplicate registration is almost a feature in that it alerts you that you've loaded two copies of x/net/trace when you almost certainly want just one in your program.

Is it possible for one of my deps to have a vendored copy of x/net/trace, and meanwhile I directly depend on x/net/trace? This seems the most likely way that this occurred. Although this is probably not ideal, it seems like something that should probably work fine instead of causing a panic.

@bradfitz
Copy link
Contributor

Although this is probably not ideal, it seems like something that should probably work fine instead of causing a panic.

It's less than ideal: it means some of your traces are in one global variable and some of them are in another global variable.

(the x/net/trace activeTraces package global)

You really want just one copy in your entire program.

@jadekler
Copy link
Member Author

Ah, that makes sense. How about this: in order to aid debugging of the problem, I can add a couple lines of documentation in the init that describes this situation. Is that acceptable? Minimally I think it'd be good for folks to understand why it's a bad thing if that panic occurs.

@bradfitz
Copy link
Contributor

Whose init? If it's just a comment in the code, who will ever see it?

Ideally we'd instead make x/net/trace detect this situation (recovering the ServeMux panic?) and re-panic with a more helpful panic. ("You have two independent copies of golang.org/x/net/trace in your binary, trying to maintain separate state. That doesn't work. Boom.")

@gopherbot
Copy link

Change https://golang.org/cl/127121 mentions this issue: x/net/trace: better error message for double init

@jadekler
Copy link
Member Author

jadekler commented Aug 1, 2018

@e-nikolov
Copy link

e-nikolov commented Aug 1, 2018

In the case of Go plugins, it makes sense for each plugin to use its own vendored version of the x/net/trace package.

If they are not vendored, Go will realize that they are the same package, and will require that they be the same exact version. Otherwise you get an error saying that the plugin was built with a different version of x/net/trace and loading it will fail. This is very inconvenient since the plugins can be built separately by different people, so there is no way to have them synchronize the versions of x/net/trace and all other shared dependencies they use.

If they are both vendored, Go will think that the two packages are different because one is loaded from:

.../plugin1/vendor/x/net/trace, and the other one from:
.../plugin2/vendor/x/net/trace, which are different paths.

In this case, the init() will be executed twice and the panic will occur. The only workaround I have seen is to reinitialize the http.DefaultServeMux before loading each plugin.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 1, 2018

I think plugins just can't use separate copies of packages which are supposed to maintain global state.

You should redesign your plugin boundaries and instead create one x/net/trace and arrang to pass what's needed to your plugin at runtime instead.

@e-nikolov
Copy link

I don't think it's easy to control the dependencies that plugins have. Those could be made and built by third parties and if we'd have to provide a list of "banned" dependencies, it wouldn't be ideal.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 2, 2018

@e-nikolov, if you're consuming plugins, you set the rules. As I said before:

You should redesign your plugin boundaries and instead create one x/net/trace and arrange to pass what's needed to your plugin at runtime instead.

@e-nikolov
Copy link

e-nikolov commented Aug 2, 2018

This might be approaching off-topic, but in my particular case, plugins are used to receive messages via some medium (e.g. http, grpc, amqp, etc.) and then pass them on to the base application. Some such plugins receive messages over GRPC, which imports x/net/trace and results in a panic due to the issue outlined here.

Are you suggesting that we should be initializing all possible mediums in the base application and then supplying them to the plugins? I don't think this is workable since the base application has no way to anticipate all possible mediums and those are chosen by the plugins themselves.

In any case, our use case doesn't seem too crazy to me and it feels like it should be possible to support it without having to resort to workarounds.

I hope I am not being too annoying, I am just trying to understand better the situation and the intentions for plugins and x/net/trace.

@gotoxu
Copy link

gotoxu commented Oct 30, 2018

I support @e-nikolov's point of view. Plugins are usually developed and maintained by third parties, limiting the dependencies that plugins can use is not a good idea.

Moreover, there is still a situation. I encountered a problem when developing the hyperledger fabric system chaincode plugin. The chaincode relies on some packages of the fabric, and these packages depend on x/net/trace. If I develop plugin in a standalone project, and vendored dependencies in my project, Go will think I use two different packages. so system will be panic when the fabric loads my plugin.

If I want to solve this problem, I have to put the source code of the plugin into the fabric project, so that plugin and fabric will use the same vendor's x/net/trace package. This is not a good development style, right?

@santsai
Copy link

santsai commented Jan 10, 2019

This fix here golang/net@f4c29de can introduce false positive panic when another package have a init that does http.HandleFunc("/", mainHandler) and runs before x/net/trace's init.

This is due to http.DefaultServeMux.Handler does most specific match and not exact match. Changing https://github.com/golang/net/blob/be1c187aa6c66b9daa1d9461c228d17e9dd2cab7/trace/trace.go#L116 if pat != "" { to if pat == "/debug/requests" { can fix this.

@bradfitz
Copy link
Contributor

@santsai, thanks. I sent https://go-review.googlesource.com/c/net/+/157378 .... feel free to review there.

gopherbot pushed a commit to golang/net that referenced this issue Jan 10, 2019
From comment at @santsai at golang/go#24137 (comment)

Change-Id: Icf0aff2172811752b240d94e709550b0da353360
Reviewed-on: https://go-review.googlesource.com/c/157378
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: JBD <jbd@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants