apm: fix race in DefaultTracer (#1248)

Fix a race in DefaultTracer which could lead
to two tracers being created, one of which
would be closed. We must hold the write lock
while creating the default tracer, checking
again that it was not set after releasing the
read lock.

tracer.go

@@ -50,30 +50,41 @@ var (
 )
 
 // DefaultTracer returns the default global Tracer, set the first time the
-// function is called. It is configured via environment variables.
+// function is called, or after calling SetDefaultTracer(nil).
 //
-// This will always be initialized to a non-nil value. If any of the
-// environment variables are invalid, the corresponding errors will be logged
-// to stderr and the default values will be used instead.
+// The default tracer is configured via environment variables, and will always
+// be non-nil. If any of the environment variables are invalid, the
+// corresponding errors will be logged to stderr and the default values will be
+// used instead.
 func DefaultTracer() *Tracer {
 	tracerMu.RLock()
-	tracer := defaultTracer
-	tracerMu.RUnlock()
-	if tracer != nil {
+	if defaultTracer != nil {
+		tracer := defaultTracer
+		tracerMu.RUnlock()
 		return tracer
 	}
+	tracerMu.RUnlock()
+
+	tracerMu.Lock()
+	defer tracerMu.Unlock()
+	if defaultTracer != nil {
+		return defaultTracer
+	}
 
 	var opts TracerOptions
 	opts.initDefaults(true)
-	tracer = newTracer(opts)
-	SetDefaultTracer(tracer)
-	return tracer
+	defaultTracer = newTracer(opts)
+	return defaultTracer
 }
 
-// SetDefaultTracer sets the tracer returned by DefaultTracer(). If another
-// tracer has already been initialized, it is closed. Any queued events are not
-// flushed; it is the responsibility of the caller to call
-// DefaultTracer().Flush().
+// SetDefaultTracer sets the tracer returned by DefaultTracer.
+//
+// If a default tracer has already been initialized, it is closed.
+// Any queued events are not flushed; it is the responsibility of the
+// caller to call the default tracer's Flush method first, if needed.
+//
+// Calling SetDefaultTracer(nil) will clear the default tracer,
+// causing DefaultTracer to initialize a new default tracer.
 func SetDefaultTracer(t *Tracer) {
 	tracerMu.Lock()
 	defer tracerMu.Unlock()

tracer_test.go

@@ -48,6 +48,24 @@ import (
 	"go.elastic.co/apm/v2/transport/transporttest"
 )
 
+func TestDefaultTracer(t *testing.T) {
+	defer apm.SetDefaultTracer(nil)
+
+	// Call DefaultTracer concurrently to ensure there are
+	// no races in creating the default tracer.
+	tracers := make(chan *apm.Tracer, 1000)
+	for i := 0; i < cap(tracers); i++ {
+		go func() {
+			tracers <- apm.DefaultTracer()
+		}()
+	}
+
+	tracer0 := <-tracers
+	for i := 1; i < cap(tracers); i++ {
+		assert.Same(t, tracer0, <-tracers)
+	}
+}
+
 func TestTracerStats(t *testing.T) {
 	tracer := apmtest.NewDiscardTracer()
 	defer tracer.Close()