From 2f7a709793b4012f66af44f5eb3f0b8495271292 Mon Sep 17 00:00:00 2001
From: Gabriel Aszalos <gabriel.aszalos@gmail.com>
Date: Wed, 15 Jan 2020 14:43:23 +0200
Subject: [PATCH] ddtrace/tracer: fix regression when flushing based on size
 (#570)

During a previous change released as part of v1.20.0 in PR #498, the
flush channel was accidentally made blocking. This would cause a
deadlock on occasion when flushing after the payload size became large.

Tests failed to catch this problem because they were not using the
constructor and instead had their own incorrect constructor. This
change also addresses that problem so that tests would now correctly
catch the issue.
---
 ddtrace/tracer/metrics_test.go | 22 ++--------------------
 ddtrace/tracer/tracer.go       | 12 ++++++++----
 ddtrace/tracer/tracer_test.go  | 12 ++----------
 internal/version/version.go    |  2 +-
 4 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/ddtrace/tracer/metrics_test.go b/ddtrace/tracer/metrics_test.go
index e86145774f..9b5c1e5401 100644
--- a/ddtrace/tracer/metrics_test.go
+++ b/ddtrace/tracer/metrics_test.go
@@ -247,13 +247,7 @@ func (tg *testStatsdClient) Wait(n int, d time.Duration) error {
 
 func TestReportRuntimeMetrics(t *testing.T) {
 	var tg testStatsdClient
-	trc := &tracer{
-		stopped:  make(chan struct{}),
-		exitChan: make(chan struct{}),
-		config: &config{
-			statsd: &tg,
-		},
-	}
+	trc := newUnstartedTracer(withStatsdClient(&tg))
 
 	trc.wg.Add(1)
 	go func() {
@@ -274,19 +268,7 @@ func TestReportRuntimeMetrics(t *testing.T) {
 func TestReportHealthMetrics(t *testing.T) {
 	assert := assert.New(t)
 	var tg testStatsdClient
-	trc := &tracer{
-		config: &config{
-			statsd:    &tg,
-			sampler:   NewAllSampler(),
-			transport: newDummyTransport(),
-		},
-		payload:          newPayload(),
-		flushChan:        make(chan chan<- struct{}),
-		exitChan:         make(chan struct{}),
-		payloadChan:      make(chan []*span, payloadQueueSize),
-		stopped:          make(chan struct{}),
-		prioritySampling: newPrioritySampler(),
-	}
+	trc := newUnstartedTracer(withStatsdClient(&tg))
 	internal.SetGlobalTracer(trc)
 	defer internal.SetGlobalTracer(&internal.NoopTracer{})
 
diff --git a/ddtrace/tracer/tracer.go b/ddtrace/tracer/tracer.go
index ac366e8b59..414fe4a584 100644
--- a/ddtrace/tracer/tracer.go
+++ b/ddtrace/tracer/tracer.go
@@ -130,7 +130,7 @@ func Inject(ctx ddtrace.SpanContext, carrier interface{}) error {
 // payloadQueueSize is the buffer size of the trace channel.
 const payloadQueueSize = 1000
 
-func newTracer(opts ...StartOption) *tracer {
+func newUnstartedTracer(opts ...StartOption) *tracer {
 	c := new(config)
 	defaults(c)
 	for _, fn := range opts {
@@ -157,17 +157,21 @@ func newTracer(opts ...StartOption) *tracer {
 			c.statsd = client
 		}
 	}
-
-	t := &tracer{
+	return &tracer{
 		config:           c,
 		payload:          newPayload(),
-		flushChan:        make(chan chan<- struct{}),
+		flushChan:        make(chan chan<- struct{}, 1),
 		exitChan:         make(chan struct{}),
 		payloadChan:      make(chan []*span, payloadQueueSize),
 		stopped:          make(chan struct{}),
 		prioritySampling: newPrioritySampler(),
 		pid:              strconv.Itoa(os.Getpid()),
 	}
+}
+
+func newTracer(opts ...StartOption) *tracer {
+	t := newUnstartedTracer(opts...)
+	c := t.config
 	t.config.statsd.Incr("datadog.tracer.started", nil, 1)
 	if c.runtimeMetrics {
 		log.Debug("Runtime metrics enabled.")
diff --git a/ddtrace/tracer/tracer_test.go b/ddtrace/tracer/tracer_test.go
index 293fbf12d1..89635110b6 100644
--- a/ddtrace/tracer/tracer_test.go
+++ b/ddtrace/tracer/tracer_test.go
@@ -867,16 +867,8 @@ func TestWorker(t *testing.T) {
 	}
 }
 
-func newTracerChannels() *tracer {
-	return &tracer{
-		payload:     newPayload(),
-		payloadChan: make(chan []*span, payloadQueueSize),
-		flushChan:   make(chan chan<- struct{}, 1),
-	}
-}
-
 func TestPushPayload(t *testing.T) {
-	tracer := newTracerChannels()
+	tracer := newUnstartedTracer()
 	s := newBasicSpan("3MB")
 	s.Meta["key"] = strings.Repeat("X", payloadSizeLimit/2+10)
 
@@ -896,7 +888,7 @@ func TestPushTrace(t *testing.T) {
 
 	tp := new(testLogger)
 	log.UseLogger(tp)
-	tracer := newTracerChannels()
+	tracer := newUnstartedTracer()
 	trace := []*span{
 		&span{
 			Name:     "pylons.request",
diff --git a/internal/version/version.go b/internal/version/version.go
index b048fcddf5..357d5fa112 100644
--- a/internal/version/version.go
+++ b/internal/version/version.go
@@ -8,4 +8,4 @@ package version
 // Tag specifies the current release tag. It needs to be manually
 // updated. A test checks that the value of Tag never points to a
 // git tag that is older than HEAD.
-const Tag = "v1.20.0"
+const Tag = "v1.20.1"