Skip to content

Commit

Permalink
Adding locks where context is accessed
Browse files Browse the repository at this point in the history
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

Per issue #526

Signed-off-by: Keerthana Selvakumar <keerukeerthana8@gmail.com>
  • Loading branch information
keer25 authored and kselvaku committed Aug 25, 2020
1 parent cf8927b commit 255ce9d
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 10 deletions.
20 changes: 11 additions & 9 deletions span.go
Expand Up @@ -120,8 +120,8 @@ func (s *Span) setTagInternal(key string, value interface{}, lock bool) opentrac

// SpanContext returns span context
func (s *Span) SpanContext() SpanContext {
s.Lock()
defer s.Unlock()
s.RLock()
defer s.RUnlock()
return s.context
}

Expand Down Expand Up @@ -345,7 +345,7 @@ func (s *Span) FinishWithOptions(options opentracing.FinishOptions) {
decision := s.tracer.sampler.OnFinishSpan(s)
s.applySamplingDecision(decision, true)
}
if s.context.IsSampled() {
if s.SpanContext().IsSampled() {
s.Lock()
s.fixLogsIfDropped()
if len(options.LogRecords) > 0 || len(options.BulkLogData) > 0 {
Expand All @@ -366,8 +366,8 @@ func (s *Span) FinishWithOptions(options opentracing.FinishOptions) {

// Context implements opentracing.Span API
func (s *Span) Context() opentracing.SpanContext {
s.Lock()
defer s.Unlock()
s.RLock()
defer s.RUnlock()
return s.context
}

Expand Down Expand Up @@ -427,10 +427,10 @@ func (s *Span) serviceName() string {

func (s *Span) applySamplingDecision(decision SamplingDecision, lock bool) {
if !decision.Retryable {
s.context.samplingState.setFinal()
s.SpanContext().samplingState.setFinal()
}
if decision.Sample {
s.context.samplingState.setSampled()
s.SpanContext().samplingState.setSampled()
if len(decision.Tags) > 0 {
if lock {
s.Lock()
Expand All @@ -445,12 +445,12 @@ func (s *Span) applySamplingDecision(decision SamplingDecision, lock bool) {

// Span can be written to if it is sampled or the sampling decision has not been finalized.
func (s *Span) isWriteable() bool {
state := s.context.samplingState
state := s.SpanContext().samplingState
return !state.isFinal() || state.isSampled()
}

func (s *Span) isSamplingFinalized() bool {
return s.context.samplingState.isFinal()
return s.SpanContext().samplingState.isFinal()
}

// setSamplingPriority returns true if the flag was updated successfully, false otherwise.
Expand All @@ -469,6 +469,8 @@ func setSamplingPriority(s *Span, value interface{}) bool {
if !ok {
return false
}
s.RLock()
defer s.RUnlock()
if val == 0 {
s.context.samplingState.unsetSampled()
s.context.samplingState.setFinal()
Expand Down
1 change: 0 additions & 1 deletion span_test.go
Expand Up @@ -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()

Expand Down

0 comments on commit 255ce9d

Please sign in to comment.