fix: Clone scope.Context in multiple places to avoid concurrent reads/writes (#638)

Author: tonyo

_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

_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

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

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")
+	}
+
+	sliceOriginal := context["key2"].([]string)
+	sliceClone := clone["key2"].([]string)
+	if &sliceOriginal[0] != &sliceClone[0] {
+		t.Error("complex values are not supposed to be copied")
+	}
+}

tracing.go

@@ -535,7 +535,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()
 

tracing_test.go

@@ -10,6 +10,7 @@ import (
 	"math"
 	"net/http"
 	"reflect"
+	"sync"
 	"testing"
 	"time"
 
@@ -891,6 +892,49 @@ 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,
+		TracesSampleRate: 1,
+	})
+	hub := GetHubFromContext(ctx)
+
+	const writersNum = 200
+
+	// Unbuffered channel, writing to it will be block if nobody reads
+	c := make(chan *Span)
+
+	// Start writers
+	for i := 0; i < writersNum; i++ {
+		go func() {
+			transaction := StartTransaction(ctx, "test")
+			c <- transaction
+			hub.Scope().SetContext("device", Context{"test": "bla"})
+		}()
+	}
+
+	var wg sync.WaitGroup
+	wg.Add(writersNum)
+
+	// Start readers
+	go func() {
+		for transaction := range c {
+			transaction := transaction
+			go func() {
+				defer wg.Done()
+				// While finalizing every transaction, scope.Contexts and Event.Contexts fields
+				// will be accessed, e.g. in environmentIntegration.processor()
+				transaction.Finish()
+			}()
+		}
+	}()
+
+	wg.Wait()
+}
+
 func TestAdjustingTransactionSourceBeforeSending(t *testing.T) {
 	tests := []struct {
 		name                   string