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

fix possible nil before casting #5017

Merged
merged 2 commits into from Dec 1, 2021
Merged

fix possible nil before casting #5017

merged 2 commits into from Dec 1, 2021

Conversation

charlesxsh
Copy link
Contributor

@charlesxsh charlesxsh commented Nov 30, 2021

This PR aims to fix a possible nil before casting to error.

--- FAIL: TestClusterManagerForwardsBalancerBuildOptions (1.00s)
panic: interface conversion: interface is nil, not error [recovered]
	panic: interface conversion: interface is nil, not error

goroutine 93 [running]:
testing.tRunner.func1.2(0xceec40, 0xc00020f230)
	/usr/local/go/src/testing/testing.go:1143 +0x335
testing.tRunner.func1(0xc0003be280)
	/usr/local/go/src/testing/testing.go:1146 +0x4c2
panic(0xceec40, 0xc00020f230)
	/usr/local/go/src/runtime/panic.go:965 +0x1b9
google.golang.org/grpc/xds/internal/balancer/clustermanager.TestClusterManagerForwardsBalancerBuildOptions(0xc0003be280)
	/fuzz/target/xds/internal/balancer/clustermanager/clustermanager_test.go:635 +0x68f
testing.tRunner(0xc0003be280, 0xe8bbb0)
	/usr/local/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1238 +0x2b5

RELEASE NOTES: none

Copy link
Contributor

@dfawley dfawley left a comment

@easwars it looks like you originally wrote this - what do you think?

@@ -560,6 +560,9 @@ func TestClusterManagerForwardsBalancerBuildOptions(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
if v, err := ccsCh.Receive(ctx); err != nil {
if v == nil {
Copy link
Contributor

@dfawley dfawley Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this actually a guarantee here?

I think this is written incorrectly, because it's not actually validating v at all in the normal case (where there is no timeout). Probably what was meant was:

v, err := ccsCh.Receive(ctx)
if err != nil {
	t.Fatalf("timed out waiting for UpdateClientConnState result: %v", err)
}
if v != nil {
	t.Fatal(v)
}

Copy link
Contributor Author

@charlesxsh charlesxsh Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes. We need to check err first.

@dfawley dfawley requested a review from easwars Nov 30, 2021
@dfawley dfawley added this to the 1.43 release milestone Nov 30, 2021
@easwars
Copy link
Contributor

@easwars easwars commented Dec 1, 2021

@easwars it looks like you originally wrote this - what do you think?

Yup, good catch and fix I think.

easwars
easwars approved these changes Dec 1, 2021
dfawley
dfawley approved these changes Dec 1, 2021
@easwars easwars merged commit 46935b9 into grpc:master Dec 1, 2021
10 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants