From 84dac9ae9808b45a7c11939e93f9db00b692e16b Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Fri, 16 Feb 2024 12:42:22 -0700 Subject: [PATCH 01/11] Fix race condition --- client/client.go | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/client/client.go b/client/client.go index 1f385f6af81..81a584f70af 100644 --- a/client/client.go +++ b/client/client.go @@ -81,6 +81,7 @@ import ( "context" "net" "strings" + "sync" ) type ctxKey struct{} @@ -104,7 +105,7 @@ type Info struct { // Metadata is an immutable map, meant to contain request metadata. type Metadata struct { - data map[string][]string + data sync.Map } // AuthData represents the authentication data as seen by authenticators tied to @@ -141,32 +142,39 @@ func FromContext(ctx context.Context) Info { // NewMetadata creates a new Metadata object to use in Info. md is used as-is. func NewMetadata(md map[string][]string) Metadata { + m := sync.Map{} + for k, v := range md { + m.Store(k, v) + } return Metadata{ - data: md, + data: m, } } // Get gets the value of the key from metadata, returning a copy. func (m Metadata) Get(key string) []string { - vals := m.data[key] - if len(vals) == 0 { + vals, ok := m.data.Load(key) + if !ok { // we didn't find the key, but perhaps it just has different cases? - for k, v := range m.data { - if strings.EqualFold(key, k) { + m.data.Range(func(k, v any) bool { + kStr := k.(string) + if strings.EqualFold(key, kStr) { vals = v // we optimize for the next lookup - m.data[key] = v + m.data.Store(key, v) } - } + return true + }) // if it's still not found, it's really not here - if len(vals) == 0 { + if vals == nil { return nil } } - ret := make([]string, len(vals)) - copy(ret, vals) + valStrs := vals.([]string) + ret := make([]string, len(valStrs)) + copy(ret, valStrs) return ret } From fe309e8794c01b1680025e811c5a4afab6ff98dc Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Fri, 16 Feb 2024 13:32:46 -0700 Subject: [PATCH 02/11] changelog --- .chloggen/make-metadata-thread-safe.yaml | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100755 .chloggen/make-metadata-thread-safe.yaml diff --git a/.chloggen/make-metadata-thread-safe.yaml b/.chloggen/make-metadata-thread-safe.yaml new file mode 100755 index 00000000000..e03b6788de9 --- /dev/null +++ b/.chloggen/make-metadata-thread-safe.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: bug_fix + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: client + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Make `Metadata.Get` thread safe + +# One or more tracking issues or pull requests related to the change +issues: [9595] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [] From 56d6273ee37b4aafed6fa26ecbf531426c842bd9 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 19 Feb 2024 10:00:06 -0700 Subject: [PATCH 03/11] Use sync.Mutex instead of sync.Map --- client/client.go | 33 +++++++++++++--------------- config/configgrpc/configgrpc_test.go | 5 ++++- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/client/client.go b/client/client.go index 81a584f70af..4df4507c3ed 100644 --- a/client/client.go +++ b/client/client.go @@ -105,7 +105,8 @@ type Info struct { // Metadata is an immutable map, meant to contain request metadata. type Metadata struct { - data sync.Map + data map[string][]string + mu *sync.Mutex } // AuthData represents the authentication data as seen by authenticators tied to @@ -142,39 +143,35 @@ func FromContext(ctx context.Context) Info { // NewMetadata creates a new Metadata object to use in Info. md is used as-is. func NewMetadata(md map[string][]string) Metadata { - m := sync.Map{} - for k, v := range md { - m.Store(k, v) - } return Metadata{ - data: m, + data: md, + mu: &sync.Mutex{}, } } // Get gets the value of the key from metadata, returning a copy. func (m Metadata) Get(key string) []string { - vals, ok := m.data.Load(key) - if !ok { + m.mu.Lock() + defer m.mu.Unlock() + vals := m.data[key] + if len(vals) == 0 { // we didn't find the key, but perhaps it just has different cases? - m.data.Range(func(k, v any) bool { - kStr := k.(string) - if strings.EqualFold(key, kStr) { + for k, v := range m.data { + if strings.EqualFold(key, k) { vals = v // we optimize for the next lookup - m.data.Store(key, v) + m.data[key] = v } - return true - }) + } // if it's still not found, it's really not here - if vals == nil { + if len(vals) == 0 { return nil } } - valStrs := vals.([]string) - ret := make([]string, len(valStrs)) - copy(ret, valStrs) + ret := make([]string, len(vals)) + copy(ret, vals) return ret } diff --git a/config/configgrpc/configgrpc_test.go b/config/configgrpc/configgrpc_test.go index 913ac06d206..f4ccfb17322 100644 --- a/config/configgrpc/configgrpc_test.go +++ b/config/configgrpc/configgrpc_test.go @@ -9,6 +9,7 @@ import ( "net" "os" "path/filepath" + "reflect" "runtime" "testing" "time" @@ -796,7 +797,9 @@ func TestContextWithClient(t *testing.T) { for _, tC := range testCases { t.Run(tC.desc, func(t *testing.T) { cl := client.FromContext(contextWithClient(tC.input, tC.doMetadata)) - assert.Equal(t, tC.expected, cl) + assert.Equal(t, tC.expected.Addr, cl.Addr) + assert.Equal(t, tC.expected.Auth, cl.Auth) + assert.True(t, reflect.DeepEqual(tC.expected.Metadata, cl.Metadata)) }) } } From 086dd0f66cb2dd6d06443f8f170ebcacfa2e8d62 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 19 Feb 2024 10:12:20 -0700 Subject: [PATCH 04/11] Add client replace to check-contrib --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index d595c03882f..3c4558b192f 100644 --- a/Makefile +++ b/Makefile @@ -246,6 +246,7 @@ check-contrib: @echo Setting contrib at $(CONTRIB_PATH) to use this core checkout @$(MAKE) -C $(CONTRIB_PATH) for-all CMD="$(GOCMD) mod edit \ -replace go.opentelemetry.io/collector=$(CURDIR) \ + -replace go.opentelemetry.io/collector/client=$(CURDIR)/client \ -replace go.opentelemetry.io/collector/component=$(CURDIR)/component \ -replace go.opentelemetry.io/collector/config/configauth=$(CURDIR)/config/configauth \ -replace go.opentelemetry.io/collector/config/configcompression=$(CURDIR)/config/configcompression \ From 90b63a7124221cf61dceb8d73a75396c2b1ea0e0 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 19 Feb 2024 10:17:20 -0700 Subject: [PATCH 05/11] revert makefile --- Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/Makefile b/Makefile index 3c4558b192f..d595c03882f 100644 --- a/Makefile +++ b/Makefile @@ -246,7 +246,6 @@ check-contrib: @echo Setting contrib at $(CONTRIB_PATH) to use this core checkout @$(MAKE) -C $(CONTRIB_PATH) for-all CMD="$(GOCMD) mod edit \ -replace go.opentelemetry.io/collector=$(CURDIR) \ - -replace go.opentelemetry.io/collector/client=$(CURDIR)/client \ -replace go.opentelemetry.io/collector/component=$(CURDIR)/component \ -replace go.opentelemetry.io/collector/config/configauth=$(CURDIR)/config/configauth \ -replace go.opentelemetry.io/collector/config/configcompression=$(CURDIR)/config/configcompression \ From b98b54a31f8ca9182858b0efdbb1e8e101be4a9b Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 19 Feb 2024 10:33:27 -0700 Subject: [PATCH 06/11] Enforce that a get is only tried if there is data --- client/client.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/client/client.go b/client/client.go index 4df4507c3ed..773a6d10e53 100644 --- a/client/client.go +++ b/client/client.go @@ -151,6 +151,10 @@ func NewMetadata(md map[string][]string) Metadata { // Get gets the value of the key from metadata, returning a copy. func (m Metadata) Get(key string) []string { + if m.data == nil { + return make([]string, 0) + } + m.mu.Lock() defer m.mu.Unlock() vals := m.data[key] From 4417ddf25ad503541791603a073fe9edac9e1057 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 20 Feb 2024 09:52:01 -0700 Subject: [PATCH 07/11] Add test --- client/client_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/client_test.go b/client/client_test.go index f71b7da0728..5b3dab84dc5 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -89,3 +89,8 @@ func TestMetadata(t *testing.T) { assert.Empty(t, md.Get("non-existent-key")) } + +func TestUninstantiatedMetadata(t *testing.T) { + i := Info{} + assert.Empty(t, i.Metadata.Get("test")) +} From 8d3ffe0893d294ae8409bc9d92d572d6705761ae Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 20 Feb 2024 18:05:48 -0700 Subject: [PATCH 08/11] Remove need for mutex --- client/client.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/client/client.go b/client/client.go index 773a6d10e53..f29e0031d9d 100644 --- a/client/client.go +++ b/client/client.go @@ -81,7 +81,6 @@ import ( "context" "net" "strings" - "sync" ) type ctxKey struct{} @@ -106,7 +105,6 @@ type Info struct { // Metadata is an immutable map, meant to contain request metadata. type Metadata struct { data map[string][]string - mu *sync.Mutex } // AuthData represents the authentication data as seen by authenticators tied to @@ -143,28 +141,27 @@ func FromContext(ctx context.Context) Info { // NewMetadata creates a new Metadata object to use in Info. md is used as-is. func NewMetadata(md map[string][]string) Metadata { + c := make(map[string][]string, len(md)) + for k, v := range md { + c[k] = v + } return Metadata{ - data: md, - mu: &sync.Mutex{}, + data: c, } } // Get gets the value of the key from metadata, returning a copy. func (m Metadata) Get(key string) []string { - if m.data == nil { - return make([]string, 0) + if len(m.data) == 0 { + return nil } - m.mu.Lock() - defer m.mu.Unlock() vals := m.data[key] if len(vals) == 0 { // we didn't find the key, but perhaps it just has different cases? for k, v := range m.data { if strings.EqualFold(key, k) { vals = v - // we optimize for the next lookup - m.data[key] = v } } From b7250e22f7a3cfbf2e2ef43405070765aa85eb97 Mon Sep 17 00:00:00 2001 From: TylerHelmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 20 Feb 2024 22:12:04 -0700 Subject: [PATCH 09/11] use strings.ToLower --- client/client.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/client/client.go b/client/client.go index f29e0031d9d..b8775d5a440 100644 --- a/client/client.go +++ b/client/client.go @@ -143,7 +143,7 @@ func FromContext(ctx context.Context) Info { func NewMetadata(md map[string][]string) Metadata { c := make(map[string][]string, len(md)) for k, v := range md { - c[k] = v + c[strings.ToLower(k)] = v } return Metadata{ data: c, @@ -156,19 +156,9 @@ func (m Metadata) Get(key string) []string { return nil } - vals := m.data[key] + vals := m.data[strings.ToLower(key)] if len(vals) == 0 { - // we didn't find the key, but perhaps it just has different cases? - for k, v := range m.data { - if strings.EqualFold(key, k) { - vals = v - } - } - - // if it's still not found, it's really not here - if len(vals) == 0 { - return nil - } + return nil } ret := make([]string, len(vals)) From 682949bec37bcc62686c1d008707031541c33900 Mon Sep 17 00:00:00 2001 From: TylerHelmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 20 Feb 2024 22:13:41 -0700 Subject: [PATCH 10/11] another test scenario --- client/client_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 5b3dab84dc5..ddec6bd89dd 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -77,10 +77,11 @@ func TestFromContext(t *testing.T) { } func TestMetadata(t *testing.T) { - source := map[string][]string{"test-key": {"test-val"}} + source := map[string][]string{"test-key": {"test-val"}, "TEST-KEY-2": {"test-val"}} md := NewMetadata(source) assert.Equal(t, []string{"test-val"}, md.Get("test-key")) - assert.Equal(t, []string{"test-val"}, md.Get("test-KEY")) // case insensitive lookup + assert.Equal(t, []string{"test-val"}, md.Get("test-KEY")) // case insensitive lookup + assert.Equal(t, []string{"test-val"}, md.Get("test-key-2")) // case insensitive lookup // test if copy. In regular use, source cannot change val := md.Get("test-key") From 5fbcc1df470c968d79fdbd990c34875631ffdf79 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 21 Feb 2024 10:44:54 -0700 Subject: [PATCH 11/11] revert configgrpc test --- config/configgrpc/configgrpc_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/config/configgrpc/configgrpc_test.go b/config/configgrpc/configgrpc_test.go index f4ccfb17322..913ac06d206 100644 --- a/config/configgrpc/configgrpc_test.go +++ b/config/configgrpc/configgrpc_test.go @@ -9,7 +9,6 @@ import ( "net" "os" "path/filepath" - "reflect" "runtime" "testing" "time" @@ -797,9 +796,7 @@ func TestContextWithClient(t *testing.T) { for _, tC := range testCases { t.Run(tC.desc, func(t *testing.T) { cl := client.FromContext(contextWithClient(tC.input, tC.doMetadata)) - assert.Equal(t, tC.expected.Addr, cl.Addr) - assert.Equal(t, tC.expected.Auth, cl.Auth) - assert.True(t, reflect.DeepEqual(tC.expected.Metadata, cl.Metadata)) + assert.Equal(t, tC.expected, cl) }) } }