Lock RemotelyControlledSampler.sampler on callbacks #543

hummerd · 2020-10-08T11:43:30Z

We've detected some race condition. In case you are using RemotelyControlledSampler it will call to Update method for sampler in it's background settings-polling routine. In the same time IsSampled (or any other exposed method of sampler) can be called from another goroutine and try to access sampler's data.

2020-10-06_09:52:19.35460 ==================
2020-10-06_09:52:19.35462 WARNING: DATA RACE
2020-10-06_09:52:19.35462 Write at 0x00c000073150 by goroutine 14:
2020-10-06_09:52:19.35463   github.com/uber/jaeger-client-go.(*ProbabilisticSampler).init()
2020-10-06_09:52:19.35463       /project/build/vendor/github.com/uber/jaeger-client-go/sampler.go:130 +0xe4
2020-10-06_09:52:19.35463   github.com/uber/jaeger-client-go.(*ProbabilisticSampler).Update()
2020-10-06_09:52:19.35463       /project/build/vendor/github.com/uber/jaeger-client-go/sampler.go:166 +0x104
2020-10-06_09:52:19.35464   github.com/uber/jaeger-client-go.(*ProbabilisticSamplerUpdater).Update()
2020-10-06_09:52:19.35464       /project/build/vendor/github.com/uber/jaeger-client-go/sampler_remote.go:225 +0xe4
2020-10-06_09:52:19.35464   github.com/uber/jaeger-client-go.(*RemotelyControlledSampler).updateSamplerViaUpdaters()
2020-10-06_09:52:19.35465       /project/build/vendor/github.com/uber/jaeger-client-go/sampler_remote.go:198 +0x129
2020-10-06_09:52:19.35465   github.com/uber/jaeger-client-go.(*RemotelyControlledSampler).UpdateSampler()
2020-10-06_09:52:19.35465       /project/build/vendor/github.com/uber/jaeger-client-go/sampler_remote.go:187 +0x59b
2020-10-06_09:52:19.35465   github.com/uber/jaeger-client-go.(*RemotelyControlledSampler).pollControllerWithTicker()
2020-10-06_09:52:19.35466       /project/build/vendor/github.com/uber/jaeger-client-go/sampler_remote.go:146 +0x57
2020-10-06_09:52:19.35466   github.com/uber/jaeger-client-go.(*RemotelyControlledSampler).pollController()
2020-10-06_09:52:19.35467       /project/build/vendor/github.com/uber/jaeger-client-go/sampler_remote.go:139 +0xa4
2020-10-06_09:52:19.35467 
2020-10-06_09:52:19.35467 Previous read at 0x00c000073150 by goroutine 713:
2020-10-06_09:52:19.35467   github.com/uber/jaeger-client-go.(*ProbabilisticSampler).IsSampled()
2020-10-06_09:52:19.35468       /project/build/vendor/github.com/uber/jaeger-client-go/sampler.go:145 +0x50
2020-10-06_09:52:19.35468   github.com/uber/jaeger-client-go.(*ProbabilisticSampler).IsSampled-fm()
2020-10-06_09:52:19.35468       /project/build/vendor/github.com/uber/jaeger-client-go/sampler.go:144 +0x26
2020-10-06_09:52:19.35469   github.com/uber/jaeger-client-go.(*legacySamplerV1Base).OnCreateSpan()
2020-10-06_09:52:19.35469       /project/build/vendor/github.com/uber/jaeger-client-go/sampler_v2.go:76 +0xd2
2020-10-06_09:52:19.35470   github.com/uber/jaeger-client-go.(*ProbabilisticSampler).OnCreateSpan()

Signed-off-by: Dima Kozlov [email protected]

joe-elliott

Thank you for this contribution and noticing this issue. However, I think this PR can be simplified by using the already existing locking in RemotelyControlledSampler in the OnCreateSpan method:

https://github.com/jaegertracing/jaeger-client-go/blob/master/sampler_remote.go#L98

This method is ultimately what calls IsSampled on the nested sampler.

The lock is already being used here:
https://github.com/jaegertracing/jaeger-client-go/blob/master/sampler_remote.go#L183
to protect the Update method which you noticed races with IsSampled.

Also, if you could add data race testing similar to the testing for the span it would help confirm the fix worked:

https://github.com/jaegertracing/jaeger-client-go/blob/master/span_test.go#L365

yurishkuro · 2020-10-08T20:35:08Z

We have another locking related PR #528

Signed-off-by: Dima Kozlov <[email protected]>

codecov · 2020-10-08T21:51:24Z

Codecov Report

Merging #543 into master will increase coverage by 0.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
+ Coverage   88.19%   88.52%   +0.33%     
==========================================
  Files          61       61              
  Lines        3277     3285       +8     
==========================================
+ Hits         2890     2908      +18     
+ Misses        260      251       -9     
+ Partials      127      126       -1

Impacted Files	Coverage Δ
sampler_remote.go	`87.20% <100.00%> (+4.29%)`	⬆️
utils/reconnecting_udp_conn.go	`91.54% <0.00%> (+2.81%)`	⬆️
sampler_v2.go	`50.00% <0.00%> (+22.22%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3d00f4...6e42cb9. Read the comment docs.

hummerd · 2020-10-08T21:54:24Z

Thanks for viewing PR! I've changed code with use of RemotelyControlledSampler.Lock()
I will make test with next commit in this PR.

hummerd · 2020-10-08T22:46:40Z

Added TestRemotelyControlledSampler_updateRace also

sampler_remote_test.go

Signed-off-by: Dima Kozlov <[email protected]>

joe-elliott

Thanks for the contribution! Looks good to me. Benchmarks below:

master

/jaeger-client-go (master)$ ./benchmark.bin -test.bench=BenchmarkTracer -test.run=BenchmarkTracer
goos: linux
goarch: amd64
pkg: github.com/uber/jaeger-client-go
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 6488808	       188 ns/op
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 5012667	       274 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 3443355	       379 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 3072049	       386 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 1713846	       644 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 4692938	       299 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	  702218	      1450 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 2399791	       710 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8       	  735396	      1431 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8        	  989468	      1269 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8      	  952508	      1267 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8       	  897516	      1413 ns/op
PASS

hummerd:master

/jaeger-client-go (hummerd-master)$ ./benchmark.bin -test.bench=BenchmarkTracer -test.run=BenchmarkTracer
goos: linux
goarch: amd64
pkg: github.com/uber/jaeger-client-go
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 6395553	       201 ns/op
BenchmarkTracer/sampler=NeverSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 4526431	       253 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	 3410322	       347 ns/op
BenchmarkTracer/sampler=AlwaysSample/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 2723958	       569 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8         	 1444323	       842 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8          	 3284641	       437 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8        	  769690	      1517 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleNoLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8         	 2167453	       644 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8       	  781092	      1469 ns/op
BenchmarkTracer/sampler=AdaptiveNeverSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8        	  749485	      1554 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=false/tags=5/ops=10/cpus-8      	 1189791	      1192 ns/op
BenchmarkTracer/sampler=AdaptiveAlwaysSampleWithLateBinding/remoteSampler=false/children=true/tags=5/ops=10/cpus-8       	  855861	      1382 ns/op
PASS

yurishkuro

Thanks!

joe-elliott suggested changes Oct 8, 2020

View reviewed changes

Make access to RemotelyControlledSampler.sampler serialized

5ef1ece

Signed-off-by: Dima Kozlov <[email protected]>

hummerd changed the title ~~Make samplers safe for concurrent use~~ Make access RemotelyControlledSampler.sampler serialized Oct 8, 2020

joe-elliott reviewed Oct 9, 2020

View reviewed changes

sampler_remote_test.go Outdated Show resolved Hide resolved

Add test for race detection in RemotelyControlledSampler

6e42cb9

Signed-off-by: Dima Kozlov <[email protected]>

joe-elliott approved these changes Oct 9, 2020

View reviewed changes

yurishkuro approved these changes Oct 9, 2020

View reviewed changes

yurishkuro changed the title ~~Make access RemotelyControlledSampler.sampler serialized~~ Lock RemotelyControlledSampler.sampler on callbacks Oct 9, 2020

yurishkuro merged commit 9a2c34e into jaegertracing:master Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock RemotelyControlledSampler.sampler on callbacks #543

Lock RemotelyControlledSampler.sampler on callbacks #543

hummerd commented Oct 8, 2020 •

edited

Loading

joe-elliott left a comment •

edited

Loading

yurishkuro commented Oct 8, 2020

codecov bot commented Oct 8, 2020 •

edited

Loading

hummerd commented Oct 8, 2020

hummerd commented Oct 8, 2020

joe-elliott left a comment •

edited

Loading

yurishkuro left a comment

Lock RemotelyControlledSampler.sampler on callbacks #543

Lock RemotelyControlledSampler.sampler on callbacks #543

Conversation

hummerd commented Oct 8, 2020 • edited Loading

joe-elliott left a comment • edited Loading

Choose a reason for hiding this comment

yurishkuro commented Oct 8, 2020

codecov bot commented Oct 8, 2020 • edited Loading

Codecov Report

hummerd commented Oct 8, 2020

hummerd commented Oct 8, 2020

joe-elliott left a comment • edited Loading

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

hummerd commented Oct 8, 2020 •

edited

Loading

joe-elliott left a comment •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading

joe-elliott left a comment •

edited

Loading