Skip to content

Commit

Permalink
fix: Clone scope.Context in multiple places to avoid concurrent reads…
Browse files Browse the repository at this point in the history
…/writes (#638)
  • Loading branch information
tonyo authored Jun 14, 2023
1 parent 97a00a4 commit 513eb10
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 7 deletions.
6 changes: 3 additions & 3 deletions _examples/basic/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion _examples/recover-repanic/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 15 additions & 2 deletions scope.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}
}
Expand Down Expand Up @@ -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
}
22 changes: 22 additions & 0 deletions scope_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
2 changes: 1 addition & 1 deletion tracing.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
44 changes: 44 additions & 0 deletions tracing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"math"
"net/http"
"reflect"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 513eb10

Please sign in to comment.