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

Jump to bottom

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 The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@yaojingguo
Copy link
Contributor

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

What is the actual problem here?

@yaojingguo
Copy link
Contributor Author

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

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 The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. labels Aug 20, 2021
@mknyszek mknyszek added this to the Backlog milestone Aug 20, 2021
@mknyszek
Copy link
Contributor

CC @neild

@neild
Copy link
Contributor

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

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
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@neild @yaojingguo @mknyszek @seankhliao