From e636522c7535a76c61396a4a81b1482ec35aab19 Mon Sep 17 00:00:00 2001 From: rzatonskiy Date: Fri, 11 Sep 2026 18:58:13 +0300 Subject: [PATCH 1/4] fix(http-server): remove data race between Start and Shutdown * pass the e.Start result through a buffered channel instead of the named result * treat http.ErrServerClosed as a clean stop and return nil * return an early start failure without waiting for the context * join the serving goroutine after the shutdown and report its error * cover both paths with tests runnable under -race --- internal/command/http_server.go | 78 ++++++++++++++------- internal/command/http_server_test.go | 101 +++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 24 deletions(-) create mode 100644 internal/command/http_server_test.go diff --git a/internal/command/http_server.go b/internal/command/http_server.go index c7541c6..3d5f63b 100644 --- a/internal/command/http_server.go +++ b/internal/command/http_server.go @@ -6,8 +6,10 @@ package command import ( "context" + "errors" "fmt" "net" + "net/http" "time" "github.com/gozix/di" @@ -40,10 +42,9 @@ func NewHTTPServer(ctn di.Container) *cobra.Command { var wg, ctx = errgroup.WithContext(cmd.Context()) for _, srvName := range serverNames { - var srvName = srvName - wg.Go(func() (err error) { + wg.Go(func() error { var e *echo.Echo - if err = ctn.Resolve(&e, di.WithTags(tagEcho+"."+srvName)); err != nil { + if err := ctn.Resolve(&e, di.WithTags(tagEcho+"."+srvName)); err != nil { return err } @@ -59,27 +60,7 @@ func NewHTTPServer(ctn di.Container) *cobra.Command { ) ) - 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) + return runServer(ctx, e, addr, log) }) } @@ -88,3 +69,52 @@ 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() + + var shutdownErr = e.Shutdown(timeoutContext) + + // join the serving goroutine, so it neither logs nor leaks after the command returns. + // Shutdown closes the listener, so Start has already returned by now. + if err := <-errCh; shutdownErr == nil && !errors.Is(err, http.ErrServerClosed) { + return err + } + + return shutdownErr +} diff --git a/internal/command/http_server_test.go b/internal/command/http_server_test.go new file mode 100644 index 0000000..6277c87 --- /dev/null +++ b/internal/command/http_server_test.go @@ -0,0 +1,101 @@ +// 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 = echo.New() + ctx, cancel = context.WithCancel(context.Background()) + done = make(chan error, 1) + ) + + defer cancel() + + go func() { + done <- runServer(ctx, e, "127.0.0.1:0", zap.NewNop()) + }() + + waitServing(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") + } +} + +// 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) + + defer func() { + require.NoError(t, busy.Close()) + }() + + var ( + e = echo.New() + 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(), e, busy.Addr().String(), zap.NewNop()) + }() + + select { + case err := <-done: + require.Error(t, err) + case <-time.After(15 * time.Second): + t.Fatal("runServer hung on a busy address instead of returning the start error") + } +} + +// waitServing blocks until the server answers a request. 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 waitServing(t *testing.T, e *echo.Echo) { + t.Helper() + + var ( + client = http.Client{Timeout: time.Second} + 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 + } + + t.Fatal("the server did not start serving") +} From f40b65b49cfa74e200a2320c37065245c3cb7646 Mon Sep 17 00:00:00 2001 From: rzatonskiy Date: Fri, 11 Sep 2026 22:17:05 +0300 Subject: [PATCH 2/4] ci: run the test suite under the race detector * add -race to the test script, so a reintroduced race fails the build * raise the go matrix to 1.23-1.27, the module requires go 1.23.0 --- .github/workflows/go.yml | 7 +++++-- .scripts/test.sh | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index c5fba70..1018ee7 100644 --- a/.github/workflows/go.yml +++ b/.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 diff --git a/.scripts/test.sh b/.scripts/test.sh index 6aadd47..9d2bfdc 100644 --- a/.scripts/test.sh +++ b/.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 From 3e1c3eec92bd8b96652464005fe82d5f6ff20cc1 Mon Sep 17 00:00:00 2001 From: rzatonskiy Date: Mon, 14 Sep 2026 13:52:45 +0300 Subject: [PATCH 3/4] fix(http-server): resolve servers before starting any of them * the di container is not safe for concurrent resolution, do it sequentially * a resolve failure now fails before a single server is started, so no server is left running behind an error --- internal/command/http_server.go | 54 +++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 19 deletions(-) diff --git a/internal/command/http_server.go b/internal/command/http_server.go index 3d5f63b..5e02757 100644 --- a/internal/command/http_server.go +++ b/internal/command/http_server.go @@ -27,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{ @@ -39,28 +46,37 @@ 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 is not safe for + // concurrent resolution, and a failure here must not leave a started server behind + var servers = make([]server, 0, len(serverNames)) for _, srvName := range serverNames { - wg.Go(func() 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), + ), + }) + } - return runServer(ctx, e, addr, log) + var wg, ctx = errgroup.WithContext(cmd.Context()) + for _, srv := range servers { + wg.Go(func() error { + return runServer(ctx, srv.echo, srv.addr, srv.log) }) } From ff27ad813218e23031bb6742262cf7281f68e4ce Mon Sep 17 00:00:00 2001 From: rzatonskiy Date: Mon, 14 Sep 2026 14:11:08 +0300 Subject: [PATCH 4/4] fix(http-server): do not join the serving goroutine after a failed shutdown * report a shutdown failure as is: the listener may still be open, and echo.Shutdown gives up on the plain server whenever the TLS one fails, so the join would hang * pin the start failure down to a listen error in the test, not to any error at all * assert the address is free once the command returned, so a shutdown that never happened cannot pass * keep the echo banner out of the test output and stop sharing the default transport --- internal/command/http_server.go | 19 ++++++--- internal/command/http_server_test.go | 60 ++++++++++++++++++---------- 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/internal/command/http_server.go b/internal/command/http_server.go index 5e02757..15aefb7 100644 --- a/internal/command/http_server.go +++ b/internal/command/http_server.go @@ -46,8 +46,10 @@ func NewHTTPServer(ctn di.Container) *cobra.Command { } return ctn.Call(func(serverNames []string, cfg *viper.Viper, logger *zap.Logger) error { - // resolve every server before starting any of them: the container is not safe for - // concurrent resolution, and a failure here must not leave a started server behind + // 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 e *echo.Echo @@ -124,13 +126,18 @@ func runServer(ctx context.Context, e *echo.Echo, addr string, log *zap.Logger) var timeoutContext, cancel = context.WithTimeout(context.Background(), timeout) defer cancel() - var shutdownErr = e.Shutdown(timeoutContext) + // 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 closes the listener, so Start has already returned by now. - if err := <-errCh; shutdownErr == nil && !errors.Is(err, http.ErrServerClosed) { + // Shutdown closed the listener, so Start has already returned by now. + if err := <-errCh; !errors.Is(err, http.ErrServerClosed) { return err } - return shutdownErr + return nil } diff --git a/internal/command/http_server_test.go b/internal/command/http_server_test.go index 6277c87..90940d2 100644 --- a/internal/command/http_server_test.go +++ b/internal/command/http_server_test.go @@ -20,18 +20,18 @@ import ( // and without reporting the shutdown itself as an error. Run it with -race. func TestRunServerGracefulShutdown(t *testing.T) { var ( - e = echo.New() + e = newTestEcho() ctx, cancel = context.WithCancel(context.Background()) done = make(chan error, 1) ) - defer cancel() + t.Cleanup(cancel) go func() { done <- runServer(ctx, e, "127.0.0.1:0", zap.NewNop()) }() - waitServing(t, e) + var addr = requireServing(t, e) cancel() select { @@ -40,6 +40,11 @@ func TestRunServerGracefulShutdown(t *testing.T) { 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. @@ -47,38 +52,51 @@ func TestRunServerStartError(t *testing.T) { var busy, err = net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - defer func() { - require.NoError(t, busy.Close()) - }() + t.Cleanup(func() { + _ = busy.Close() + }) - var ( - e = echo.New() - done = make(chan error, 1) - ) + 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(), e, busy.Addr().String(), zap.NewNop()) + done <- runServer(context.Background(), newTestEcho(), busy.Addr().String(), zap.NewNop()) }() select { case err := <-done: - require.Error(t, err) + 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") } } -// waitServing blocks until the server answers a request. 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 waitServing(t *testing.T, e *echo.Echo) { +// 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() - var ( - client = http.Client{Timeout: time.Second} - deadline = time.Now().Add(15 * time.Second) - ) + // 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 { @@ -94,8 +112,10 @@ func waitServing(t *testing.T, e *echo.Echo) { require.NoError(t, res.Body.Close()) - return + return addr.String() } t.Fatal("the server did not start serving") + + return "" }