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
Comments
Duplicate of #20478 |
Thanks @dgryski. I'll leave this open unless maintainers decide to close, since it remains an issue. |
Duplicate of #20478 |
@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. |
#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. |
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 The solution would be not to register these on It's not clear to me why the previous conversation was locked. @davecheney maybe you can provide context? |
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. |
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. |
Like I said above, in that case the panic is a feature: it told you accidentally introduced a copy. |
I wish I had thought ahead enough to document a repro, so that we had something concrete to discuss :(
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. |
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. |
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. |
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.") |
Change https://golang.org/cl/127121 mentions this issue: |
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: 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. |
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. |
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. |
@e-nikolov, if you're consuming plugins, you set the rules. As I said before:
|
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. |
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? |
This fix here golang/net@f4c29de can introduce false positive panic when another package have a This is due to |
@santsai, thanks. I sent https://go-review.googlesource.com/c/net/+/157378 .... feel free to review there. |
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>
x/net registers an endpoint in the http global. When this package is imported multiple times (because other packages I use import it) causes:
The text was updated successfully, but these errors were encountered: