Skip to content

Commit d92193f

Browse files
authored
fix: data race on sender.enabled in trace writer (#54640)
<!--Please give us some feedback on your experience writing this PR ! https://app.datadoghq.com/forms/43db4c02-6837-400c-8083-692e141b1b88 !--> ### What does this PR do? Fixes a data race on `sender.enabled` in `pkg/trace/writer/sender.go`: `isEnabled()` read/wrote the field with no synchronization. Switched it to `atomic.Bool`. ### Motivation Fix a race, found via a race-detector-enabled build in staging. ### Describe how you validated your changes Added `TestIsEnabledConcurrent` in `pkg/trace/writer/sender_test.go`, which calls `isEnabled()` concurrently; it reproduces the race under `-race` before the fix and is clean after. ### Additional Notes Co-authored-by: pierre.gimalac <pierre.gimalac@datadoghq.com>
1 parent d5d46db commit d92193f

3 files changed

Lines changed: 31 additions & 6 deletions

File tree

pkg/trace/writer/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,5 +57,6 @@ dd_agent_go_test(
5757
"@com_github_stretchr_testify//require",
5858
"@com_github_tinylib_msgp//msgp",
5959
"@org_golang_google_protobuf//proto",
60+
"@org_uber_go_atomic//:atomic",
6061
],
6162
)

pkg/trace/writer/sender.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ type sender struct {
244244
mu sync.RWMutex // guards closed
245245
closed bool // closed reports if the loop is stopped
246246
statsd statsd.ClientInterface
247-
enabled bool // false on inactive MRF senders. True otherwise
247+
enabled *atomic.Bool // false on inactive MRF senders. True otherwise
248248
}
249249

250250
// newSender returns a new sender based on the given config cfg.
@@ -256,7 +256,7 @@ func newSender(cfg *senderConfig, apiKeyManager *apiKeyManager, statsd statsd.Cl
256256
inflight: atomic.NewInt32(0),
257257
maxRetries: int32(cfg.maxRetries),
258258
statsd: statsd,
259-
enabled: true,
259+
enabled: atomic.NewBool(true),
260260
}
261261
for i := 0; i < cfg.maxConns; i++ {
262262
go s.loop()
@@ -432,15 +432,13 @@ func (s *sender) isEnabled() bool {
432432
}
433433
// Endpoint is MRF and MRF is enabled. Figure out if we need to failover APM data
434434
if s.cfg.MRFFailoverAPM() {
435-
if !s.enabled {
435+
if s.enabled.CompareAndSwap(false, true) {
436436
log.Infof("Sender for domain %v has been failed over to, enabling it for MRF.", s.cfg.url)
437-
s.enabled = true
438437
}
439438
return true
440439
}
441440

442-
if s.enabled {
443-
s.enabled = false
441+
if s.enabled.CompareAndSwap(true, false) {
444442
log.Infof("Sender for domain %v was disabled; payloads will be dropped for this domain.", s.cfg.url)
445443
} else {
446444
log.Debugf("Sender for domain %v is disabled; dropping payload for this domain.", s.cfg.url)

pkg/trace/writer/sender_test.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"time"
2020

2121
"github.com/stretchr/testify/assert"
22+
"go.uber.org/atomic"
2223

2324
"github.com/DataDog/datadog-go/v5/statsd"
2425

@@ -457,6 +458,31 @@ func syncTest403ThrottlesRefresh(server *testServer) func(*testing.T) {
457458
}
458459
}
459460

461+
// TestIsEnabledConcurrent verifies that concurrent calls to isEnabled on an MRF
462+
// sender don't race on the enabled field. isEnabled is invoked concurrently in
463+
// production from multiple producers calling sendPayloads on the same *sender.
464+
func TestIsEnabledConcurrent(t *testing.T) {
465+
server := newTestServer()
466+
defer server.Close()
467+
468+
s, err := newTestSender(server.URL)
469+
assert.NoError(t, err)
470+
defer s.Stop()
471+
472+
s.cfg.isMRF = true
473+
failover := atomic.NewBool(false)
474+
s.cfg.MRFFailoverAPM = failover.Load
475+
476+
var wg sync.WaitGroup
477+
for i := range 50 {
478+
wg.Go(func() {
479+
failover.Store(i%2 == 0)
480+
s.isEnabled()
481+
})
482+
}
483+
wg.Wait()
484+
}
485+
460486
func TestPayload(t *testing.T) {
461487
expectBody := bytes.NewBufferString("body")
462488
bodyLength := strconv.Itoa(expectBody.Len())

0 commit comments

Comments
 (0)