panic: runtime error: comparing uncomparable type map[string]interface {}

[cruvie]

ref etcd-io/etcd#19700

[arjan-bal]

We have been using == to compare Addresses for a long time.

grpc-go/resolver/resolver.go

Lines 124 to 135 in ce35fd4

	// Equal returns whether a and o are identical. Metadata is compared directly,
	// not with any recursive introspection.
	//
	// This method compares all fields of the address. When used to tell apart
	// addresses during subchannel creation or connection establishment, it might be
	// more appropriate for the caller to implement custom equality logic.
	func (a Address) Equal(o Address) bool {
	return a.Addr == o.Addr && a.ServerName == o.ServerName &&
	a.Attributes.Equal(o.Attributes) &&
	a.BalancerAttributes.Equal(o.BalancerAttributes) &&
	a.Metadata == o.Metadata
	}

The panic is happening because the Metadata field is of type map[string]interface {} instead of a pointer *map[string]interface {}.

[cruvie]

@arjan-bal Hi, the problem is

grpc-go/clientconn.go

Line 944 in 51d6a43

a.Metadata == b.Metadata

and v1.70.0 works fine anyway

[arjan-bal]

That being said, the pickfirstleaf balancer is unable to handle Addresses with the Metadata field set and gets stuck in CONNECTING. The problem is that it uses the Metadata field for comparing addresses, but the AddressMap that is uses for storing SubConns doesn't consider the metadata field.

grpc-go/resolver/map.go

Lines 38 to 42 in ce35fd4

	// AddressMapV2 is a map of addresses to arbitrary values taking into account
	// Attributes. BalancerAttributes are ignored, as are Metadata and Type.
	// Multiple accesses may not be performed concurrently. Must be created via
	// NewAddressMap; do not construct directly.
	type AddressMapV2[T any] struct {

I've raised a PR to fix this.

[arjan-bal]

@arjan-bal Hi, the problem is

grpc-go/clientconn.go

Line 944 in 51d6a43

a.Metadata == b.Metadata
and v1.70.0 works fine anyway

If we only fix this check that is panicking, comparing Addresses using the Addresses.Equal method would still panic. Its documentation mentions that metadata is compared directly making me suspect that having a comparable type is an undocumented requirement. I will discuss this with other maintainers.

[arjan-bal]

@arjan-bal Hi, the problem is

grpc-go/clientconn.go

Line 944 in 51d6a43

a.Metadata == b.Metadata
and v1.70.0 works fine anyway

This code was introduced in July last year in #7378. Before the change Addreess.Equal was being used, which could also panic.

The panic stack trace in etcd-io/etcd#19700 issue is from the new pickfirstleaf balancer that has a similar function because we moved the pick first logic from ClientConn into the LB policy.

grpc-go/balancer/pickfirst/pickfirstleaf/pickfirstleaf.go

Lines 924 to 928 in ce35fd4

	func equalAddressIgnoringBalAttributes(a, b *resolver.Address) bool {
	return a.Addr == b.Addr && a.ServerName == b.ServerName &&
	a.Attributes.Equal(b.Attributes) &&
	a.Metadata == b.Metadata
	}

I'm not sure why 1.70 doesn't panic.

[arjan-bal]

The new pickfirst balancer no longer reads the Metadata field, so it will not panic after #8227. If only the Metadata field within an address is updated, pickfirst will consider the address as unchanged and continue using the same subchannel. This fix will be part of the 1.72 release happening soon. I have not changed the code in clientconn.go as there was no recent behaviour changes there, we always compared the Metadata directly.

v1.72 will make the new pickfirst the default balancer, so the code path in clientconn.go which triggers a similar panic should not be used anymore. Marking this issue as closed. If there are more questions, I'll re-open the issue.