attributes: print typed nil values instead of panic #6574

searKing · 2023-08-23T05:19:50Z

This fixes a panic caused by typed nil, introduced in #6224.
String() should catch panics when formatting by using the fmt package.

RELEASE NOTES: none

linux-foundation-easycla · 2023-08-23T05:19:53Z

The committers listed above are authorized under a signed CLA.

✅ login: searKing / name: Haixin Chen (1114f39, dbe2270, 5936668, e8fe6ad, 55f2b24, 32abddd, 2e3e6ee, 9116500)
✅ login: easwars / name: Easwar Swaminathan (9116500)

easwars · 2023-08-29T16:52:17Z

attributes/attributes_test.go

+	fmt.Println("a6:", a6.String())
+	fmt.Println("a7:", a7.String())
+	// Output:
+	// a1: {"<%!p(attributes_test.key={})>": "<nil>" }


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={})>"?

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>" } }

easwars · 2023-08-29T17:18:52Z

Where are you seeing the panic? Is one of our types that implements Stringer not handling a nil receiver?

…e=value>`, not `%!verb(type=value)` grpc#6574 (comment)

searKing · 2023-08-30T16:18:56Z

Where are you seeing the panic? Is one of our types that implements Stringer not handling a nil receiver?

when I write a resolver builder, some options were passed by attributes without check value is nil or not.
I meet the panic when attributes value is nil.
panic XXX.String called using nil *XXX pointer at

resolver.go#L162: sb.WriteString(fmt.Sprintf("Attributes: %v, ", a.Attributes.String()))
clientconn.go#L1476: channelz.Warningf(logger, ac.channelzID, "grpc: addrConn.createTransport failed to connect to %s. Err: %v", addr, err)

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)

searKing · 2023-08-30T17:56:52Z

Where are you seeing the panic? Is one of our types that implements Stringer not handling a nil receiver?

when I write a resolver builder, some options were passed by attributes without check value is nil or not. I meet the panic when attributes value is nil. panic XXX.String called using nil *XXX pointer at

resolver.go#L162: sb.WriteString(fmt.Sprintf("Attributes: %v, ", a.Attributes.String()))

clientconn.go#L1476: channelz.Warningf(logger, ac.channelzID, "grpc: addrConn.createTransport failed to connect to %s. Err: %v", addr, err)

I have added TestResolverAddressesWithTypedNilAttribute in resolver_test.go#L146 to reproduce the panic above mentioned.
below is the full call stack:

=== 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

easwars · 2023-08-31T17:30:05Z

when I write a resolver builder, some options were passed by attributes without check value is nil or not.

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 nil and are seeing the panic. Is that correct?

searKing · 2023-09-01T03:04:58Z

when I write a resolver builder, some options were passed by attributes without check value is nil or not.

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 nil and are seeing the panic. Is that correct?

Yes. Upgraded grpc-go from v1.53.0 to v1.57.0, our gRPC client got a panic.

Some attributes will be resolved with Address, like a struct pointer with geolocation records(Location, TTL, healthy), a map with metadata.

Now avoid type-nil within grpc-go@1.57 in three ways:

check all values whether it's nil or not (check manually is costly and error prone):

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
}

guard by a Generics wrapper func (guard is convenient, but implicit behaving without wrapper function like fmt in golang maybe better):

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 as defined below:

// 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
}

guard by a reflect wrapper func (guard is convenient, but implicit behaving without wrapper function like fmt in golang maybe better):

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 as defined below:

// 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
}

easwars · 2023-09-05T21:33:59Z

@searKing : We had a lengthy discussion last week about these changes. We would like to avoid making the attributes package handle a lot of edge cases like what a fmt package does, only for the purpose of having these attributes values show up nicely in logs. So, instead of having custom code to handle the pretty printing of attributes, we think it would be better if we can make use of the fmt package as much as possible.

While I do think that it is a user error to populate attributes with typed-nils, it is not ideal for the library to panic under such situations. So, I would like to request that we keep the changes in this PR down to only the handling of typed nils.

Would the following work?

Get rid of the str() function
In Attributes.String(), instead of calling sb.WriteString(fmt.Sprintf("%q: %q ", str(k), str(v))), we could call sb.WriteString(fmt.Sprintf("%q, %q", k, v)) or we could call sb.WriteString(fmt.Sprintf("\"%v\": \"%v\" ", k, v))
The last of the above options is to work around the current faulty implementation of Attributes.MarshalJSON() which currently uses the output of Attributes.String() and assumes that the string output is actually valid JSON.
- We ideally should fix the implementation of Attributes.MarshalJSON() to delegate to the MarshalJSON() method of the key and value types (if they implement it), or emit string JSON of the kind couldn't marshal JSON; here's a string version

Let me know your thoughts on this? Thanks.

searKing · 2023-09-06T09:08:42Z

@searKing : We had a lengthy discussion last week about these changes. We would like to avoid making the attributes package handle a lot of edge cases like what a fmt package does, only for the purpose of having these attributes values show up nicely in logs. So, instead of having custom code to handle the pretty printing of attributes, we think it would be better if we can make use of the fmt package as much as possible.

Attributes.String() for pretty only, like fmt.Println("attr", attr).
may be we can usesb.WriteString(fmt.Sprintf("%+v: %+v", k, v)), %+v will call Error() if error implemented, call String() if fmt.Stringer implemented, otherwise, print with field names of struct.
Attributes.MarshalJSON() just removed, for serialization and deserialization only, since json can handle json.Marshaler and json.UnMarshaler well.

sb.WriteString(fmt.Sprintf("%+v: %+v", k, v))

a1: {{}: <nil> }
a2: {{}: <nil> }
a3: {{}: <nil> }
a4: {{}: <nil> }
a5: {{}: 1 }
a6: {{}: two }
a7: {{}: {s:two} } // print with field names of struct
a8: {1: true }

sb.WriteString(fmt.Sprintf("%q: %q", k, v))

a1: {{}: <nil> }
a2: {{}: <nil> }
a3: {{}: <nil> }
a4: {{}: <nil> }
a5: {{}: 1 }
a6: {{}: two }   
a7: {{}: {two} } // without field names of struct
a8: {1: true }

sb.WriteString(fmt.Sprintf("%q: %q", k, v))

a1: {{}: <nil> }
a2: {{}: <nil> }
a3: {{}: %!q(*attributes_test.stringVal=<nil>) } // bad verb case for %q 
a4: {{}: %!q(<nil>) }
a5: {{}: '\x01' }
a6: {{}: "two" }
a7: {{}: {"two"} }
a8: {'\x01': %!q(bool=true) }

sb.WriteString(fmt.Sprintf("%#v: %#v ", k, v))

a1: {attributes_test.key{}: (*attributes_test.stringerVal)(nil) }
a2: {attributes_test.key{}: (*attributes_test.stringerVal)(nil) }
a3: {attributes_test.key{}: (*attributes_test.stringVal)(nil) }
a4: {attributes_test.key{}: <nil> }
a5: {attributes_test.key{}: 1 }
a6: {attributes_test.key{}: attributes_test.stringerVal{s:"two"} } // print with struct name and field names of struct, but ignore [fmt.Stringer], repect [fmt.GoStringer]
a7: {attributes_test.key{}: attributes_test.stringVal{s:"two"} }
a8: {1: true }

sb.WriteString(fmt.Sprintf("%q: %q ", str(k), str(v)))

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>" }

attributes/attributes.go

easwars · 2023-09-15T18:02:34Z

Once you move the implementation to directly use the functionality provided by the fmt package, please let us know and we will take another look at the PR.

Thanks for your contribution and the discussions so far.

…Go handle the panic itself. https://github.com/grpc/grpc-go/pull/6574/files/5936668fb03365179b2900bb139865add6b4aa93#r1324886324

searKing · 2023-09-16T07:13:32Z

Once you move the implementation to directly use the functionality provided by the fmt package, please let us know and we will take another look at the PR.

Thanks for your contribution and the discussions so far.

Commit Done.

use fmt.Sprint(v) instead of v.String() since it's more efficient than fmt.Sprintf("%s", v).
panic&recover codes removed, fmt will take this case.

attributes/attributes.go

resolver_test.go

…nd live with what `%#v` gives us

resolver_test.go

Co-authored-by: Easwar Swaminathan <easwars@google.com>

easwars · 2023-09-21T17:23:28Z

@dfawley for second set of eyes.

easwars · 2023-09-22T19:09:21Z

Thanks for your contribution, @searKing.

searKing force-pushed the typednil branch 2 times, most recently from 3be80aa to e1ad082 Compare August 23, 2023 05:59

attributes: catch panics during typed nil formatting by fmt.Stringer

1114f39

searKing force-pushed the typednil branch from e1ad082 to 1114f39 Compare August 23, 2023 06:20

dfawley added this to the 1.59 Release milestone Aug 24, 2023

easwars reviewed Aug 29, 2023

View reviewed changes

easwars assigned searKing and easwars Aug 29, 2023

easwars added Type: Internal Cleanup Refactors, etc Status: Requires Reporter Clarification labels Aug 29, 2023

attributes: format invalid argument for '%p' to string in style `<typ…

dbe2270

…e=value>`, not `%!verb(type=value)` grpc#6574 (comment)

resolver: Add TestResolverAddressesWithTypedNilAttribute for ensures …

5936668

…no panic if typed-nil attributes within resolver.State.Addresses (grpc#6574)

arvindbr8 removed the Status: Requires Reporter Clarification label Sep 5, 2023

easwars added the Status: Requires Reporter Clarification label Sep 5, 2023

easwars removed the Status: Requires Reporter Clarification label Sep 12, 2023

easwars reviewed Sep 13, 2023

View reviewed changes

attributes/attributes.go Outdated Show resolved Hide resolved

easwars added the Status: Requires Reporter Clarification label Sep 13, 2023

easwars removed their assignment Sep 15, 2023

attributes: use fmt.Sprint(v) instead of v.String() and recover, let …

e8fe6ad

…Go handle the panic itself. https://github.com/grpc/grpc-go/pull/6574/files/5936668fb03365179b2900bb139865add6b4aa93#r1324886324

easwars removed the Status: Requires Reporter Clarification label Sep 18, 2023

easwars reviewed Sep 18, 2023

View reviewed changes

attributes/attributes.go Outdated Show resolved Hide resolved

attributes/attributes.go Outdated Show resolved Hide resolved

resolver_test.go Outdated Show resolved Hide resolved

resolver_test.go Outdated Show resolved Hide resolved

easwars self-assigned this Sep 18, 2023

easwars added the Status: Requires Reporter Clarification label Sep 18, 2023

searKing added 3 commits September 19, 2023 11:25

resolver: use t.Name() instead of a const scheme

55f2b24

attributes: let badVerb handle nil

32abddd

attributes: get rid of the special handling in the badVerb function a…

2e3e6ee

…nd live with what `%#v` gives us

ginayeh removed the Status: Requires Reporter Clarification label Sep 19, 2023

easwars reviewed Sep 19, 2023

View reviewed changes

resolver_test.go Outdated Show resolved Hide resolved

easwars added the Status: Requires Reporter Clarification label Sep 19, 2023

resolver: Update doc of TestResolverAddressesWithTypedNilAttribute

9116500

Co-authored-by: Easwar Swaminathan <easwars@google.com>

easwars approved these changes Sep 21, 2023

View reviewed changes

easwars requested a review from dfawley September 21, 2023 17:22

easwars assigned dfawley Sep 21, 2023

easwars removed the Status: Requires Reporter Clarification label Sep 21, 2023

dfawley approved these changes Sep 21, 2023

View reviewed changes

easwars merged commit 58e2f2b into grpc:master Sep 22, 2023
10 of 11 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

attributes: print typed nil values instead of panic #6574

attributes: print typed nil values instead of panic #6574

searKing commented Aug 23, 2023 •

edited by dfawley

linux-foundation-easycla bot commented Aug 23, 2023 •

edited

easwars Aug 29, 2023

searKing Aug 30, 2023

easwars commented Aug 29, 2023

searKing commented Aug 30, 2023 •

edited

searKing commented Aug 30, 2023

easwars commented Aug 31, 2023

searKing commented Sep 1, 2023 •

edited

easwars commented Sep 5, 2023

searKing commented Sep 6, 2023 •

edited

easwars commented Sep 15, 2023

searKing commented Sep 16, 2023

easwars commented Sep 21, 2023

easwars commented Sep 22, 2023

attributes: print typed nil values instead of panic #6574

attributes: print typed nil values instead of panic #6574

Conversation

searKing commented Aug 23, 2023 • edited by dfawley

linux-foundation-easycla bot commented Aug 23, 2023 • edited

easwars Aug 29, 2023

Choose a reason for hiding this comment

searKing Aug 30, 2023

Choose a reason for hiding this comment

easwars commented Aug 29, 2023

searKing commented Aug 30, 2023 • edited

searKing commented Aug 30, 2023

easwars commented Aug 31, 2023

searKing commented Sep 1, 2023 • edited

easwars commented Sep 5, 2023

searKing commented Sep 6, 2023 • edited

easwars commented Sep 15, 2023

searKing commented Sep 16, 2023

easwars commented Sep 21, 2023

easwars commented Sep 22, 2023

searKing commented Aug 23, 2023 •

edited by dfawley

linux-foundation-easycla bot commented Aug 23, 2023 •

edited

searKing commented Aug 30, 2023 •

edited

searKing commented Sep 1, 2023 •

edited

searKing commented Sep 6, 2023 •

edited