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: missing a test scenario for using done channel for two calls #47832

Open
yaojingguo opened this issue Aug 20, 2021 · 6 comments
Open

net/rpc: missing a test scenario for using done channel for two calls #47832

yaojingguo opened this issue Aug 20, 2021 · 6 comments
Labels
NeedsFix Testing
Milestone

Comments

@yaojingguo
Copy link
Contributor

@yaojingguo yaojingguo commented Aug 20, 2021

After skimming Go net/rpc source code (last commit: e9e0d1e), I found that no test scenario exists for using done channel for two RPC calls. I suggest adding a test scenario for this case.

The following code shows the scenario details:

client.Go(serviceMethod, &args1, &reply1, done)
client.Go(serviceMethod, &args2, &reply2, done)

c1 := <- done
log.Println("get:", c1.Reply.(*GetReply).Value)

c2 := <- done
log.Println("get:", c2.Reply.(*GetReply).Value)
@seankhliao
Copy link
Member

@seankhliao seankhliao commented Aug 20, 2021

What is the actual problem here?

@yaojingguo
Copy link
Contributor Author

@yaojingguo yaojingguo commented Aug 20, 2021

https://pkg.go.dev/net/rpc@go1.17#Client.Go allows using done argument for two RPC calls. But such a scenario is not included in the net/rpc's test cases.

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Aug 20, 2021

If you have a suggested improvement for tests, you don't have to create an issue for it. You can just send the change. If you want to check with maintainers first, https://groups.google.com/g/golang-dev is a good place for this kind of communication (specifically, for working on the Go project itself). See https://golang.org/doc/contribute.

@mknyszek mknyszek added NeedsFix Testing labels Aug 20, 2021
@mknyszek mknyszek added this to the Backlog milestone Aug 20, 2021
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Aug 20, 2021

CC @neild

@neild
Copy link
Contributor

@neild neild commented Aug 20, 2021

The net/rpc package is frozen and not accepting new features. While a new test is not a feature, aside from tests associated with bugfixes adding more test cases doesn't seem like a good use of anyone's time.

@yaojingguo
Copy link
Contributor Author

@yaojingguo yaojingguo commented Aug 21, 2021

The Go function allows the reuse of the done channel argument. But neither net/rpc's doc nor its test cases show such a usage. So I think that it is worth of time to add such a test scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix Testing
Projects
None yet
Development

No branches or pull requests

4 participants