Skip to content

Commit

Permalink
tracing: fix child span optimization
Browse files Browse the repository at this point in the history
When given a context with a non-recording (but real) Span, we would
return the incoming context. This would lead to an extra premature
call to `Finish()` and subsequent use-after-Finish of the Span, which
can blow up and/or hang, most of the time within net/trace code.

Prior to this commit, the crash reproduced readily (within seconds) via

```
make stress PKG=./pkg/sql TESTS=TestTrace
```

and I had no issues for ~10 minutes with this commit.

Fixes #57875.

Release note: None
  • Loading branch information
tbg committed Dec 15, 2020
1 parent bf50af3 commit 947bd9c
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 25 deletions.
17 changes: 0 additions & 17 deletions pkg/util/tracing/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
package tracing

import (
"context"
"regexp"
"strings"
"testing"
Expand Down Expand Up @@ -182,19 +181,3 @@ Span grandchild:
`
require.Equal(t, exp, recToStrippedString(childRec))
}

func TestChildSpan(t *testing.T) {
tr := NewTracer()
// Set up non-recording span.
sp := tr.StartSpan("foo", WithForceRealSpan())
ctx := ContextWithSpan(context.Background(), sp)
// Since the parent span was not recording, we would expect the
// noop span back. However - we don't, we get our inputs instead.
// This is a performance optimization; there is a to-do in
// childSpan asking for its removal, but for now it's here to stay.
{
newCtx, newSp := ChildSpan(ctx, "foo")
require.Equal(t, ctx, newCtx)
require.Equal(t, sp, newSp)
}
}
15 changes: 7 additions & 8 deletions pkg/util/tracing/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -559,14 +559,13 @@ func childSpan(ctx context.Context, opName string, remote bool) (context.Context
opts.RemoteParent = sp.Meta()
}
newSpan := tr.startSpanGeneric(opName, opts)
if newSpan.isNoop() {
// Optimization: if we end up with a noop, return the inputs
// to avoid ContextWithSpan call below.
//
// TODO(tbg): this is unsound. We are returning the incoming
// context which may have a Span that could later start recording.
// So in effect that span may capture parts of two goroutines
// accidentally.
if sp.isNoop() && newSpan.isNoop() {
// Optimization: if we started and end up with a noop, we can return
// the original context to save on the ContextWithSpan alloc. Note
// that it is important that the incoming span was the noop span. If
// it was a real, non-recording span, it might later start recording.
// Besides, the caller expects to get their own span, and will
// .Finish() it, leading to an extra, premature call to Finish().
return ctx, sp
}
return ContextWithSpan(ctx, newSpan), newSpan
Expand Down

0 comments on commit 947bd9c

Please sign in to comment.