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.

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
+
+	initOnce sync.Once // guards shutdown
+	shutdown func(ctx context.Context) error
 }
 
 const (
@@ -50,42 +54,45 @@ 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")
+var errServerStarted = errors.New("server already started")
 
-	server.http = &http.Server{
-		Addr:         addr,
-		Handler:      server.Routes(),
-		WriteTimeout: wait_timeout,
-		ReadTimeout:  read_timeout,
-		IdleTimeout:  60 * time.Second,
+func (server *ApiServer) Listen(addr string) error {
+	var s *http.Server
+	server.initOnce.Do(func() {
+		server.Logger.
+			Info().
+			Str("address", addr).
+			Msg("Starting Toxiproxy HTTP server")
+
+		s = &http.Server{
+			Addr:         addr,
+			Handler:      server.Routes(),
+			WriteTimeout: wait_timeout,
+			ReadTimeout:  read_timeout,
+			IdleTimeout:  60 * time.Second,
+		}
+	})
+	if s == nil {
+		return errServerStarted
 	}
 
-	err := server.http.ListenAndServe()
-	if err == http.ErrServerClosed {
-		err = nil
+	server.shutdown = s.Shutdown
+	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.initOnce.Do(func() {})
+	if server.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 server.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,