Skip to content

Commit

Permalink
tracer.StartSpanFromContext: Fix a race if opts is reused
Browse files Browse the repository at this point in the history
The opts input slice comes from the caller. If it has extra capacity,
the calls to append will use the slice that is passed in. If this
function is called from multiple goroutines, this causes a race,
where spans may not have the correct parents.

Add a test to demonstrate the error. This test fails with race, and
should also fail without race detection, but that will depend on the
race itself.
  • Loading branch information
evanj committed Jan 14, 2022
1 parent d688b28 commit d7eb0d2
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 3 deletions.
11 changes: 8 additions & 3 deletions ddtrace/tracer/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,19 @@ func SpanFromContext(ctx context.Context) (Span, bool) {
// is found in the context, it will be used as the parent of the resulting span. If the ChildOf
// option is passed, the span from context will take precedence over it as the parent span.
func StartSpanFromContext(ctx context.Context, operationName string, opts ...StartSpanOption) (Span, context.Context) {
// copy opts in case the caller reuses the slice in parallel
// we will add at least 1, at most 2 items
optsLocal := make([]StartSpanOption, len(opts), len(opts)+2)
copy(optsLocal, opts)

if ctx == nil {
// default to context.Background() to avoid panics on Go >= 1.15
ctx = context.Background()
} else if s, ok := SpanFromContext(ctx); ok {
opts = append(opts, ChildOf(s.Context()))
optsLocal = append(optsLocal, ChildOf(s.Context()))
}
opts = append(opts, withContext(ctx))
s := StartSpan(operationName, opts...)
optsLocal = append(optsLocal, withContext(ctx))
s := StartSpan(operationName, optsLocal...)
if span, ok := s.(*span); ok && span.pprofCtxActive != nil {
// If pprof labels were applied for this span, use the derived ctx that
// includes them. Otherwise a child of this span wouldn't be able to
Expand Down
31 changes: 31 additions & 0 deletions ddtrace/tracer/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,37 @@ func TestStartSpanFromContext(t *testing.T) {
assert.Equal("/", got.Resource)
}

func TestStartSpanFromContextRace(t *testing.T) {
_, _, _, stop := startTestTracer(t)
defer stop()

// Start 100 goroutines that create child spans with StartSpanFromContext in parallel,
// with a shared options slice. The child spans should get parented to the correct slices
const contextKey = "key"
const numContexts = 100
options := make([]StartSpanOption, 0, 3)
outputValues := make(chan uint64, numContexts)
var expectedTraceIDs []uint64
for i := 0; i < numContexts; i++ {
parent, childCtx := StartSpanFromContext(context.Background(), "parent")
expectedTraceIDs = append(expectedTraceIDs, parent.Context().TraceID())
go func() {
span, _ := StartSpanFromContext(childCtx, "testoperation", options...)
defer span.Finish()
outputValues <- span.Context().TraceID()
}()
parent.Finish()
}

// collect the outputs
var outputs []uint64
for i := 0; i < numContexts; i++ {
outputs = append(outputs, <-outputValues)
}
assert.Len(t, outputs, numContexts)
assert.ElementsMatch(t, outputs, expectedTraceIDs)
}

func TestStartSpanFromNilContext(t *testing.T) {
_, _, _, stop := startTestTracer(t)
defer stop()
Expand Down

0 comments on commit d7eb0d2

Please sign in to comment.