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: server.Register() should not internally log by default #19957

Open
prashanthpai opened this issue Apr 13, 2017 · 10 comments
Open

net/rpc: server.Register() should not internally log by default #19957

prashanthpai opened this issue Apr 13, 2017 · 10 comments

Comments

@prashanthpai
Copy link

@prashanthpai prashanthpai commented Apr 13, 2017

What version of Go are you using (go version)?

$ go version
go version go1.8 linux/amd64

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ppai/work"
GORACE=""
GOROOT="/home/ppai/go"
GOTOOLDIR="/home/ppai/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build142468001=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

https://play.golang.org/p/Y5tF3ruvTw

From the net/rpc source:

// suitableMethods returns suitable Rpc methods of typ, it will report
// error using log if reportErr is true.
func suitableMethods(typ reflect.Type, reportErr bool) map[string]*methodType {
...
...
		if mtype.NumIn() != 3 {
   				if reportErr {
   					log.Println("method", mname, "has wrong number of ins:", mtype.NumIn())
   				}
   				continue
   		}

The unexported caller sets reportErr to true:

func (server *Server) register(rcvr interface{}, name string, useName bool) error {
...
...
	// Install the methods
   	s.method = suitableMethods(s.typ, true)

What did you expect to see?

It is not uncommon for a type to contain methods that do not conform to the method signatures required by net/rpc. The net/rpc should not log these benign log messages by default when it finds such methods. There should be a way to turn this off or ignore it. It should be logged only if a flag or variable is set.

What did you see instead?

2009/11/10 23:00:00 method Name has wrong number of ins: 1
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Apr 14, 2017

It is not uncommon for a type to contain methods that do not conform to the method signatures required by net/rpc.

What's an example of case where that's acceptable to ignore the warning?

@prashanthpai

This comment has been minimized.

Copy link
Author

@prashanthpai prashanthpai commented Apr 14, 2017

What's an example of case where that's acceptable to ignore the warning?

The type which has one or more RPC method signature conforming methods may also be implementing some other interface and hence can contain other methods. Here's a very simplified example: https://play.golang.org/p/HINYYRn1mU

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Apr 14, 2017

That comment has bad grammar and needs fixing in any case.

The package is frozen featurewise, but I agree that log message may be spammier than needed. I have no memory of doing the review of https://codereview.appspot.com/6819081, which added the log message along with many others. @minux thought it was important to add them, so I'll pass this one on to him.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jun 5, 2017

OK, sounds like Rob is OK with dropping the log statement. I will note that this makes the failure to invoke the method somewhat more mysterious, at least unexplained.

@rsc rsc added NeedsFix and removed NeedsDecision labels Jun 5, 2017
@rsc rsc unassigned robpike Jun 5, 2017
@rsc rsc added this to the Go1.10 milestone Jun 5, 2017
@robpike robpike marked this as a duplicate of #20862 Jul 21, 2017
@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@namusyaka

This comment has been minimized.

Copy link
Member

@namusyaka namusyaka commented Jan 10, 2018

I don't think the log is necessarily needed, but I can agree to improve log message.
How about this?

diff --git a/src/net/rpc/server.go b/src/net/rpc/server.go
index a021292603..89a07fc92d 100644
--- a/src/net/rpc/server.go
+++ b/src/net/rpc/server.go
@@ -296,7 +296,7 @@ func suitableMethods(typ reflect.Type, reportErr bool) map[string]*methodType {
                // Method needs three ins: receiver, *args, *reply.
                if mtype.NumIn() != 3 {
                        if reportErr {
-                               log.Println("method", mname, "has wrong number of ins:", mtype.NumIn())
+                               log.Printf("rpc.Register: method %q has wrong number of parameters, got: %d\n", mname, mtype.NumIn())
                        }
                        continue
                }

If we apply changes like the above patch, there are other things we need to fix.

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Jan 10, 2018

That seems fine to me. Please send a CL.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 10, 2018

Change https://golang.org/cl/87335 mentions this issue: net/rpc: improve error report messages

gopherbot pushed a commit that referenced this issue Jan 15, 2018
Updates #19957

Change-Id: I8e2e3837db9e5e69b7102f9bd5831fe78ac60cfc
Reviewed-on: https://go-review.googlesource.com/87335
Run-TryBot: Rob Pike <r@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
@namusyaka

This comment has been minimized.

Copy link
Member

@namusyaka namusyaka commented Mar 11, 2018

@robpike Is it OK to close this issue?
If we think that this problem was inherently solved by having the error message become more detailed, I would like to close it.

@robpike robpike closed this Mar 12, 2018
@prashanthpai

This comment has been minimized.

Copy link
Author

@prashanthpai prashanthpai commented Sep 14, 2018

Well, https://golang.org/cl/87335 just changes the log message but doesn't remove it. It did not fix the issue reported in any way.

It's really annoying and misleading to see these many log messages logged by standard library. The net/rpc package is "frozen" only featurewise.

I can send a patch to remove the log if you guys agree.

@golang golang locked and limited conversation to collaborators Sep 14, 2019
@eliben

This comment has been minimized.

Copy link
Member

@eliben eliben commented Jan 10, 2020

I found this issue because I just ran into this myself. I have a type which I want to expose as an RPC endpoint, but it has additional exported methods. The mandatory emission of logs is bad ergonomics here, and goes against the documentation, which says:

Only methods that satisfy these criteria will be made available for remote access; other methods will be ignored: [...] list of criteria

Emitting a log is not entirely ignoring other methods.

From this issue it sounds like @robpike was for reverting the mandatory logging, but then the issue got auto-closed after only the error message part changed.

I'm reopening the issue, and will be happy to implement the change. Are there any objections?

@minux

@eliben eliben reopened this Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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