From 4cb7bb79fc62aa54ec44593c87d89989fca9eaff Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov Date: Thu, 11 May 2023 11:52:57 +0200 Subject: [PATCH 1/5] fix: Introduce cloneContext --- scope.go | 17 +++++++++++++++-- scope_test.go | 22 ++++++++++++++++++++++ tracing.go | 2 +- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/scope.go b/scope.go index 03602bd65..52cb76192 100644 --- a/scope.go +++ b/scope.go @@ -287,7 +287,7 @@ func (scope *Scope) Clone() *Scope { clone.tags[key] = value } for key, value := range scope.contexts { - clone.contexts[key] = value + clone.contexts[key] = cloneContext(value) } for key, value := range scope.extra { clone.extra[key] = value @@ -350,7 +350,7 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event { // Ensure we are not overwriting event fields if _, ok := event.Contexts[key]; !ok { - event.Contexts[key] = value + event.Contexts[key] = cloneContext(value) } } } @@ -404,3 +404,16 @@ func (scope *Scope) ApplyToEvent(event *Event, hint *EventHint) *Event { return event } + +// cloneContext returns a new context with keys and values copied from the passed one. +// +// Note: a new Context (map) is returned, but the function does NOT do +// a proper deep copy: if some context values are pointer types (e.g. maps), +// they won't be properly copied. +func cloneContext(c Context) Context { + res := Context{} + for k, v := range c { + res[k] = v + } + return res +} diff --git a/scope_test.go b/scope_test.go index eb295598b..6a325fcaa 100644 --- a/scope_test.go +++ b/scope_test.go @@ -618,3 +618,25 @@ func TestEventProcessorsAddEventProcessor(t *testing.T) { t.Error("event should be dropped") } } + +func TestCloneContext(t *testing.T) { + context := Context{ + "key1": "value1", + "key2": []string{"s1", "s2"}, + } + + clone := cloneContext(context) + + // Value-wise they should be identical + assertEqual(t, context, clone) + // ..but it shouldn't be the same map + if &context == &clone { + t.Error("original and cloned context should be different objects") + } + + slice_original := context["key2"].([]string) + slice_clone := clone["key2"].([]string) + if &slice_original[0] != &slice_clone[0] { + t.Error("complex values are not supposed to be copied") + } +} diff --git a/tracing.go b/tracing.go index 519dc20ef..da43eecec 100644 --- a/tracing.go +++ b/tracing.go @@ -515,7 +515,7 @@ func (s *Span) toEvent() *Event { contexts := map[string]Context{} for k, v := range s.contexts { - contexts[k] = v + contexts[k] = cloneContext(v) } contexts["trace"] = s.traceContext().Map() From 4444ab731ca5b8351c2dc3f72808a67b7b3f43b4 Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov Date: Thu, 11 May 2023 14:05:08 +0200 Subject: [PATCH 2/5] Add a test --- tracing_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tracing_test.go b/tracing_test.go index 8a90e4e22..bb22c5155 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -10,6 +10,7 @@ import ( "math" "net/http" "reflect" + "sync" "testing" "time" @@ -890,3 +891,38 @@ func TestDeprecatedSpanOptionSpanSampled(t *testing.T) { func TestDeprecatedSpanOptionTransctionSource(t *testing.T) { StartSpan(context.Background(), "op", TransctionSource("src")) } + +func TestConcurrentContextAccess(t *testing.T) { + ctx := NewTestContext(ClientOptions{ + EnableTracing: true, + TracesSampleRate: 1, + }) + hub := GetHubFromContext(ctx) + + const writers_num = 200 + // Unbuffered channel, writing to it will be block if nobody reads + c := make(chan *Span) + for i := 0; i < writers_num; i++ { + go func() { + // Every span will be a transaction + span := StartSpan(ctx, "test") + c <- span + hub.Scope().SetContext("device", Context{"test": "bla"}) + }() + } + + var wg sync.WaitGroup + wg.Add(writers_num) + + go func() { + for span := range c { + span := span + go func() { + span.Finish() + wg.Done() + }() + } + }() + + wg.Wait() +} From e89ffb67c455b9c55778567d2df3049251d9ef8b Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov Date: Thu, 11 May 2023 14:17:25 +0200 Subject: [PATCH 3/5] Update test --- tracing_test.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tracing_test.go b/tracing_test.go index bb22c5155..c144bf484 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -892,6 +892,9 @@ func TestDeprecatedSpanOptionTransctionSource(t *testing.T) { StartSpan(context.Background(), "op", TransctionSource("src")) } +// This test checks that there are no concurrent reads/writes to +// substructures in scope.contexts. +// See https://github.com/getsentry/sentry-go/issues/570 for more details. func TestConcurrentContextAccess(t *testing.T) { ctx := NewTestContext(ClientOptions{ EnableTracing: true, @@ -900,13 +903,15 @@ func TestConcurrentContextAccess(t *testing.T) { hub := GetHubFromContext(ctx) const writers_num = 200 + // Unbuffered channel, writing to it will be block if nobody reads c := make(chan *Span) + + // Start writers for i := 0; i < writers_num; i++ { go func() { - // Every span will be a transaction - span := StartSpan(ctx, "test") - c <- span + transaction := StartTransaction(ctx, "test") + c <- transaction hub.Scope().SetContext("device", Context{"test": "bla"}) }() } @@ -914,12 +919,15 @@ func TestConcurrentContextAccess(t *testing.T) { var wg sync.WaitGroup wg.Add(writers_num) + // Start readers go func() { - for span := range c { - span := span + for transaction := range c { + transaction := transaction go func() { - span.Finish() - wg.Done() + defer wg.Done() + // While finalizing every transaction, scope.Contexts and Event.Contexts fields + // will be accessed, e.g. in environmentIntegration.processor() + transaction.Finish() }() } }() From d77c5ae91c24eb363cb355a26c3e956215867416 Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov Date: Thu, 11 May 2023 14:32:44 +0200 Subject: [PATCH 4/5] Fix lint --- scope_test.go | 6 +++--- tracing_test.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/scope_test.go b/scope_test.go index 6a325fcaa..b2ca37465 100644 --- a/scope_test.go +++ b/scope_test.go @@ -634,9 +634,9 @@ func TestCloneContext(t *testing.T) { t.Error("original and cloned context should be different objects") } - slice_original := context["key2"].([]string) - slice_clone := clone["key2"].([]string) - if &slice_original[0] != &slice_clone[0] { + sliceOriginal := context["key2"].([]string) + sliceClone := clone["key2"].([]string) + if &sliceOriginal[0] != &sliceClone[0] { t.Error("complex values are not supposed to be copied") } } diff --git a/tracing_test.go b/tracing_test.go index c144bf484..34fda877a 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -902,13 +902,13 @@ func TestConcurrentContextAccess(t *testing.T) { }) hub := GetHubFromContext(ctx) - const writers_num = 200 + const writersNum = 200 // Unbuffered channel, writing to it will be block if nobody reads c := make(chan *Span) // Start writers - for i := 0; i < writers_num; i++ { + for i := 0; i < writersNum; i++ { go func() { transaction := StartTransaction(ctx, "test") c <- transaction @@ -917,7 +917,7 @@ func TestConcurrentContextAccess(t *testing.T) { } var wg sync.WaitGroup - wg.Add(writers_num) + wg.Add(writersNum) // Start readers go func() { From cb73fed5a9f622f4e706de15fec16015412195e1 Mon Sep 17 00:00:00 2001 From: Anton Ovchinnikov Date: Mon, 15 May 2023 13:11:44 +0200 Subject: [PATCH 5/5] go fmt --- _examples/basic/main.go | 6 +++--- _examples/recover-repanic/main.go | 2 +- tracing_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/_examples/basic/main.go b/_examples/basic/main.go index 9833cf456..bb1c51da6 100644 --- a/_examples/basic/main.go +++ b/_examples/basic/main.go @@ -3,9 +3,9 @@ // // Try it by running: // -// go run main.go -// go run main.go https://sentry.io -// go run main.go bad-url +// go run main.go +// go run main.go https://sentry.io +// go run main.go bad-url // // To actually report events to Sentry, set the DSN either by editing the // appropriate line below or setting the environment variable SENTRY_DSN to diff --git a/_examples/recover-repanic/main.go b/_examples/recover-repanic/main.go index 5344f0933..a35bc7482 100644 --- a/_examples/recover-repanic/main.go +++ b/_examples/recover-repanic/main.go @@ -5,7 +5,7 @@ // // Try it by running: // -// go run main.go +// go run main.go // // To actually report events to Sentry, set the DSN either by editing the // appropriate line below or setting the environment variable SENTRY_DSN to diff --git a/tracing_test.go b/tracing_test.go index a5e5b39db..dc7fd9d75 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -934,7 +934,7 @@ func TestConcurrentContextAccess(t *testing.T) { wg.Wait() } - + func TestAdjustingTransactionSourceBeforeSending(t *testing.T) { tests := []struct { name string