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: Proposal: Remove import side effects #15359
Comments
There's a number of packages (including some in the standard library) Auth shouldn't be a problem if people are unaware. The default |
A quick greap for
Ah yes, I missed that; that's much much better, but it still feels like this would be much better off being something that I as an API consumer have to enable explicitly. Anything that modifies my servers behavior, I probably want to know about. We don't know how users machines are set up, or how they're running their infrastructure, for all we know access to those methods from the same machine is still a problem (maybe there's sensative info in those logs that not all devs get access too and only ops can see? It's a contrived and vague example, but I think the point still stands). |
expvar exports on /debug/vars by default too. If you're in a truly paranoid situation, you can always set up your own http.ServeMux. Remember that you won't get any of this doing anything unless you call something like http.ListenAndServe. The issue with having to enable it explicitly is that it's the kind of debugging endpoint you want to have present by default. It's annoying to have to load up your I think this is a massive convenience, and a pretty minor risk in practice. |
Ah, yup, didn't see that. I'm not a huge fan of it there either, but that's in the standard library so there's no changing it. At least it's very clearly documented in that case. I'll grant you that this does establish prescident, but just because something else was done this way before it doesn't make it a good idea or mean we should keep doing it this way in the future.
You could do that, but the point is that you have to know to do that and this violates the principal of least suprise. Most users won't expect a package to register http handlers just by being imported [citation needed]. A small bit of setup in main (or wherever you're doing your HTTP setup) does not feel inconvenient to me, it feels explicit and readable; it makes it immediately obvious what's happening (whereas otherwise there is no indication from reading a snippet of code that these handlers are being registered). |
This feels similarly annoying to when packages register flags in the global flagset as an import side effect. Explicit is better than implicit. |
If you want explicitness, don't use DefaultServeMux. Once you use it, you're already in implicit-land. (And the whole point of that variable is convenience. Better to go all the way, rather than "sort of a little bit more convenient, but not really".) |
Multiple suggestions here:
|
So it's been a few days and comments have trailed off. Who generally arbitrates / decides final behavior in disputes about packages in the |
This package was originally written by @dsymonds and he seems OK with the current behavior. Assuming it is documented correctly I'm not sure there is any change to make here. |
Oh, I didn't realize that; yah, fair enough. It still feels poor to me, and I don't fully understand ownership in the x/ tree, but if it's original-author's-call I suppose I don't really have anything else to say that I haven't already said to try and convince him. If this were ever merged into the standard library I'd certainly want to bring this up again though; it doesn't feel like appropriate behavior to me (and still needs to be better documented). Thanks for the discussion all. |
I know I was overruled on this, but I'd like to bring it up again. I'm not having an issue where I'm trying to build an application that imports a vendored version of trace, and also imports a (not-vendored because it's in the same repo using the "repo root is a library with a cmd subdirectory that imports that library" pattern) library that imports trace itself. This causes a panic because both versions of trace call ML discussion in case I'm missing an obvious solution is here: https://groups.google.com/forum/#!topic/golang-nuts/J9BZwJfLnXI |
See discussion after this comment: golang/go#15359 (comment)
I have not expressed an opinion in this thread and don't wish to do so, but I was summoned so I'll reply to this part:
How do you get to that conclusion? If an application vendors two version of net/trace and it blows up, the application can fix their vendoring config and move on. |
Oops, sorry about that; not sure who i was trying to CC there, maybe I just saw that you closed the issue.
In this case the application is not vendoring two versions of net/trace; the application is vendering 1 version, and the library that provides the applications core functionality is not vendoring (and not vendored by the application since it's part of the contract that the application will always be up to date with the library that provides its core functionality). I suppose one could argue that this is really a problem with vendoring, and not net/trace (if you don't vendor everything you'll run into problems), but it still seems foolish to put things that could panic like this in the init() method of a library; it violates the principle of least suprise (although thankfully in this case it does so loudly, so it's not likely to sneak into prod). |
Yeah, I suppose you should vendor everything or nothing. |
Cross posting this issue from the mailing list (https://groups.google.com/forum/#!topic/golang-dev/IfEhzW_mmV4) in accordance with the contributing doc (I'm still unsure what the usual procedure is here; should I really be posting things in two places?):
Right now if you import the
x/net/trace
package you wind up with theside effect that
net/http
'sDefaultServeMux
has handlers for/debug/requests
and/debug/events
registered on it. The only mentionof this side effect in the docs is a somewhat unclear statement right
at the start:
To me, this was very confusing. I did not take that statement to mean
that the package implicitly registers the handlers, but that this was
behavior you could achieve with the package (eg. maybe there's a
RegisterHandlers
function that registers those two handlers).I've put in a CL to make the documentation clearer
(https://go-review.googlesource.com/#/c/22208/1) but I'd also like to
propose that this "feature" be removed.
Implicitly setting up the handlers on package import violates Go's
principal of least suprise; in my case, that was just a panic and a
few minutes of me starting blankly at a screen wondering how the
handler I was registering could already be registered, but in someone
elses case it could involve a route to their debugging info being
exposed to the public (because they only thought to put authorization
or authentication in front of the /trace route which is where they
explicitly exposed the trace handler after importing this package).
In the elegant words of one of my coworkers when we discovered this
behavior: "that is a pretty gross side effect".
The text was updated successfully, but these errors were encountered: