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

net/rpc: HandleHTTP should be idempotent. #13239

Open
keep94 opened this issue Nov 13, 2015 · 1 comment
Open

net/rpc: HandleHTTP should be idempotent. #13239

keep94 opened this issue Nov 13, 2015 · 1 comment
Milestone

Comments

@keep94
Copy link

@keep94 keep94 commented Nov 13, 2015

Our team wrote a library to expose health metrics of a process. We wanted an anonymous import of our library to be enough to expose metrics. Our init method would handle setting up the RPC handlers including calling rpc.HandleHTTP().

Unfortunately, we discovered that this approach was hostile toward any other code that happens to call rpc.HandleHTTP(). rpc.HandleHTTP() panics if called twice. Alas, we had to move the rpc.HandleHTTP() call out of our library and leave it up to clients to remember to explicitly call it in their main. If the client forgets to do this, our library won't expose their health metrics. Worse than that, the client won't see any error letting them know that our library is not working.

It would be nice if rpc.HandleHTTP() could be idempotent. That is, calling it multiple times should be the same as calling it once. Then our API could call it on behalf of clients without fear of breaking other code the client uses.

@ianlancetaylor ianlancetaylor changed the title rpc.HandleHTTP should be idempotent. net/rpc: HandleHTTP should be idempotent. Nov 14, 2015
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Nov 14, 2015
@meirf

This comment has been minimized.

Copy link
Contributor

@meirf meirf commented May 14, 2018

rpc.HandleHTTP associates rpc.DefaultServer with http.DefaultServeMux on regular/debug rpc paths. Calling rpc.HandleHTTP more than once fails because http.ServeMux.Handle is not idempotent on the path parameter. I agree that rpc.HandleHTTP should be idempotent. Just like like the pprof library sets paths on the default http mux so too should other libraries be able to register rpc receivers on the default rpc server AND associate the default rpc server with the default http mux. In both cases, client code should just be able to tell the default http mux what to listen on. Of course any new code in net/rpc must be fixing something (no new feature) due to the freeze on net/rpc. IMO this should qualify as a fix.

My notes below is a way for the OP (and others) to alleviate their situation assuming changing current behavior is blocked by the freeze.

We wanted an anonymous import of our library to be enough to expose metrics.

@keep94, are you saying that the importer of your library has to add literally no code to expose metrics?

import _ "github.com/foo/bar"
func main(){}

A. Importer doesn't need to add any code
That would mean that your library's init must have logic which tells the http.DefaultServeMux which address to listen on - like http.ListenAndServe(":1234", nil). I don't think it's recommended to dictate to your user which port to use. Note the pattern that pprof uses: "If your application is not already running an http server, you need to start one." The pprof package requires that you set what the http.DefaultServeMux listens on.

B. Importer must add listener
If the importer must add listener logic, can you tell them to add one more line - rpc.HandleHTTP?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.