From 665f930c5bf66f7ffffa8034fd4f2ce2e05947b3 Mon Sep 17 00:00:00 2001
From: Keerthana Selvakumar <keerukeerthana8@gmail.com>
Date: Wed, 26 Aug 2020 00:47:27 +0530
Subject: [PATCH] Adding locks where context is accessed

This commit addresses data race issues where span.context is accessed without locks in methods which can be called concurrently with method setBaggageItem which modifies context

Resolves issue #526

Signed-off-by: Keerthana Selvakumar <keerukeerthana8@gmail.com>
---
 span.go      | 24 ++++++++----------------
 span_test.go |  1 -
 tracer.go    |  2 +-
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/span.go b/span.go
index efc37067..c8199ea2 100644
--- a/span.go
+++ b/span.go
@@ -85,11 +85,11 @@ func NewTag(key string, value interface{}) Tag {
 func (s *Span) SetOperationName(operationName string) opentracing.Span {
 	s.Lock()
 	s.operationName = operationName
-	s.Unlock()
 	if !s.isSamplingFinalized() {
 		decision := s.tracer.sampler.OnSetOperationName(s, operationName)
-		s.applySamplingDecision(decision, true)
+		s.applySamplingDecision(decision)
 	}
+	s.Unlock()
 	s.observer.OnSetOperationName(operationName)
 	return s
 }
@@ -101,18 +101,16 @@ func (s *Span) SetTag(key string, value interface{}) opentracing.Span {
 
 func (s *Span) setTagInternal(key string, value interface{}, lock bool) opentracing.Span {
 	s.observer.OnSetTag(key, value)
+	s.Lock()
+	defer s.Unlock()
 	if key == string(ext.SamplingPriority) && !setSamplingPriority(s, value) {
 		return s
 	}
 	if !s.isSamplingFinalized() {
 		decision := s.tracer.sampler.OnSetTag(s, key, value)
-		s.applySamplingDecision(decision, lock)
+		s.applySamplingDecision(decision)
 	}
 	if s.isWriteable() {
-		if lock {
-			s.Lock()
-			defer s.Unlock()
-		}
 		s.appendTagNoLocking(key, value)
 	}
 	return s
@@ -340,13 +338,11 @@ func (s *Span) FinishWithOptions(options opentracing.FinishOptions) {
 	s.observer.OnFinish(options)
 	s.Lock()
 	s.duration = options.FinishTime.Sub(s.startTime)
-	s.Unlock()
 	if !s.isSamplingFinalized() {
 		decision := s.tracer.sampler.OnFinishSpan(s)
-		s.applySamplingDecision(decision, true)
+		s.applySamplingDecision(decision)
 	}
 	if s.context.IsSampled() {
-		s.Lock()
 		s.fixLogsIfDropped()
 		if len(options.LogRecords) > 0 || len(options.BulkLogData) > 0 {
 			// Note: bulk logs are not subject to maxLogsPerSpan limit
@@ -357,8 +353,8 @@ func (s *Span) FinishWithOptions(options opentracing.FinishOptions) {
 				s.logs = append(s.logs, ld.ToLogRecord())
 			}
 		}
-		s.Unlock()
 	}
+	s.Unlock()
 	// call reportSpan even for non-sampled traces, to return span to the pool
 	// and update metrics counter
 	s.tracer.reportSpan(s)
@@ -425,17 +421,13 @@ func (s *Span) serviceName() string {
 	return s.tracer.serviceName
 }
 
-func (s *Span) applySamplingDecision(decision SamplingDecision, lock bool) {
+func (s *Span) applySamplingDecision(decision SamplingDecision) {
 	if !decision.Retryable {
 		s.context.samplingState.setFinal()
 	}
 	if decision.Sample {
 		s.context.samplingState.setSampled()
 		if len(decision.Tags) > 0 {
-			if lock {
-				s.Lock()
-				defer s.Unlock()
-			}
 			for _, tag := range decision.Tags {
 				s.appendTagNoLocking(tag.key, tag.value)
 			}
diff --git a/span_test.go b/span_test.go
index 18083039..cced143d 100644
--- a/span_test.go
+++ b/span_test.go
@@ -363,7 +363,6 @@ func TestSpan_References(t *testing.T) {
 }
 
 func TestSpanContextRaces(t *testing.T) {
-	t.Skip("Skipped: test will panic with -race, see https://github.com/jaegertracing/jaeger-client-go/issues/526")
 	tracer, closer := NewTracer("test", NewConstSampler(true), NewNullReporter())
 	defer closer.Close()
 
diff --git a/tracer.go b/tracer.go
index 477c6eae..bab86264 100644
--- a/tracer.go
+++ b/tracer.go
@@ -322,7 +322,7 @@ func (t *Tracer) startSpanWithOptions(
 
 	if !sp.isSamplingFinalized() {
 		decision := t.sampler.OnCreateSpan(sp)
-		sp.applySamplingDecision(decision, false)
+		sp.applySamplingDecision(decision)
 	}
 	sp.observer = t.observer.OnStartSpan(sp, operationName, options)