fix(http-server): remove data race between Start and Shutdown

.github/workflows/go.yml

@@ -14,8 +14,11 @@ jobs:
     strategy:
       matrix:
         go-version:
-          - 1.18
-          - 1.19
+          - "1.23"
+          - "1.24"
+          - "1.25"
+          - "1.26"
+          - "1.27"
 
     steps:
       - uses: actions/checkout@v3

.scripts/test.sh

@@ -7,7 +7,7 @@ if [[ -f coverage.txt ]]; then
 fi
 
 for d in $(go list ./... | grep -v ditest); do
-  go test -coverprofile=profile.out -coverpkg=./... -covermode=atomic "$d"
+  go test -race -coverprofile=profile.out -coverpkg=./... -covermode=atomic "$d"
   if [[ -f profile.out ]]; then
     cat profile.out >>coverage.txt
     rm profile.out

internal/command/http_server.go

@@ -6,8 +6,10 @@ package command
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net"
+	"net/http"
 	"time"
 
 	"github.com/gozix/di"
@@ -25,6 +27,13 @@ const (
 	tagServerName = "echo.server_name"
 )
 
+// server is a resolved http server, ready to be run.
+type server struct {
+	echo *echo.Echo
+	addr string
+	log  *zap.Logger
+}
+
 // NewHTTPServer is command constructor.
 func NewHTTPServer(ctn di.Container) *cobra.Command {
 	return &cobra.Command{
@@ -37,49 +46,39 @@ func NewHTTPServer(ctn di.Container) *cobra.Command {
 			}
 
 			return ctn.Call(func(serverNames []string, cfg *viper.Viper, logger *zap.Logger) error {
-				var wg, ctx = errgroup.WithContext(cmd.Context())
-
+				// resolve every server before starting any of them: the container caches shared
+				// definitions with a check-then-set, so resolving one definition from two
+				// goroutines closes its ready channel twice, and a failure here must not leave
+				// an already started server behind
+				var servers = make([]server, 0, len(serverNames))
 				for _, srvName := range serverNames {
-					var srvName = srvName
-					wg.Go(func() (err error) {
-						var e *echo.Echo
-						if err = ctn.Resolve(&e, di.WithTags(tagEcho+"."+srvName)); err != nil {
-							return err
-						}
-
-						var (
-							subCfg = cfg.Sub("echo." + srvName)
-							addr   = net.JoinHostPort(
-								subCfg.GetString("host"),
-								subCfg.GetString("port"),
-							)
-							log = logger.With(
-								zap.String("name", srvName),
-								zap.String("addr", addr),
-							)
+					var e *echo.Echo
+					if err := ctn.Resolve(&e, di.WithTags(tagEcho+"."+srvName)); err != nil {
+						return err
+					}
+
+					var (
+						subCfg = cfg.Sub("echo." + srvName)
+						addr   = net.JoinHostPort(
+							subCfg.GetString("host"),
+							subCfg.GetString("port"),
 						)
+					)
+
+					servers = append(servers, server{
+						echo: e,
+						addr: addr,
+						log: logger.With(
+							zap.String("name", srvName),
+							zap.String("addr", addr),
+						),
+					})
+				}
 
-						log.Info("Starting HTTP server")
-
-						go func() {
-							if err = e.Start(addr); err != nil {
-								log.Info("Gracefully shutting down the HTTP server")
-							}
-						}()
-
-						log.Info("HTTP server started")
-
-						// wait, context cancellation
-						<-ctx.Done()
-
-						// graceful shutdown
-						var timeout = 10 * time.Second
-						log.Info("Stopping HTTP server", zap.Duration("timeout", timeout))
-
-						var timeoutContext, cancel = context.WithTimeout(context.Background(), timeout)
-						defer cancel()
-
-						return e.Shutdown(timeoutContext)
+				var wg, ctx = errgroup.WithContext(cmd.Context())
+				for _, srv := range servers {
+					wg.Go(func() error {
+						return runServer(ctx, srv.echo, srv.addr, srv.log)
 					})
 				}
 
@@ -88,3 +87,57 @@ func NewHTTPServer(ctn di.Container) *cobra.Command {
 		},
 	}
 }
+
+// runServer starts the server and gracefully shuts it down on context cancellation.
+//
+// The start result travels through a channel instead of a shared variable, so the goroutine running
+// the server and the one shutting it down never write to the same memory.
+func runServer(ctx context.Context, e *echo.Echo, addr string, log *zap.Logger) error {
+	log.Info("Starting HTTP server")
+
+	var errCh = make(chan error, 1)
+	go func() {
+		var err = e.Start(addr)
+		if err != nil {
+			log.Info("Gracefully shutting down the HTTP server")
+		}
+
+		errCh <- err
+	}()
+
+	log.Info("HTTP server started")
+
+	// wait, either an early start failure or a context cancellation
+	select {
+	case err := <-errCh:
+		// a closed server is an expected outcome, not a failure
+		if errors.Is(err, http.ErrServerClosed) {
+			return nil
+		}
+
+		return err
+	case <-ctx.Done():
+	}
+
+	// graceful shutdown
+	var timeout = 10 * time.Second
+	log.Info("Stopping HTTP server", zap.Duration("timeout", timeout))
+
+	var timeoutContext, cancel = context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+
+	// a failed shutdown is reported as is, without joining the serving goroutine below: the
+	// listener may still be open, and echo.Shutdown gives up on the plain server whenever the
+	// TLS one fails, so the join would block until the process is killed
+	if err := e.Shutdown(timeoutContext); err != nil {
+		return err
+	}
+
+	// join the serving goroutine, so it neither logs nor leaks after the command returns.
+	// Shutdown closed the listener, so Start has already returned by now.
+	if err := <-errCh; !errors.Is(err, http.ErrServerClosed) {
+		return err
+	}
+
+	return nil
+}

internal/command/http_server_test.go

@@ -0,0 +1,121 @@
+// Copyright 2018 Sergey Novichkov. All rights reserved.
+// For the full copyright and license information, please view the LICENSE
+// file that was distributed with this source code.
+
+package command
+
+import (
+	"context"
+	"net"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/labstack/echo/v4"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+// TestRunServerGracefulShutdown ensures a context cancellation stops the server without a data race
+// and without reporting the shutdown itself as an error. Run it with -race.
+func TestRunServerGracefulShutdown(t *testing.T) {
+	var (
+		e           = newTestEcho()
+		ctx, cancel = context.WithCancel(context.Background())
+		done        = make(chan error, 1)
+	)
+
+	t.Cleanup(cancel)
+
+	go func() {
+		done <- runServer(ctx, e, "127.0.0.1:0", zap.NewNop())
+	}()
+
+	var addr = requireServing(t, e)
+	cancel()
+
+	select {
+	case err := <-done:
+		require.NoError(t, err)
+	case <-time.After(15 * time.Second):
+		t.Fatal("runServer did not return after the context cancellation")
+	}
+
+	// the address is free again, so the server is really down and not just reported as such
+	var listener, err = net.Listen("tcp", addr)
+	require.NoError(t, err)
+	require.NoError(t, listener.Close())
+}
+
+// TestRunServerStartError ensures a failed start is reported as is and does not wait for the context.
+func TestRunServerStartError(t *testing.T) {
+	var busy, err = net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+
+	t.Cleanup(func() {
+		_ = busy.Close()
+	})
+
+	var done = make(chan error, 1)
+
+	// the context is never cancelled, so only an early return can unblock the test
+	go func() {
+		done <- runServer(context.Background(), newTestEcho(), busy.Addr().String(), zap.NewNop())
+	}()
+
+	select {
+	case err := <-done:
+		var opErr *net.OpError
+		require.ErrorAs(t, err, &opErr)
+		require.Equal(t, "listen", opErr.Op)
+		require.NotErrorIs(t, err, http.ErrServerClosed)
+	case <-time.After(15 * time.Second):
+		t.Fatal("runServer hung on a busy address instead of returning the start error")
+	}
+}
+
+// newTestEcho builds a server that does not write to the test output.
+func newTestEcho() *echo.Echo {
+	var e = echo.New()
+	e.HideBanner = true
+	e.HidePort = true
+
+	return e
+}
+
+// requireServing blocks until the server answers a request and returns the address it listens on.
+// A non nil ListenerAddr is not enough here: echo opens the listener before entering Serve, so
+// shutting down on it would test another path.
+func requireServing(t *testing.T, e *echo.Echo) string {
+	t.Helper()
+
+	// a dedicated transport, so a kept alive connection neither delays the shutdown below
+	// nor survives into another test run
+	var client = http.Client{
+		Timeout:   time.Second,
+		Transport: &http.Transport{DisableKeepAlives: true},
+	}
+
+	var deadline = time.Now().Add(15 * time.Second)
+	for time.Now().Before(deadline) {
+		var addr = e.ListenerAddr()
+		if addr == nil {
+			time.Sleep(time.Millisecond)
+			continue
+		}
+
+		var res, err = client.Get("http://" + addr.String() + "/")
+		if err != nil {
+			time.Sleep(time.Millisecond)
+			continue
+		}
+
+		require.NoError(t, res.Body.Close())
+
+		return addr.String()
+	}
+
+	t.Fatal("the server did not start serving")
+
+	return ""
+}