cmd/server: remove data race between Listen and Shutdown

Shutdown is now idempotent: it does nothing if the server
was never started or was already stopped.

Listen is guarded by sync.Once so the HTTP server can be
started at most once.

Fixes #667

Author: bmizerany

api.go

@@ -3,10 +3,12 @@ package toxiproxy
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/gorilla/mux"
@@ -34,7 +36,9 @@ type ApiServer struct {
 	Collection *ProxyCollection
 	Metrics    *metricsContainer
 	Logger     *zerolog.Logger
-	http       *http.Server
+
+	shutdownMu sync.Mutex
+	shutdown   func(ctx context.Context) error
 }
 
 const (
@@ -51,41 +55,44 @@ func NewServer(m *metricsContainer, logger zerolog.Logger) *ApiServer {
 }
 
 func (server *ApiServer) Listen(addr string) error {
-	server.Logger.
-		Info().
-		Str("address", addr).
-		Msg("Starting Toxiproxy HTTP server")
-
-	server.http = &http.Server{
+	server.shutdownMu.Lock()
+	if server.shutdown != nil {
+		server.shutdownMu.Unlock()
+		return errors.New("server already started")
+	}
+	s := &http.Server{
 		Addr:         addr,
 		Handler:      server.Routes(),
 		WriteTimeout: wait_timeout,
 		ReadTimeout:  read_timeout,
 		IdleTimeout:  60 * time.Second,
 	}
+	server.shutdown = s.Shutdown
+	server.shutdownMu.Unlock()
 
-	err := server.http.ListenAndServe()
-	if err == http.ErrServerClosed {
-		err = nil
-	}
+	server.Logger.
+		Info().
+		Str("address", addr).
+		Msg("Starting Toxiproxy HTTP server")
 
+	err := s.ListenAndServe()
+	if errors.Is(err, http.ErrServerClosed) {
+		return nil
+	}
 	return err
 }
 
 func (server *ApiServer) Shutdown() error {
-	if server.http == nil {
-		return nil
+	server.shutdownMu.Lock()
+	shutdown := server.shutdown
+	server.shutdownMu.Unlock()
+	if shutdown == nil {
+		return nil // in desire state
 	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), wait_timeout)
 	defer cancel()
-
-	err := server.http.Shutdown(ctx)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return shutdown(ctx)
 }
 
 func (server *ApiServer) Routes() *mux.Router {

api_test.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"net/http"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -1124,6 +1125,25 @@ func TestInvalidStream(t *testing.T) {
 	})
 }
 
+// Issue https://github.com/Shopify/toxiproxy/issues/667
+func TestApiServerShutdownListen(t *testing.T) {
+	s := &toxiproxy.ApiServer{Logger: &zerolog.Logger{}}
+	defer s.Shutdown()
+
+	got := s.Shutdown()
+	if got != nil {
+		t.Fatal("error shutting down unstarted server:", got)
+	}
+
+	got = s.Listen(":0")
+	// Avoid inventing an exported sentinal error just for this test, which is heavy handed
+	// given this error is specific only to starting the server twice, via the Listen method,
+	// and is unlikely to change, we can just do a substring match.
+	if !strings.Contains(got.Error(), "server already started") {
+		t.Fatal("error starting server:", got)
+	}
+}
+
 func AssertToxicExists(
 	t *testing.T,
 	toxics tclient.Toxics,