From 5a98d277de4ba1e8bf2becb99508dcdf95a26971 Mon Sep 17 00:00:00 2001 From: Nick Ripley Date: Fri, 17 Jun 2022 13:09:43 -0400 Subject: [PATCH] ddtrace/tracer: address race detector warning from TestDefaultHTTPClient Doing a deep comparision (through assert.Equal/assert.NotEqual) of http.Clients in these tests can trigger a race detector warning: === FAIL: ddtrace/tracer TestDefaultHTTPClient/socket (0.00s) ================== WARNING: DATA RACE Read at 0x000001e0a080 by goroutine 123: [ ... elided ... ] reflect.DeepEqual() /usr/local/Cellar/go/1.18.3/libexec/src/reflect/deepequal.go:237 +0x3f9 github.com/stretchr/testify/assert.ObjectsAreEqual() /Users/nick.ripley/go/pkg/mod/github.com/stretchr/testify@v1.7.0/assert/assertions.go:65 +0x184 github.com/stretchr/testify/assert.NotEqual() /Users/nick.ripley/go/pkg/mod/github.com/stretchr/testify@v1.7.0/assert/assertions.go:694 +0x204 gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestDefaultHTTPClient.func2() /Users/nick.ripley/dd/dd-trace-go/ddtrace/tracer/option_test.go:433 +0x3a4 testing.tRunner() /usr/local/Cellar/go/1.18.3/libexec/src/testing/testing.go:1439 +0x213 testing.(*T).Run.func1() /usr/local/Cellar/go/1.18.3/libexec/src/testing/testing.go:1486 +0x47 Previous write at 0x000001e0a080 by goroutine 19: sync/atomic.AddInt32() /usr/local/Cellar/go/1.18.3/libexec/src/runtime/race_amd64.s:279 +0xb sync/atomic.AddInt32() :1 +0x1a net/http.(*Transport).removeIdleConn.func1() /usr/local/Cellar/go/1.18.3/libexec/src/net/http/transport.go:1096 +0x39 runtime.deferreturn() /usr/local/Cellar/go/1.18.3/libexec/src/runtime/panic.go:436 +0x32 net/http.(*persistConn).readLoop.func1() /usr/local/Cellar/go/1.18.3/libexec/src/net/http/transport.go:2062 +0x72 runtime.deferreturn() /usr/local/Cellar/go/1.18.3/libexec/src/runtime/panic.go:436 +0x32 net/http.(*Transport).dialConn.func5() /usr/local/Cellar/go/1.18.3/libexec/src/net/http/transport.go:1750 +0x39 This is a data race but not actually a bug in the tracer library, just a bug in the tests. We can use assert.Same/assert.NotSame to compare the *http.Clients as pointers, since the tests are just checking whether or not the default client is returned. --- ddtrace/tracer/option_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ddtrace/tracer/option_test.go b/ddtrace/tracer/option_test.go index a055c4fb7a..56d147c07c 100644 --- a/ddtrace/tracer/option_test.go +++ b/ddtrace/tracer/option_test.go @@ -416,7 +416,10 @@ func TestTracerOptionsDefaults(t *testing.T) { func TestDefaultHTTPClient(t *testing.T) { t.Run("no-socket", func(t *testing.T) { - assert.Equal(t, defaultHTTPClient(), defaultClient) + // We care that whether clients are different, but doing a deep + // comparison is overkill and can trigger the race detector, so + // just compare the pointers. + assert.Same(t, defaultHTTPClient(), defaultClient) }) t.Run("socket", func(t *testing.T) { @@ -430,7 +433,7 @@ func TestDefaultHTTPClient(t *testing.T) { defer os.RemoveAll(f.Name()) defer func(old string) { defaultSocketAPM = old }(defaultSocketAPM) defaultSocketAPM = f.Name() - assert.NotEqual(t, defaultHTTPClient(), defaultClient) + assert.NotSame(t, defaultHTTPClient(), defaultClient) }) }