-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
attributes: print typed nil values instead of panic #6574
Conversation
3be80aa
to
e1ad082
Compare
attributes/attributes_test.go
Outdated
fmt.Println("a6:", a6.String()) | ||
fmt.Println("a7:", a7.String()) | ||
// Output: | ||
// a1: {"<%!p(attributes_test.key={})>": "<nil>" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been noticing this %!p
while debugging some tests and its not very pleasing to the eye (and I understand this is not introduced by your changes in this PR). Is there a way to make this better? Like is it possible to print "<attributes_test.key={}>"
instead of "<%!p(attributes_test.key={})>"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made it by change the style from %!verb(type=value)
to <type=value>
, for invalid argument for '%p'.
Adapted from the code in fmt/print.go.
func ExampleAttributes_String() {
type key struct{}
var typedNil *stringerVal
a1 := attributes.New(key{}, typedNil) // typed nil implements [fmt.Stringer]
a2 := attributes.New(key{}, (*stringerVal)(nil)) // typed nil implements [fmt.Stringer]
a3 := attributes.New(key{}, (*stringVal)(nil)) // typed nil not implements [fmt.Stringer]
a4 := attributes.New(key{}, nil) // untyped nil
a5 := attributes.New(key{}, 1)
a6 := attributes.New(key{}, stringerVal{s: "two"})
a7 := attributes.New(key{}, stringVal{s: "two"})
a8 := attributes.New(1, true)
fmt.Println("a1:", a1.String())
fmt.Println("a2:", a2.String())
fmt.Println("a3:", a3.String())
fmt.Println("a4:", a4.String())
fmt.Println("a5:", a5.String())
fmt.Println("a6:", a6.String())
fmt.Println("a7:", a7.String())
fmt.Println("a8:", a8.String())
// Output:
// a1: {"<attributes_test.key={}>": "<nil>" }
// a2: {"<attributes_test.key={}>": "<nil>" }
// a3: {"<attributes_test.key={}>": "<0x0>" }
// a4: {"<attributes_test.key={}>": "<nil>" }
// a5: {"<attributes_test.key={}>": "<int=1>" }
// a6: {"<attributes_test.key={}>": "two" }
// a7: {"<attributes_test.key={}>": "<attributes_test.stringVal={two}>" }
// a8: {"<int=1>": "<bool=true>" }
}
Where are you seeing the panic? Is one of our types that implements |
…e=value>`, not `%!verb(type=value)` grpc#6574 (comment)
when I write a resolver builder, some options were passed by attributes without check value is nil or not.
func (cb *ConsulBuilder) Build(target resolver.Target, cc resolver.ClientConn, opts resolver.BuildOptions) (resolver.Resolver, error) {
namespace, service, err := parseTarget(target)
if err != nil {
return nil, err
}
pr := &consulResolver{
namespace: namespace,
service: service,
cc: cc,
}
serviceConfig := cc.ParseServiceConfig(fmt.Sprintf(`{"LoadBalancingPolicy": "%s"}`, Name))
attrs := attributes.New(attributeKeyTargetNamespace, namespace).
WithValue(attributeKeyTargetService, service).
WithValue(attributeKeyMetadata, cb.metadata) // panic if metadata is nil
cc.UpdateState(resolver.State{
Addresses: []resolver.Address{},
ServiceConfig: serviceConfig,
Attributes: attrs,
})
return pr, nil
} |
…no panic if typed-nil attributes within resolver.State.Addresses (grpc#6574)
=== RUN Test/ResolverAddressesWithTypedNilAttribute
tlogger.go:116: INFO clientconn.go:317 [core] [Channel #130] Channel created (t=+16.709µs)
tlogger.go:116: INFO clientconn.go:1814 [core] [Channel #130] original dial target is: "testresolveraddresseswithtypednilattribute:///" (t=+44.625µs)
tlogger.go:116: INFO clientconn.go:1821 [core] [Channel #130] parsed dial target is: {URL:{Scheme:testresolveraddresseswithtypednilattribute Opaque: User: Host: Path:/ RawPath: OmitHost:false ForceQuery:false RawQuery: Fragment: RawFragment:}} (t=+64.167µs)
tlogger.go:116: INFO clientconn.go:1976 [core] [Channel #130] Channel authority set to "" (t=+78.334µs)
panic: value method google.golang.org/grpc.stringerVal.String called using nil *stringerVal pointer [recovered]
panic: value method google.golang.org/grpc.stringerVal.String called using nil *stringerVal pointer
goroutine 2817 [running]:
encoding/json.(*encodeState).marshal.func1()
/usr/local/go/src/encoding/json/encode.go:291 +0x80
panic({0x10154c080?, 0x1400011b4d0?})
/usr/local/go/src/runtime/panic.go:914 +0x218
google.golang.org/grpc.(*stringerVal).String(0x101544ec0?)
<autogenerated>:1 +0x40
google.golang.org/grpc/attributes.str({0x10153d860, 0x0})
/data/workspace/src/github.com/searKing/grpc-go/attributes/attributes.go:147 +0x140
google.golang.org/grpc/attributes.(*Attributes).String(0x140002a81a8?)
/data/workspace/src/github.com/searKing/grpc-go/attributes/attributes.go:118 +0x1dc
google.golang.org/grpc/attributes.(*Attributes).MarshalJSON(0x101582de0?)
/data/workspace/src/github.com/searKing/grpc-go/attributes/attributes.go:206 +0x1c
encoding/json.marshalerEncoder(0x14000162680, {0x101582de0?, 0x140002ac4e0?, 0x12877e9d0?}, {0x78?, 0x16?})
/usr/local/go/src/encoding/json/encode.go:442 +0xb8
encoding/json.structEncoder.encode({{{0x1400021a900, 0x5, 0x8}, 0x14000219140, 0x14000219170}}, 0x14000162680, {0x1015bf840?, 0x140002ac4c0?, 0x1016cc266?}, {0x55?, ...})
/usr/local/go/src/encoding/json/encode.go:706 +0x190
encoding/json.arrayEncoder.encode({0x14000601e48?}, 0x14000162680, {0x101511840?, 0x140002ac5c0?, 0x0?}, {0x0?, 0x0?})
/usr/local/go/src/encoding/json/encode.go:849 +0xc8
encoding/json.sliceEncoder.encode({0x14000601958?}, 0x14000162680, {0x101511840?, 0x140002ac5c0?, 0x14000601968?}, {0xc?, 0x0?})
/usr/local/go/src/encoding/json/encode.go:822 +0x260
encoding/json.structEncoder.encode({{{0x1400017afc0, 0x4, 0x4}, 0x14000219440, 0x14000219470}}, 0x14000162680, {0x1015a7ec0?, 0x140002ac5c0?, 0x30?}, {0x40?, ...})
/usr/local/go/src/encoding/json/encode.go:706 +0x190
encoding/json.(*encodeState).reflectValue(0x14000601b18?, {0x1015a7ec0?, 0x140002ac5c0?, 0x0?}, {0x0?, 0x0?})
/usr/local/go/src/encoding/json/encode.go:323 +0x70
encoding/json.(*encodeState).marshal(0x140002b73b8?, {0x1015a7ec0?, 0x140002ac5c0?}, {0x0?, 0x0?})
/usr/local/go/src/encoding/json/encode.go:295 +0xb8
encoding/json.Marshal({0x1015a7ec0, 0x140002ac5c0})
/usr/local/go/src/encoding/json/encode.go:162 +0x94
encoding/json.MarshalIndent({0x1015a7ec0?, 0x140002ac5c0?}, {0x0, 0x0}, {0x1013f6ac6, 0x2})
/usr/local/go/src/encoding/json/encode.go:175 +0x34
google.golang.org/grpc/internal/pretty.ToJSON({0x1015a7ec0?, 0x140002ac5c0})
/data/workspace/src/github.com/searKing/grpc-go/internal/pretty/pretty.go:64 +0x140
google.golang.org/grpc.(*ccResolverWrapper).addChannelzTraceEvent(0x1400036c180, {{0x140002ac4c0, 0x1, 0x1}, {0x1400015cd00, 0x1, 0x1}, 0x0, 0x0})
/data/workspace/src/github.com/searKing/grpc-go/resolver_conn_wrapper.go:246 +0x28c
google.golang.org/grpc.(*ccResolverWrapper).UpdateState.func1({0x1400028cae0, 0x140002b7728})
/data/workspace/src/github.com/searKing/grpc-go/resolver_conn_wrapper.go:164 +0x54
google.golang.org/grpc/internal/grpcsync.(*CallbackSerializer).run(0x1400015ccc0, {0x1015f9e08, 0x1400010ea50})
/data/workspace/src/github.com/searKing/grpc-go/internal/grpcsync/callback_serializer.go:92 +0x114
created by google.golang.org/grpc/internal/grpcsync.NewCallbackSerializer in goroutine 2814
/data/workspace/src/github.com/searKing/grpc-go/internal/grpcsync/callback_serializer.go:55 +0x134 |
So, IIUC, you are implementing your own custom resolver and are passing some values through the attributes and you are setting one of those values to |
Yes. Upgraded
Now avoid type-nil within
attr := attributes.New("namespace", ns) // var ns string
if geo !=nil{
attr = attr.WithValue("geolocation", geo) // geo := &struct{...}
}
attr = attr.WithValue("label", label) // var label string
if metadata !=nil{
attr = attr.WithValue("metadata", metadata) // var metadata map[string]string or []string
}
attr := attributes.New("namespace", ns).
WithValue("geolocation", Any(geo)).
WithValue("label", Any(label)). // UnTypeNil is not needed for `string` here. removed recommended.
WithValue("metadata", Any(metadata))
// Any returns its argument v or nil if and only if v is a typed nil or an untyped nil.
// For example, if v was created by set with `var p *int` or calling [Any]((*int)(nil)),
// [Any] returns nil.
func Any[T any](v *T) any {
if v == nil {
return nil
}
return v
}
attr := attributes.New("namespace", ns).
WithValue("geolocation", UnTypeNil(geo)).
WithValue("label", UnTypeNil(label)). // UnTypeNil is not needed for `string` here. removed recommended.
WithValue("metadata", UnTypeNil(metadata))
// UnTypeNil returns its argument v or nil if and only if v is a nil interface value or an untyped nil.
func UnTypeNil(v any) any {
// convert typed nil into untyped nil
if IsNil(v) {
return nil
}
return v
}
// IsNil reports whether its argument v is a nil interface value or an untyped nil.
// Note that IsNil is not always equivalent to a regular comparison with nil in Go.
// It is equivalent to:
//
// var typedNil any = (v's underlying type)(nil)
// return v == nil || v == typedNil
//
// For example, if v was created by set with `var p *int` or calling IsNil((*int)(nil)),
// i==nil will be false but [IsNil] will be true.
func IsNil(v any) bool {
if v == nil {
return true
}
return IsNilValue(reflect.ValueOf(v))
}
// IsNilValue reports whether v is untyped nil or typed nil for its type.
func IsNilValue(v reflect.Value) bool {
var zeroV reflect.Value
if v == zeroV {
return true
}
if !v.IsValid() {
// This should never happen, but will act as a safeguard for later,
// as a default value doesn't make sense here.
panic(&reflect.ValueError{Method: "reflect.Value.IsNilValue", Kind: v.Kind()})
}
switch v.Kind() {
case reflect.Chan, reflect.Func, reflect.Map, reflect.Pointer, reflect.UnsafePointer,
reflect.Interface, reflect.Slice:
return v.IsNil()
}
return false
} |
@searKing : We had a lengthy discussion last week about these changes. We would like to avoid making the While I do think that it is a user error to populate attributes with typed-nils, it is not ideal for the library to Would the following work?
Let me know your thoughts on this? Thanks. |
|
Once you move the implementation to directly use the functionality provided by the Thanks for your contribution and the discussions so far. |
Commit Done.
|
Co-authored-by: Easwar Swaminathan <easwars@google.com>
@dfawley for second set of eyes. |
Thanks for your contribution, @searKing. |
This fixes a panic caused by typed nil, introduced in #6224.
String()
should catch panics when formatting by using thefmt
package.RELEASE NOTES: none