From f34b223dd5db8276499a5b4c1113e0dae39e473f Mon Sep 17 00:00:00 2001
From: Will Hughes <willhughes01@gmail.com>
Date: Wed, 6 Dec 2017 11:13:10 -0800
Subject: [PATCH] Copy Histogram Buckets before sorting (#68)

Summary: go's sort.Sort function will occasionally modify the data
that is passed into it even if that data is already sorted.
(https://play.golang.org/p/QJ8kiJzzVG)

For tally, this means that if a bucket slice is passed into tally, and
tally calls sort.Sort on that slice, the slice will get modified inline
(even if the list is already sorted).

In practice, this means that if a user is using a single slice of buckets
to create multiple tally histograms, there is a race condition where the
shared bucket object could be modified mid-histogram creation.

This PR creates copies of the []float64 and []time.Duration slices before
sorting them, guaranteeing that we won't have a race condition.
---
 histogram.go      | 25 ++++++++++++++++++++-----
 histogram_test.go | 28 ++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/histogram.go b/histogram.go
index 1ad45ee1..7f876f05 100644
--- a/histogram.go
+++ b/histogram.go
@@ -135,12 +135,9 @@ func BucketPairs(buckets Buckets) []BucketPair {
 		}
 	}
 
-	// Sort before iterating to create pairs
-	sort.Sort(buckets)
-
 	var (
-		asValueBuckets    = buckets.AsValues()
-		asDurationBuckets = buckets.AsDurations()
+		asValueBuckets    = copyAndSortValues(buckets.AsValues())
+		asDurationBuckets = copyAndSortDurations(buckets.AsDurations())
 		pairs             = make([]BucketPair, 0, buckets.Len()+2)
 	)
 
@@ -175,6 +172,24 @@ func BucketPairs(buckets Buckets) []BucketPair {
 	return pairs
 }
 
+func copyAndSortValues(values []float64) []float64 {
+	valuesCopy := make([]float64, len(values))
+	for i := range values {
+		valuesCopy[i] = values[i]
+	}
+	sort.Sort(ValueBuckets(valuesCopy))
+	return valuesCopy
+}
+
+func copyAndSortDurations(durations []time.Duration) []time.Duration {
+	durationsCopy := make([]time.Duration, len(durations))
+	for i := range durations {
+		durationsCopy[i] = durations[i]
+	}
+	sort.Sort(DurationBuckets(durationsCopy))
+	return durationsCopy
+}
+
 type bucketPair struct {
 	lowerBoundValue    float64
 	upperBoundValue    float64
diff --git a/histogram_test.go b/histogram_test.go
index 2a4a4b35..c1eb57b1 100644
--- a/histogram_test.go
+++ b/histogram_test.go
@@ -165,3 +165,31 @@ func TestMustMakeExponentialDurationBucketsPanicsOnBadFactor(t *testing.T) {
 		MustMakeExponentialDurationBuckets(2*time.Second, 1, 2)
 	})
 }
+
+func TestBucketPairsNoRaceWhenSorted(t *testing.T) {
+	buckets := DurationBuckets{}
+	for i := 0; i < 99; i++ {
+		buckets = append(buckets, time.Duration(i) * time.Second)
+	}
+	newPair := func() {
+		pairs := BucketPairs(buckets)
+		require.Equal(t, 100, len(pairs))
+	}
+	for i := 0; i < 10; i++ {
+		go newPair()
+	}
+}
+
+func TestBucketPairsNoRaceWhenUnsorted(t *testing.T) {
+	buckets := DurationBuckets{}
+	for i := 100; i > 1; i-- {
+		buckets = append(buckets, time.Duration(i) * time.Second)
+	}
+	newPair := func() {
+		pairs := BucketPairs(buckets)
+		require.Equal(t, 100, len(pairs))
+	}
+	for i := 0; i < 10; i++ {
+		go newPair()
+	}
+}