diff --git a/tracing.go b/tracing.go index a5fd70771..d1263c86b 100644 --- a/tracing.go +++ b/tracing.go @@ -60,6 +60,8 @@ type Span struct { //nolint: maligned // prefer readability over optimal memory contexts map[string]Context // collectProfile is a function that collects a profile of the current transaction. May be nil. collectProfile transactionProfiler + // a Once instance to make sure that Finish() is only called once. + finishOnce sync.Once } // TraceParentContext describes the context of a (remote) parent span. @@ -196,31 +198,11 @@ func StartSpan(ctx context.Context, operation string, options ...SpanOption) *Sp // Finish sets the span's end time, unless already set. If the span is the root // of a span tree, Finish sends the span tree to Sentry as a transaction. +// +// The logic is executed at most once per span, so that (incorrectly) calling it twice +// never double sends to Sentry. func (s *Span) Finish() { - // TODO(tracing): maybe make Finish run at most once, such that - // (incorrectly) calling it twice never double sends to Sentry. - - if s.EndTime.IsZero() { - s.EndTime = monotonicTimeSince(s.StartTime) - } - - if !s.Sampled.Bool() { - return - } - event := s.toEvent() - if event == nil { - return - } - - if s.collectProfile != nil { - event.sdkMetaData.transactionProfile = s.collectProfile(s) - } - - // TODO(tracing): add breadcrumbs - // (see https://github.com/getsentry/sentry-python/blob/f6f3525f8812f609/sentry_sdk/tracing.py#L372) - - hub := hubFromContext(s.ctx) - hub.CaptureEvent(event) + s.finishOnce.Do(s.doFinish) } // Context returns the context containing the span. @@ -349,6 +331,31 @@ func (s *Span) SetDynamicSamplingContext(dsc DynamicSamplingContext) { } } +// doFinish runs the actual Span.Finish() logic. +func (s *Span) doFinish() { + if s.EndTime.IsZero() { + s.EndTime = monotonicTimeSince(s.StartTime) + } + + if !s.Sampled.Bool() { + return + } + event := s.toEvent() + if event == nil { + return + } + + if s.collectProfile != nil { + event.sdkMetaData.transactionProfile = s.collectProfile(s) + } + + // TODO(tracing): add breadcrumbs + // (see https://github.com/getsentry/sentry-python/blob/f6f3525f8812f609/sentry_sdk/tracing.py#L372) + + hub := hubFromContext(s.ctx) + hub.CaptureEvent(event) +} + // sentryTracePattern matches either // // TRACE_ID - SPAN_ID diff --git a/tracing_test.go b/tracing_test.go index dc7fd9d75..0e5102e30 100644 --- a/tracing_test.go +++ b/tracing_test.go @@ -980,3 +980,28 @@ func TestAdjustingTransactionSourceBeforeSending(t *testing.T) { }) } } + +// This is a regression test for https://github.com/getsentry/sentry-go/issues/587 +// Without the "spans can be finished only once" fix, this test will fail +// when run with race detection ("-race"). +func TestSpanFinishConcurrentlyWithoutRaces(t *testing.T) { + ctx := NewTestContext(ClientOptions{ + EnableTracing: true, + TracesSampleRate: 1, + }) + transaction := StartTransaction(ctx, "op") + + go func() { + for { + transaction.Finish() + } + }() + + go func() { + for { + transaction.Finish() + } + }() + + time.Sleep(50 * time.Millisecond) +} diff --git a/transport_test.go b/transport_test.go index 7f6db0682..b8ad7f472 100644 --- a/transport_test.go +++ b/transport_test.go @@ -367,7 +367,7 @@ func TestHTTPTransport(t *testing.T) { transportMustFlush := func(t *testing.T, id string) { t.Helper() - ok := transport.Flush(100 * time.Millisecond) + ok := transport.Flush(500 * time.Millisecond) if !ok { t.Fatalf("[CLIENT] {%.4s} Flush() timed out", id) }