internal/server: fix data race on broadcast set

The broadcast set is accessed from within HTTP handlers
whereas at least one of those accesses is a write operation.
Fix the resulting data race by synchronizing access using a
mutex.

Fixes #1.

internal/server/display.go

@@ -5,6 +5,7 @@ import (
 	"log"
 	"net/http"
 	"strconv"
+	"sync"
 
 	"github.com/JamesStewy/sse"
 	"github.com/hashicorp/go-multierror"
@@ -32,9 +33,13 @@ var state = displayState{
 }
 
 // clients holds the clients that are connected to the event handler. It is used to broadcast state changes to all SSE (Server-Sent Events) clients.
+// Must acquire clientsMu before accessing the map.
 // Note: the only reason we use a sse.Client => bool map is that we can call *delete* with the client as key. The actual bool value that is stored holds no significance whatsoever.
 // This is basically a *set* in go.
-var clients map[*sse.Client]bool = make(map[*sse.Client]bool)
+var (
+	clientsMu sync.Mutex
+	clients   map[*sse.Client]bool = make(map[*sse.Client]bool)
+)
 
 // displayStateHandler updated the state based on the query string.
 // For example
@@ -130,8 +135,21 @@ func displayStateHandler(w http.ResponseWriter, r *http.Request) {
 			Data: string(body),
 		}
 
-		// broadcast it to all clients
+		// We'd like to broadcast the message to all clients, as
+		// recorded in the clients map. The message send is a
+		// potentially blocking operation so we don't want to hold the
+		// lock across send operations. Thus, we first build up a list
+		// of clients to send to (while holding clientsMu) and start
+		// sending only after giving up the lock again.
+		var sendingTo []*sse.Client
+		clientsMu.Lock()
 		for client := range clients {
+			sendingTo = append(sendingTo, client)
+		}
+		clientsMu.Unlock()
+
+		// now broadcast it to all clients
+		for _, client := range sendingTo {
 			client.Send(msg)
 		}
 	}
@@ -149,11 +167,18 @@ func displayEventsHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// add client to broadcast set
+	// Add client to broadcast set. Need to serialize access to clients as
+	// we're running concurrently with other requests.
+	clientsMu.Lock()
 	clients[client] = true
+	clientsMu.Unlock()
 
 	// remove client from broadcast set on exit
-	defer delete(clients, client)
+	defer func() {
+		clientsMu.Lock()
+		delete(clients, client)
+		clientsMu.Unlock()
+	}()
 
 	body, err := json.Marshal(state)
 	if err != nil {