From e73bfc234642e8d3e6c5a9303febb284a2cfbd42 Mon Sep 17 00:00:00 2001 From: horpto <__singleton__@hackerdom.ru> Date: Fri, 19 Aug 2022 20:01:22 +0300 Subject: [PATCH 1/6] Add helper for getting value from incoming context FromIncomingContext creates a copy of md every time. So even if you need a single header value you will get a value and an overhead for this. Many interceptors and user code doesn't need all values at a time. --- metadata/metadata.go | 38 ++++++++++++++++++++++++++++++++++---- metadata/metadata_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 8e0f6abe89d7..346e4a22b351 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -50,7 +50,7 @@ type MD map[string][]string // Keys beginning with "grpc-" are reserved for grpc-internal use only and may // result in errors if set in metadata. func New(m map[string]string) MD { - md := MD{} + md := make(MD, len(m)) for k, val := range m { key := strings.ToLower(k) md[key] = append(md[key], val) @@ -74,7 +74,7 @@ func Pairs(kv ...string) MD { if len(kv)%2 == 1 { panic(fmt.Sprintf("metadata: Pairs got the odd number of input pairs for metadata: %d", len(kv))) } - md := MD{} + md := make(MD, len(kv)/2) for i := 0; i < len(kv); i += 2 { key := strings.ToLower(kv[i]) md[key] = append(md[key], kv[i+1]) @@ -182,7 +182,7 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { if !ok { return nil, false } - out := MD{} + out := make(MD, len(md)) for k, v := range md { // We need to manually convert all keys to lower case, because MD is a // map, and there's no guarantee that the MD attached to the context is @@ -195,6 +195,36 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { return out, true } +// ValueFromIncomingContext returns value from the incoming metadata if exists. +// +// This is intended for using as fast access to context value with string constant. +func ValueFromIncomingContext(ctx context.Context, key string) []string { + md, ok := ctx.Value(mdIncomingKey{}).(MD) + if !ok { + return nil + } + // fastpath + if v, ok := md[key]; ok { + res := make([]string, len(v)) + copy(res, v) + return res + } + + // slowpath + key = strings.ToLower(key) + for k, v := range md { + // We need to manually convert all keys to lower case, because MD is a + // map, and there's no guarantee that the MD attached to the context is + // created using our helper functions. + if strings.ToLower(k) == key { + s := make([]string, len(v)) + copy(s, v) + return s + } + } + return nil +} + // FromOutgoingContextRaw returns the un-merged, intermediary contents of rawMD. // // Remember to perform strings.ToLower on the keys, for both the returned MD (MD @@ -222,7 +252,7 @@ func FromOutgoingContext(ctx context.Context) (MD, bool) { return nil, false } - out := MD{} + out := make(MD, len(raw.md)+len(raw.added)) for k, v := range raw.md { // We need to manually convert all keys to lower case, because MD is a // map, and there's no guarantee that the MD attached to the context is diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 89be06eaada0..3980e0d40658 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -198,6 +198,31 @@ func (s) TestDelete(t *testing.T) { } } +func (s) TestValueFromIncomingContext(t *testing.T) { + md := Pairs( + "X-My-Header-1", "42", + "X-My-Header-2", "43-1", + "X-My-Header-2", "43-2", + ) + ctx := NewIncomingContext(context.Background(), md) + + var v []string + v = ValueFromIncomingContext(ctx, "X-My-Header-1") + if !reflect.DeepEqual(v, []string{"42"}) { + t.Errorf("value from context is %v", v) + } + + v = ValueFromIncomingContext(ctx, "x-my-header-1") + if !reflect.DeepEqual(v, []string{"42"}) { + t.Errorf("value from context is %v", v) + } + + v = ValueFromIncomingContext(ctx, "x-my-header-2") + if !reflect.DeepEqual(v, []string{"43-1", "43-2"}) { + t.Errorf("value from context is %v", v) + } +} + func (s) TestAppendToOutgoingContext(t *testing.T) { // Pre-existing metadata tCtx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) From 3dfd0969053cd972ce6a689ffd6dcfcd4dc324ec Mon Sep 17 00:00:00 2001 From: horpto <__singleton__@hackerdom.ru> Date: Fri, 2 Sep 2022 02:06:42 +0300 Subject: [PATCH 2/6] review fixes --- metadata/metadata.go | 7 +++---- metadata/metadata_test.go | 39 +++++++++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 346e4a22b351..bb84d6f0f1dc 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -197,20 +197,19 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { // ValueFromIncomingContext returns value from the incoming metadata if exists. // -// This is intended for using as fast access to context value with string constant. +// ValueFromIncomingContext returns the metadata value corresponding to the metadata +// key from the incoming metadata if it exists. func ValueFromIncomingContext(ctx context.Context, key string) []string { md, ok := ctx.Value(mdIncomingKey{}).(MD) if !ok { return nil } - // fastpath + if v, ok := md[key]; ok { res := make([]string, len(v)) copy(res, v) return res } - - // slowpath key = strings.ToLower(key) for k, v := range md { // We need to manually convert all keys to lower case, because MD is a diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 3980e0d40658..34aedeb4baf0 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -206,20 +206,31 @@ func (s) TestValueFromIncomingContext(t *testing.T) { ) ctx := NewIncomingContext(context.Background(), md) - var v []string - v = ValueFromIncomingContext(ctx, "X-My-Header-1") - if !reflect.DeepEqual(v, []string{"42"}) { - t.Errorf("value from context is %v", v) - } - - v = ValueFromIncomingContext(ctx, "x-my-header-1") - if !reflect.DeepEqual(v, []string{"42"}) { - t.Errorf("value from context is %v", v) - } - - v = ValueFromIncomingContext(ctx, "x-my-header-2") - if !reflect.DeepEqual(v, []string{"43-1", "43-2"}) { - t.Errorf("value from context is %v", v) + for _, test := range []struct { + key string + want []string + }{ + { + key: "X-My-Header-1", + want: []string{"42"}, + }, + { + key: "x-my-header-1", + want: []string{"42"}, + }, + { + key: "x-my-header-2", + want: []string{"43-1", "43-2"}, + }, + { + key: "x-unknown", + want: nil, + }, + } { + v := ValueFromIncomingContext(ctx, test.key) + if !reflect.DeepEqual(v, test.want) { + t.Errorf("value of metadata is %v, want %v", v, test.want) + } } } From b6bddb532977c8bbd83de5fdcd30c645a5d544ae Mon Sep 17 00:00:00 2001 From: horpto <__singleton__@hackerdom.ru> Date: Wed, 7 Sep 2022 10:07:04 +0300 Subject: [PATCH 3/6] fixes nits --- metadata/metadata.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index bb84d6f0f1dc..12bcb0113585 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -195,8 +195,6 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { return out, true } -// ValueFromIncomingContext returns value from the incoming metadata if exists. -// // ValueFromIncomingContext returns the metadata value corresponding to the metadata // key from the incoming metadata if it exists. func ValueFromIncomingContext(ctx context.Context, key string) []string { @@ -206,9 +204,9 @@ func ValueFromIncomingContext(ctx context.Context, key string) []string { } if v, ok := md[key]; ok { - res := make([]string, len(v)) - copy(res, v) - return res + vals := make([]string, len(v)) + copy(vals, v) + return vals } key = strings.ToLower(key) for k, v := range md { @@ -216,9 +214,9 @@ func ValueFromIncomingContext(ctx context.Context, key string) []string { // map, and there's no guarantee that the MD attached to the context is // created using our helper functions. if strings.ToLower(k) == key { - s := make([]string, len(v)) - copy(s, v) - return s + vals := make([]string, len(v)) + copy(vals, v) + return vals } } return nil From 16daf0a754d61ca992b526c2d1658b89ad29a933 Mon Sep 17 00:00:00 2001 From: horpto <__singleton__@hackerdom.ru> Date: Wed, 7 Sep 2022 21:27:57 +0300 Subject: [PATCH 4/6] review fixes for md size and copyOf --- metadata/metadata.go | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 12bcb0113585..6a51768bf3d7 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -188,9 +188,7 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { // map, and there's no guarantee that the MD attached to the context is // created using our helper functions. key := strings.ToLower(k) - s := make([]string, len(v)) - copy(s, v) - out[key] = s + out[key] = copyOf(v) } return out, true } @@ -204,9 +202,7 @@ func ValueFromIncomingContext(ctx context.Context, key string) []string { } if v, ok := md[key]; ok { - vals := make([]string, len(v)) - copy(vals, v) - return vals + return copyOf(v) } key = strings.ToLower(key) for k, v := range md { @@ -214,14 +210,19 @@ func ValueFromIncomingContext(ctx context.Context, key string) []string { // map, and there's no guarantee that the MD attached to the context is // created using our helper functions. if strings.ToLower(k) == key { - vals := make([]string, len(v)) - copy(vals, v) - return vals + return copyOf(v) } } return nil } +// the returned slice must not be modified in place +func copyOf(v []string) (vals []string) { + vals = make([]string, len(v)) + copy(vals, v) + return +} + // FromOutgoingContextRaw returns the un-merged, intermediary contents of rawMD. // // Remember to perform strings.ToLower on the keys, for both the returned MD (MD @@ -249,15 +250,18 @@ func FromOutgoingContext(ctx context.Context) (MD, bool) { return nil, false } - out := make(MD, len(raw.md)+len(raw.added)) + mdSize := len(raw.md) + for i := range raw.added { + mdSize += len(raw.added[i]) / 2 + } + + out := make(MD, mdSize) for k, v := range raw.md { // We need to manually convert all keys to lower case, because MD is a // map, and there's no guarantee that the MD attached to the context is // created using our helper functions. key := strings.ToLower(k) - s := make([]string, len(v)) - copy(s, v) - out[key] = s + out[key] = copyOf(v) } for _, added := range raw.added { if len(added)%2 == 1 { From a4b627a964f042147b5c1b88c6796be35be6e39f Mon Sep 17 00:00:00 2001 From: horpto <__singleton__@hackerdom.ru> Date: Wed, 7 Sep 2022 21:34:24 +0300 Subject: [PATCH 5/6] review fixes for ToLower(key) --- metadata/metadata.go | 3 +-- metadata/metadata_test.go | 9 +++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 6a51768bf3d7..610ec09621b5 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -194,7 +194,7 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { } // ValueFromIncomingContext returns the metadata value corresponding to the metadata -// key from the incoming metadata if it exists. +// key from the incoming metadata if it exists. Key must be lower-case. func ValueFromIncomingContext(ctx context.Context, key string) []string { md, ok := ctx.Value(mdIncomingKey{}).(MD) if !ok { @@ -204,7 +204,6 @@ func ValueFromIncomingContext(ctx context.Context, key string) []string { if v, ok := md[key]; ok { return copyOf(v) } - key = strings.ToLower(key) for k, v := range md { // We need to manually convert all keys to lower case, because MD is a // map, and there's no guarantee that the MD attached to the context is diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 34aedeb4baf0..57763cd6a973 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -203,6 +203,7 @@ func (s) TestValueFromIncomingContext(t *testing.T) { "X-My-Header-1", "42", "X-My-Header-2", "43-1", "X-My-Header-2", "43-2", + "x-my-header-3", "44", ) ctx := NewIncomingContext(context.Background(), md) @@ -210,10 +211,6 @@ func (s) TestValueFromIncomingContext(t *testing.T) { key string want []string }{ - { - key: "X-My-Header-1", - want: []string{"42"}, - }, { key: "x-my-header-1", want: []string{"42"}, @@ -222,6 +219,10 @@ func (s) TestValueFromIncomingContext(t *testing.T) { key: "x-my-header-2", want: []string{"43-1", "43-2"}, }, + { + key: "x-my-header-3", + want: []string{"44"}, + }, { key: "x-unknown", want: nil, From e399e1103ffa856f31ff3f8183903c1bbab87d05 Mon Sep 17 00:00:00 2001 From: horpto <__singleton__@hackerdom.ru> Date: Wed, 7 Sep 2022 22:20:01 +0300 Subject: [PATCH 6/6] fix return and add experimental tag --- metadata/metadata.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/metadata/metadata.go b/metadata/metadata.go index 610ec09621b5..98d62e0675f6 100644 --- a/metadata/metadata.go +++ b/metadata/metadata.go @@ -195,6 +195,11 @@ func FromIncomingContext(ctx context.Context) (MD, bool) { // ValueFromIncomingContext returns the metadata value corresponding to the metadata // key from the incoming metadata if it exists. Key must be lower-case. +// +// Experimental +// +// Notice: This API is EXPERIMENTAL and may be changed or removed in a +// later release. func ValueFromIncomingContext(ctx context.Context, key string) []string { md, ok := ctx.Value(mdIncomingKey{}).(MD) if !ok { @@ -216,10 +221,10 @@ func ValueFromIncomingContext(ctx context.Context, key string) []string { } // the returned slice must not be modified in place -func copyOf(v []string) (vals []string) { - vals = make([]string, len(v)) +func copyOf(v []string) []string { + vals := make([]string, len(v)) copy(vals, v) - return + return vals } // FromOutgoingContextRaw returns the un-merged, intermediary contents of rawMD.