Skip to content

net/rpc: Client.Go allocates a buffered channel with large capacity (10) instead of small (1) when done is nil #41901

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

Closed
furthergo opened this issue Oct 10, 2020 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@furthergo
Copy link

furthergo commented Oct 10, 2020

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

 go1.15 darwin/amd64

Does this issue reproduce with the latest release?

Yes (go1.15)

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

go env Output
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPATH="~/go"
GOROOT="/usr/local/Cellar/go/1.15/libexec"
GOSUMDB="sum.golang.google.cn"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

// net/rpc/client.go

// Go invokes the function asynchronously. It returns the Call structure representing
// the invocation. The done channel will signal when the call is complete by returning
// the same Call object. If done is nil, Go will allocate a new channel.
// If non-nil, done must be buffered or Go will deliberately crash.
func (client *Client) Go(serviceMethod string, args interface{}, reply interface{}, done chan *Call) *Call {
	call := new(Call)
	call.ServiceMethod = serviceMethod
	call.Args = args
	call.Reply = reply
	if done == nil {
		done = make(chan *Call, 10) // buffered.
	} else {
		// If caller passes done != nil, it must arrange that
		// done has enough buffer for the number of simultaneous
		// RPCs that will be using that channel. If the channel
		// is totally unbuffered, it's best not to run at all.
		if cap(done) == 0 {
			log.Panic("rpc: done channel is unbuffered")
		}
	}
	call.Done = done
	client.send(call)
	return call
}

In the function func (client *Client) Go(serviceMethod string, args interface{}, reply interface{}, done chan *Call) *Call, when caller passes done == nil, Go will allocate a new channel with buffer 10, which used to notify current Call is finished.

What did you expect to see?

It's enough to create a channel with capacity 1

if done == nil {
    done = make(chan *Call, 1)  // buffered
}

What did you see instead?

if done == nil {
    done = make(chan *Call, 10) // buffered.
}
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 13, 2020
@dmitshur dmitshur added this to the Backlog milestone Oct 13, 2020
@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 13, 2020
@dmitshur
Copy link
Contributor

If this is a question, please see https://golang.org/wiki/Questions.

If this is a bug, please fill in the questions in the issue template.

@furthergo
Copy link
Author

If this is a question, please see https://golang.org/wiki/Questions.

If this is a bug, please fill in the questions in the issue template.

Updated

@dmitshur dmitshur changed the title net/rpc/client: too big capacity of Call's buffered Done channel net/rpc: Client.Go allocates a buffered channel with large capacity (10) instead of small (1) when done is nil Oct 16, 2020
@dmitshur dmitshur removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 16, 2020
@dmitshur
Copy link
Contributor

Thanks for filling in the template.

Please be aware that package net/rpc is frozen and not accepting new features.

I don't believe there's an issue here. Capacity 1 is enough for when the frequency of requests is very small. But it will not be enough if there are many requests happening in a short amount of time. The user can always supply a custom non-nil done channel with their desired capacity, but 10 seems like a reasonable default otherwise.

Also note the comment in the snippet you included:

// If caller passes done != nil, it must arrange that
// done has enough buffer for the number of simultaneous
// RPCs that will be using that channel. If the channel
// is totally unbuffered, it's best not to run at all.

So I'll close as this is working as intended and there's no issue here.

CC @robpike per owners.

@golang golang locked and limited conversation to collaborators Oct 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants