DATA RACE : span.context overwritten in span.SetBaggageItem() without any guards

[keer25]

Problem : DATA RACE when setting baggage item and tag concurrently

In Version : 2.25.0

• span.context is read in a lot of places without any mutex, while SetBaggageItem overwrites the context in bagge_setter.go
  span.context = span.context.WithBaggageItem(key, value)

WARNING: DATA RACE

Read at 0x00c0000824e0 by goroutine 45:
...../vendor/github.com/uber/jaeger-client-go.(*Span).isSamplingFinalized()
/home/paasuser/..../vendor/github.com/uber/jaeger-client-go/span.go:446 +0x3e
.../vendor/github.com/uber/jaeger-client-go.(*Span).setTagInternal()
/home/paasuser/..../vendor/github.com/uber/jaeger-client-go/span.go:107 +0x109
.../vendor/github.com/uber/jaeger-client-go.(*Span).SetTag()
/home/paasuser/src/.../vendor/github.com/uber/jaeger-client-go/span.go:99 +0x6c

Previous write at 0x00c0000824e0 by goroutine 46:
.../vendor/github.com/uber/jaeger-client-go.(*baggageSetter).setBaggage()
/home/paasuser/.../vendor/github.com/uber/jaeger-client-go/baggage_setter.go:54 +0x3ca
.../vendor/github.com/uber/jaeger-client-go.(*Span).SetBaggageItem()
/home/paasuser/.../vendor/github.com/uber/jaeger-client-go/tracer.go:470 +0xf3

Proposal

• Don't overwrite context in baggage_setter.go
• Use a sync.Map for storing baggage items, the change can touch 5 files where context.baggage is read and updated.

(OR)

• Use locks in operation where span.context is accessed

Any open questions to address

• I also don't fully understand why a new span context is allocated every time a baggage item is set. Is there any explicit need for that which I am not seeing?
• I will be happy to create a PR for this if the above question is clarified.

[yurishkuro]

Thanks for reporting. I added a unit test (currently disabled) that illustrates the race. We need to ensure that access to s.context is always done under read-lock, at minimum. I would like to see how it would affect a micro-benchmark.

To answer your question, I believe the intention was for Context object to not contain any lock objects, because it was designed to be passed by-value (a decision that I regret in retrospect, since it does not actually reduce memory allocations due to conversion to opentracing.SpanContext interface). Therefore, it must contain immutable baggage and perform copy-on-write (I just added comments about that - cf8927b).