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

attributes: print typed nil values instead of panic #6574

Merged
merged 8 commits into from
Sep 22, 2023

Conversation

searKing
Copy link
Contributor

@searKing searKing commented Aug 23, 2023

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
Copy link

linux-foundation-easycla bot commented Aug 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@searKing searKing force-pushed the typednil branch 2 times, most recently from 3be80aa to e1ad082 Compare August 23, 2023 05:59
@dfawley dfawley added this to the 1.59 Release milestone Aug 24, 2023
fmt.Println("a6:", a6.String())
fmt.Println("a7:", a7.String())
// Output:
// a1: {"<%!p(attributes_test.key={})>": "<nil>" }
Copy link
Contributor

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

Copy link
Contributor Author

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

@easwars
Copy link
Contributor

easwars commented Aug 29, 2023

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

@searKing
Copy link
Contributor Author

searKing commented Aug 30, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

easwars commented Aug 31, 2023

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
Copy link
Contributor Author

searKing commented Sep 1, 2023

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:

  1. 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
}
  1. 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
}
  1. 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 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
Copy link
Contributor

easwars commented Sep 5, 2023

@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
Copy link
Contributor Author

searKing commented Sep 6, 2023

@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.
  1. 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 }
  1. 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 }
  1. 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) }
  1. 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 }
  1. 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 Outdated Show resolved Hide resolved
@easwars
Copy link
Contributor

easwars commented Sep 15, 2023

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.

@easwars easwars removed their assignment Sep 15, 2023
@searKing
Copy link
Contributor Author

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 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
resolver_test.go Outdated Show resolved Hide resolved
Co-authored-by: Easwar Swaminathan <easwars@google.com>
@easwars
Copy link
Contributor

easwars commented Sep 21, 2023

@dfawley for second set of eyes.

@easwars easwars merged commit 58e2f2b into grpc:master Sep 22, 2023
9 of 10 checks passed
@easwars
Copy link
Contributor

easwars commented Sep 22, 2023

Thanks for your contribution, @searKing.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants