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 diff --git a/internal/command/http_server.go b/internal/command/http_server.go index c7541c6..15aefb7 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" @@ -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 +} diff --git a/internal/command/http_server_test.go b/internal/command/http_server_test.go new file mode 100644 index 0000000..90940d2 --- /dev/null +++ b/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 "" +}