diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index fca94c9..2d0800c 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -325,6 +325,7 @@ func newTestServerFx( cfg *config.Config, mware *middleware.Middleware, hdlr *handlers.Handlers, + shutdowner fx.Shutdowner, ) (*server.Server, error) { srv, err := server.New(lifecycle, server.Params{ //nolint:exhaustruct Logger: log, @@ -332,6 +333,7 @@ func newTestServerFx( Config: cfg, Middleware: mware, Handlers: hdlr, + Shutdowner: shutdowner, }) if err != nil { return nil, fmt.Errorf("test server: %w", err) diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 656b378..3908b62 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -1,6 +1,10 @@ package server -import "github.com/go-chi/chi/v5" +import ( + "net" + + "github.com/go-chi/chi/v5" +) // NewTestSPAServer creates a Server whose router serves only // the embedded web client. @@ -10,3 +14,9 @@ func NewTestSPAServer() *Server { return srv } + +// ServeUntilShutdown runs serveUntilShutdown, which serves the +// HTTP server on listener, and returns when it does. +func (srv *Server) ServeUntilShutdown(listener net.Listener) { + srv.serveUntilShutdown(listener) +} diff --git a/internal/server/server.go b/internal/server/server.go index 49bcd50..0bd8951 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "log/slog" + "net" "net/http" "os" "os/signal" @@ -39,6 +40,7 @@ type Params struct { Config *config.Config Middleware *middleware.Middleware Handlers *handlers.Handlers + Shutdowner fx.Shutdowner } // Server is the main HTTP server. @@ -66,7 +68,7 @@ func New( } lifecycle.Append(fx.Hook{ - OnStart: func(_ context.Context) error { + OnStart: func(ctx context.Context) error { srv.startupTime = time.Now() // Build the router before OnStart returns, so srv can @@ -78,9 +80,7 @@ func New( // A closed stdout or stderr must not end the process. signal.Ignore(syscall.SIGPIPE) - srv.startHTTP() - - return nil + return srv.startHTTP(ctx) }, OnStop: func(ctx context.Context) error { srv.cleanShutdown(ctx) @@ -162,15 +162,23 @@ func (srv *Server) configure() { // Server configuration placeholder. } -// startHTTP creates the HTTP server and serves it from a new -// goroutine. srv.httpServer is set before that goroutine -// starts and never changes, so it and cleanShutdown read it -// without a lock. -func (srv *Server) startHTTP() { +// startHTTP binds the HTTP listen address, so that a port +// that is taken fails startup, then serves the HTTP server +// from a new goroutine. srv.httpServer is set before that +// goroutine starts and never changes, so it and cleanShutdown +// read it without a lock. +func (srv *Server) startHTTP(ctx context.Context) error { listenAddr := fmt.Sprintf( ":%d", srv.params.Config.Port, ) + var listenConfig net.ListenConfig + + listener, err := listenConfig.Listen(ctx, "tcp", listenAddr) + if err != nil { + return fmt.Errorf("http listen: %w", err) + } + srv.httpServer = &http.Server{ //nolint:exhaustruct // optional fields Addr: listenAddr, ReadTimeout: httpReadTimeout, @@ -179,16 +187,30 @@ func (srv *Server) startHTTP() { Handler: srv, } - go srv.serveUntilShutdown() + go func() { + srv.serveUntilShutdown(listener) + }() + + return nil } -func (srv *Server) serveUntilShutdown() { +// serveUntilShutdown serves the HTTP server on listener until +// cleanShutdown stops it. If serving fails before then, it +// stops the app with exit code 1. +func (srv *Server) serveUntilShutdown(listener net.Listener) { srv.log.Info( "http begin listen", "listenaddr", srv.httpServer.Addr, ) - err := srv.httpServer.ListenAndServe() - if err != nil && !errors.Is(err, http.ErrServerClosed) { - srv.log.Error("listen error", "error", err) + err := srv.httpServer.Serve(listener) + if errors.Is(err, http.ErrServerClosed) { + return + } + + srv.log.Error("listen error", "error", err) + + err = srv.params.Shutdowner.Shutdown(fx.ExitCode(1)) + if err != nil { + srv.log.Error("shutdown failed", "error", err) } } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index c740ba1..148f28e 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1,3 +1,7 @@ +// Tests use a global viper instance for configuration, +// making parallel execution unsafe. +// +//nolint:paralleltest package server_test import ( @@ -6,6 +10,7 @@ import ( "net" "net/http" "strconv" + "strings" "testing" "time" @@ -40,8 +45,6 @@ const ( // for a long poll the HTTP server is serving to finish, and // that the poll gets its response. func TestStopWaitsForRequestInProgress(t *testing.T) { - t.Parallel() - port := freePort(t) addr := net.JoinHostPort("127.0.0.1", strconv.Itoa(port)) @@ -103,12 +106,114 @@ func TestStopWaitsForRequestInProgress(t *testing.T) { } } -// startApp starts the server with the dependencies -// cmd/neoircd gives it, on a fresh in-memory database, with -// the HTTP server listening on port. +// TestStartFailsWhenHTTPPortIsTaken checks that starting the +// app, which runs the start hook server.New registers, +// returns an error naming the HTTP listen address when +// something else is already listening on its port. +func TestStartFailsWhenHTTPPortIsTaken(t *testing.T) { + var listenConfig net.ListenConfig + + taken, err := listenConfig.Listen(t.Context(), "tcp", ":0") + if err != nil { + t.Fatalf("listen: %v", err) + } + + t.Cleanup(func() { _ = taken.Close() }) + + tcpAddr, ok := taken.Addr().(*net.TCPAddr) + if !ok { + t.Fatalf("listener address %v is not TCP", taken.Addr()) + } + + app, _, _ := newApp(t, tcpAddr.Port) + + err = app.Start(t.Context()) + if err == nil { + app.RequireStop() + t.Fatal("the app started while its HTTP port was taken") + } + + listenAddr := fmt.Sprintf(":%d", tcpAddr.Port) + if !strings.Contains(err.Error(), listenAddr) { + t.Errorf("start error %q does not name %s", err, listenAddr) + } +} + +// TestServeFailureStopsApp checks that when serving HTTP fails +// after the start hook has bound the listen address, the +// server stops the app with exit code 1. +func TestServeFailureStopsApp(t *testing.T) { + var srv *server.Server + + app, _, _ := startApp(t, freePort(t), fx.Populate(&srv)) + + shutdowns := app.Wait() + + srv.ServeUntilShutdown(closedListener(t)) + + select { + case shutdown := <-shutdowns: + if shutdown.ExitCode != 1 { + t.Errorf("exit code %d, want 1", shutdown.ExitCode) + } + default: + t.Error("the server did not stop the app") + } +} + +// TestServeAfterStopDoesNotStopApp checks that serving HTTP +// after the app has stopped, which ends at once because the +// stop hook has shut the HTTP server down, does not stop the +// app. Serving ends the same way on every normal stop. +func TestServeAfterStopDoesNotStopApp(t *testing.T) { + var srv *server.Server + + app, _, _ := startApp(t, freePort(t), fx.Populate(&srv)) + + shutdowns := app.Wait() + + app.RequireStop() + + srv.ServeUntilShutdown(closedListener(t)) + + select { + case shutdown := <-shutdowns: + t.Errorf( + "the server stopped the app with exit code %d", + shutdown.ExitCode, + ) + default: + } +} + +// startApp starts the app newApp builds, with the HTTP server +// listening on port. func startApp( t *testing.T, port int, + options ...fx.Option, +) (*fxtest.App, *broker.Broker, *db.Database) { + t.Helper() + + app, brk, database := newApp(t, port, options...) + + app.RequireStart() + + // Stops the app if the test ends before stopping it. + t.Cleanup(app.RequireStop) + + return app, brk, database +} + +// newApp builds, without starting it, an app that runs the +// server with the dependencies cmd/neoircd gives it, on a +// fresh in-memory database, with the HTTP port set to port. +// options are added to the app, for a test that needs more +// from it than the broker and the database. +func newApp( + t *testing.T, + port int, + options ...fx.Option, ) (*fxtest.App, *broker.Broker, *db.Database) { t.Helper() @@ -146,13 +251,9 @@ func startApp( }), fx.Invoke(func(*server.Server) {}), fx.Populate(&brk, &database), + fx.Options(options...), ) - app.RequireStart() - - // Stops the app if the test ends before stopping it. - t.Cleanup(app.RequireStop) - return app, brk, database } @@ -161,6 +262,21 @@ func startApp( func freePort(t *testing.T) int { t.Helper() + listener := closedListener(t) + + tcpAddr, ok := listener.Addr().(*net.TCPAddr) + if !ok { + t.Fatalf("listener address %v is not TCP", listener.Addr()) + } + + return tcpAddr.Port +} + +// closedListener returns a TCP listener that has already been +// closed, so that accepting a connection on it fails. +func closedListener(t *testing.T) net.Listener { + t.Helper() + var listenConfig net.ListenConfig listener, err := listenConfig.Listen( @@ -170,17 +286,12 @@ func freePort(t *testing.T) int { t.Fatalf("listen: %v", err) } - tcpAddr, ok := listener.Addr().(*net.TCPAddr) - if !ok { - t.Fatalf("listener address %v is not TCP", listener.Addr()) - } - err = listener.Close() if err != nil { t.Fatalf("close listener: %v", err) } - return tcpAddr.Port + return listener } // canConnect reports whether addr accepts a TCP connection.