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

client: handle empty address lists correctly in addrConn.updateAddrs #6354

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Jun 7, 2023

This fixes a bug caused by #6302, and will need to be cherry-picked into v1.56.x before the release.

grpclb can run into this situation, which the added test also stimulates. It may not be "proper" to have a SubConn with no addresses, but the result is currently a nil pointer panic, which we should not allow, and needs to be fixed ASAP.

RELEASE NOTES: none

@dfawley dfawley added this to the 1.56 Release milestone Jun 7, 2023
@dfawley dfawley requested a review from zasweq June 7, 2023 00:04
Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

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

LGTM.

}
defer ss.Stop()
if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); err != nil {
t.Fatalf("UnaryCall RPC failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, super nit: UnaryCall is distinct from EmptyCall wrt StubServer. Technically it is a UnaryCall, but would prefer calling this EmptyCall.

t.Log("Re-adding addresses to resolver and SubConn")
ss.R.UpdateState(resolver.State{Addresses: []resolver.Address{{Addr: ss.Address}}})
if _, err := ss.Client.EmptyCall(ctx, &testpb.Empty{}); err != nil {
t.Fatalf("UnaryCall RPC failed: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@dfawley dfawley merged commit 6578ef7 into grpc:master Jun 7, 2023
1 check passed
@dfawley dfawley deleted the addrConnTest branch June 7, 2023 15:37
zasweq added a commit that referenced this pull request Jun 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2023
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.

2 participants