golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.34k stars 17.71k forks source link

net/rpc: HandleHTTP should be idempotent. #13239

Open keep94 opened 9 years ago

keep94 commented 9 years ago

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.

meirf commented 6 years ago

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?