Fixed race condition in OnEnd and added a unit test (open-telemetry#3951

) * Fixed race condition in OnEnd and added a test * fixed code review comments * fixed lint * Update CHANGELOG.md Co-authored-by: Robert Pająk <[email protected]> * Update sdk/trace/simple_span_processor_test.go Co-authored-by: Tyler Yahn <[email protected]> * Update sdk/trace/simple_span_processor_test.go Co-authored-by: Tyler Yahn <[email protected]> * Update sdk/trace/simple_span_processor_test.go Co-authored-by: Tyler Yahn <[email protected]> * fixed panic check --------- Co-authored-by: Robert Pająk <[email protected]> Co-authored-by: Tyler Yahn <[email protected]>
infracloudio · Apr 21, 2023 · ac6e181 · ac6e181
1 parent 9125ad0
commit ac6e181
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 3 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,6 +31,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - `TracerProvider` allows calling `Tracer()` while it's shutting down.
   It used to deadlock. (#3924)
 - Use the SDK version for the Telemetry SDK resource detector in `go.opentelemetry.io/otel/sdk/resource`. (#3949)
+- Fix a data race in `SpanProcessor` returned by `NewSimpleSpanProcessor` in `go.opentelemetry.io/otel/sdk/trace`. (#3951)
 - Automatically figure out the default aggregation with `aggregation.Default`. (#3967)
 - Changed `WithEndpoint` to require a protocol, and be the same as the environment variable. (#4016)
 

diff --git a/sdk/trace/simple_span_processor.go b/sdk/trace/simple_span_processor.go
@@ -25,7 +25,7 @@ import (
 // simpleSpanProcessor is a SpanProcessor that synchronously sends all
 // completed Spans to a trace.Exporter immediately.
 type simpleSpanProcessor struct {
-	exporterMu sync.RWMutex
+	exporterMu sync.Mutex
 	exporter   SpanExporter
 	stopOnce   sync.Once
 }
@@ -54,8 +54,8 @@ func (ssp *simpleSpanProcessor) OnStart(context.Context, ReadWriteSpan) {}
 
 // OnEnd immediately exports a ReadOnlySpan.
 func (ssp *simpleSpanProcessor) OnEnd(s ReadOnlySpan) {
-	ssp.exporterMu.RLock()
-	defer ssp.exporterMu.RUnlock()
+	ssp.exporterMu.Lock()
+	defer ssp.exporterMu.Unlock()
 
 	if ssp.exporter != nil && s.SpanContext().TraceFlags().IsSampled() {
 		if err := ssp.exporter.ExportSpans(context.Background(), []ReadOnlySpan{s}); err != nil {

diff --git a/sdk/trace/simple_span_processor_test.go b/sdk/trace/simple_span_processor_test.go
@@ -17,9 +17,12 @@ package trace_test
 import (
 	"context"
 	"errors"
+	"sync"
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+
 	sdktrace "go.opentelemetry.io/otel/sdk/trace"
 	"go.opentelemetry.io/otel/trace"
 )
@@ -150,6 +153,32 @@ func TestSimpleSpanProcessorShutdownOnEndConcurrency(t *testing.T) {
 	<-done
 }
 
+func TestSimpleSpanProcessorShutdownOnEndRace(t *testing.T) {
+	exporter := &testExporter{}
+	ssp := sdktrace.NewSimpleSpanProcessor(exporter)
+	tp := basicTracerProvider(t)
+	tp.RegisterSpanProcessor(ssp)
+
+	var wg sync.WaitGroup
+	wg.Add(2)
+
+	span := func(spanName string) {
+		assert.NotPanics(t, func() {
+			defer wg.Done()
+			_, span := tp.Tracer("test").Start(context.Background(), spanName)
+			span.End()
+		})
+	}
+
+	go span("test-span-1")
+	go span("test-span-2")
+
+	wg.Wait()
+
+	assert.NoError(t, ssp.Shutdown(context.Background()))
+	assert.True(t, exporter.shutdown, "exporter shutdown")
+}
+
 func TestSimpleSpanProcessorShutdownHonorsContextDeadline(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond)
 	defer cancel()