From 25059c0591b70c1a3b4c8c1acd679f02b2914d92 Mon Sep 17 00:00:00 2001
From: Tim Raymond <tim@timraymond.com>
Date: Fri, 28 Jul 2017 11:05:25 -0400
Subject: [PATCH] Fix data races in dashboard response construction

Dashboard responses had data races because multiple goroutines were
reading and modifying dashboards before sending them out on the wire.
This patch introduces immutability in the construction of the response,
so that each goroutine is working with its own set of dashboardResponse
structs.
---
 server/cells.go           | 27 +++++++++++++++++----------
 server/dashboards.go      | 31 +++++++++++++++++--------------
 server/dashboards_test.go |  2 +-
 3 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/server/cells.go b/server/cells.go
index 6c7859df4..29c0bd57a 100644
--- a/server/cells.go
+++ b/server/cells.go
@@ -30,27 +30,34 @@ func newCellResponses(dID chronograf.DashboardID, dcells []chronograf.DashboardC
 	base := "/chronograf/v1/dashboards"
 	cells := make([]dashboardCellResponse, len(dcells))
 	for i, cell := range dcells {
-		if len(cell.Queries) == 0 {
-			cell.Queries = make([]chronograf.DashboardQuery, 0)
-		}
+		newCell := chronograf.DashboardCell{}
+
+		newCell.Queries = make([]chronograf.DashboardQuery, len(cell.Queries))
+		copy(newCell.Queries, cell.Queries)
 
 		// ensure x, y, and y2 axes always returned
 		labels := []string{"x", "y", "y2"}
-		if cell.Axes == nil {
-			cell.Axes = make(map[string]chronograf.Axis, len(labels))
-		}
+		newCell.Axes = make(map[string]chronograf.Axis, len(labels))
+
+		newCell.X = cell.X
+		newCell.Y = cell.Y
+		newCell.W = cell.W
+		newCell.H = cell.H
+		newCell.Name = cell.Name
+		newCell.ID = cell.ID
 
 		for _, lbl := range labels {
-			_, found := cell.Axes[lbl]
-			if !found {
-				cell.Axes[lbl] = chronograf.Axis{
+			if axis, found := cell.Axes[lbl]; !found {
+				newCell.Axes[lbl] = chronograf.Axis{
 					Bounds: []string{},
 				}
+			} else {
+				newCell.Axes[lbl] = axis
 			}
 		}
 
 		cells[i] = dashboardCellResponse{
-			DashboardCell: cell,
+			DashboardCell: newCell,
 			Links: dashboardCellLinks{
 				Self: fmt.Sprintf("%s/%d/cells/%s", base, dID, cell.ID),
 			},
diff --git a/server/dashboards.go b/server/dashboards.go
index ee9bb8a60..df307a8b4 100644
--- a/server/dashboards.go
+++ b/server/dashboards.go
@@ -28,20 +28,19 @@ type getDashboardsResponse struct {
 
 func newDashboardResponse(d chronograf.Dashboard) *dashboardResponse {
 	base := "/chronograf/v1/dashboards"
-	DashboardDefaults(&d)
-	AddQueryConfigs(&d)
-	cells := newCellResponses(d.ID, d.Cells)
-	templates := newTemplateResponses(d.ID, d.Templates)
+	dd := AddQueryConfigs(DashboardDefaults(d))
+	cells := newCellResponses(dd.ID, dd.Cells)
+	templates := newTemplateResponses(dd.ID, dd.Templates)
 
 	return &dashboardResponse{
-		ID:        d.ID,
-		Name:      d.Name,
+		ID:        dd.ID,
+		Name:      dd.Name,
 		Cells:     cells,
 		Templates: templates,
 		Links: dashboardLinks{
-			Self:      fmt.Sprintf("%s/%d", base, d.ID),
-			Cells:     fmt.Sprintf("%s/%d/cells", base, d.ID),
-			Templates: fmt.Sprintf("%s/%d/templates", base, d.ID),
+			Self:      fmt.Sprintf("%s/%d", base, dd.ID),
+			Cells:     fmt.Sprintf("%s/%d/cells", base, dd.ID),
+			Templates: fmt.Sprintf("%s/%d/templates", base, dd.ID),
 		},
 	}
 }
@@ -229,24 +228,28 @@ func ValidDashboardRequest(d *chronograf.Dashboard) error {
 			return err
 		}
 	}
-	DashboardDefaults(d)
+	(*d) = DashboardDefaults(*d)
 	return nil
 }
 
 // DashboardDefaults updates the dashboard with the default values
 // if none are specified
-func DashboardDefaults(d *chronograf.Dashboard) {
+func DashboardDefaults(d chronograf.Dashboard) (newDash chronograf.Dashboard) {
+	newDash.Cells = make([]chronograf.DashboardCell, len(d.Cells))
 	for i, c := range d.Cells {
 		CorrectWidthHeight(&c)
-		d.Cells[i] = c
+		newDash.Cells[i] = c
 	}
+	return
 }
 
 // AddQueryConfigs updates all the celsl in the dashboard to have query config
 // objects corresponding to their influxql queries.
-func AddQueryConfigs(d *chronograf.Dashboard) {
+func AddQueryConfigs(d chronograf.Dashboard) (newDash chronograf.Dashboard) {
+	newDash.Cells = make([]chronograf.DashboardCell, len(d.Cells))
 	for i, c := range d.Cells {
 		AddQueryConfig(&c)
-		d.Cells[i] = c
+		newDash.Cells[i] = c
 	}
+	return
 }
diff --git a/server/dashboards_test.go b/server/dashboards_test.go
index d659099dc..f7cbaa747 100644
--- a/server/dashboards_test.go
+++ b/server/dashboards_test.go
@@ -128,7 +128,7 @@ func TestDashboardDefaults(t *testing.T) {
 		},
 	}
 	for _, tt := range tests {
-		if DashboardDefaults(&tt.d); !reflect.DeepEqual(tt.d, tt.want) {
+		if actual := DashboardDefaults(tt.d); !reflect.DeepEqual(actual, tt.want) {
 			t.Errorf("%q. DashboardDefaults() = %v, want %v", tt.name, tt.d, tt.want)
 		}
 	}