From 844802aa9a5edadef134d1057660acd6caa6a1b7 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Fri, 26 Nov 2021 01:32:51 +0100 Subject: [PATCH] chore(httpserver): option to set shutdown timeout for unit tests (#2066) * chore(httpserver): optional shutdown timeout parameter * fix(pprof-tests): bigger shutdown timeout * Increase timeout for `Test_Server_Run_success` * Add test depth to `Test_New` in `httpserver` * Add licenses --- internal/httpserver/option_test.go | 45 ++++++++++++++++++++++++++++++ internal/httpserver/options.go | 34 ++++++++++++++++++++++ internal/httpserver/run.go | 8 +++--- internal/httpserver/run_test.go | 4 +++ internal/httpserver/server.go | 4 ++- internal/httpserver/server_test.go | 7 ++++- internal/pprof/server.go | 5 ++-- internal/pprof/server_test.go | 5 +++- 8 files changed, 103 insertions(+), 9 deletions(-) create mode 100644 internal/httpserver/option_test.go create mode 100644 internal/httpserver/options.go diff --git a/internal/httpserver/option_test.go b/internal/httpserver/option_test.go new file mode 100644 index 0000000000..5a3e6c7554 --- /dev/null +++ b/internal/httpserver/option_test.go @@ -0,0 +1,45 @@ +// Copyright 2021 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + +package httpserver + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func Test_newOptionalSettings(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + options []Option + settings optionalSettings + }{ + "no option": { + settings: optionalSettings{ + shutdownTimeout: 3 * time.Second, + }, + }, + "shutdown option": { + options: []Option{ + ShutdownTimeout(time.Second), + }, + settings: optionalSettings{ + shutdownTimeout: time.Second, + }, + }, + } + + for name, testCase := range testCases { + testCase := testCase + t.Run(name, func(t *testing.T) { + t.Parallel() + + settings := newOptionalSettings(testCase.options) + + assert.Equal(t, testCase.settings, settings) + }) + } +} diff --git a/internal/httpserver/options.go b/internal/httpserver/options.go new file mode 100644 index 0000000000..2b59cc1e60 --- /dev/null +++ b/internal/httpserver/options.go @@ -0,0 +1,34 @@ +// Copyright 2021 ChainSafe Systems (ON) +// SPDX-License-Identifier: LGPL-3.0-only + +package httpserver + +import "time" + +// Option is a functional option for the HTTP server. +type Option func(s *optionalSettings) + +type optionalSettings struct { + shutdownTimeout time.Duration +} + +func newOptionalSettings(options []Option) (settings optionalSettings) { + for _, option := range options { + option(&settings) + } + + if settings.shutdownTimeout == 0 { + const defaultShutdownTimeout = 3 * time.Second + settings.shutdownTimeout = defaultShutdownTimeout + } + + return settings +} + +// ShutdownTimeout sets an optional timeout for the HTTP server +// to shutdown. The default shutdown is 3 seconds. +func ShutdownTimeout(timeout time.Duration) Option { + return func(s *optionalSettings) { + s.shutdownTimeout = timeout + } +} diff --git a/internal/httpserver/run.go b/internal/httpserver/run.go index 2e5c8539ec..6bbdbad87b 100644 --- a/internal/httpserver/run.go +++ b/internal/httpserver/run.go @@ -8,7 +8,6 @@ import ( "errors" "net" "net/http" - "time" ) // Run runs the HTTP server until ctx is canceled. @@ -28,11 +27,12 @@ func (s *Server) Run(ctx context.Context, ready chan<- struct{}, done chan<- err } s.logger.Warn(s.name + " http server shutting down: " + ctx.Err().Error()) - const shutdownGraceDuration = 3 * time.Second - shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownGraceDuration) + shutdownCtx, cancel := context.WithTimeout(context.Background(), + s.optional.shutdownTimeout) defer cancel() if err := server.Shutdown(shutdownCtx); err != nil { - s.logger.Error(s.name + " http server failed shutting down: " + err.Error()) + s.logger.Error(s.name + " http server failed shutting down within " + + s.optional.shutdownTimeout.String()) } }() diff --git a/internal/httpserver/run_test.go b/internal/httpserver/run_test.go index 424f8c0b0e..1b88fd53c0 100644 --- a/internal/httpserver/run_test.go +++ b/internal/httpserver/run_test.go @@ -7,6 +7,7 @@ import ( "context" "regexp" "testing" + "time" gomock "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -26,6 +27,9 @@ func Test_Server_Run_success(t *testing.T) { address: "127.0.0.1:0", addressSet: make(chan struct{}), logger: logger, + optional: optionalSettings{ + shutdownTimeout: 10 * time.Second, + }, } ctx, cancel := context.WithCancel(context.Background()) diff --git a/internal/httpserver/server.go b/internal/httpserver/server.go index 1f26e135e2..6faa8da1f8 100644 --- a/internal/httpserver/server.go +++ b/internal/httpserver/server.go @@ -35,17 +35,19 @@ type Server struct { addressSet chan struct{} handler http.Handler logger Logger + optional optionalSettings } // New creates a new HTTP server with a name, listening on // the address specified and using the HTTP handler provided. func New(name, address string, handler http.Handler, - logger Logger) *Server { + logger Logger, options ...Option) *Server { return &Server{ name: name, address: address, addressSet: make(chan struct{}), handler: handler, logger: logger, + optional: newOptionalSettings(options), } } diff --git a/internal/httpserver/server_test.go b/internal/httpserver/server_test.go index bfe2be6977..661c41bcb2 100644 --- a/internal/httpserver/server_test.go +++ b/internal/httpserver/server_test.go @@ -6,6 +6,7 @@ package httpserver import ( "net/http" "testing" + "time" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -27,9 +28,13 @@ func Test_New(t *testing.T) { address: address, handler: handler, logger: logger, + optional: optionalSettings{ + shutdownTimeout: time.Second, + }, } - server := New(name, address, handler, logger) + server := New(name, address, handler, logger, + ShutdownTimeout(time.Second)) assert.NotNil(t, server.addressSet) server.addressSet = nil diff --git a/internal/pprof/server.go b/internal/pprof/server.go index a097fd4d6d..d0428363d6 100644 --- a/internal/pprof/server.go +++ b/internal/pprof/server.go @@ -12,7 +12,8 @@ import ( // NewServer creates a new Pprof server which will listen at // the address specified. -func NewServer(address string, logger httpserver.Logger) *httpserver.Server { +func NewServer(address string, logger httpserver.Logger, + options ...httpserver.Option) *httpserver.Server { handler := http.NewServeMux() handler.HandleFunc("/debug/pprof/", pprof.Index) handler.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) @@ -23,5 +24,5 @@ func NewServer(address string, logger httpserver.Logger) *httpserver.Server { handler.Handle("/debug/pprof/goroutine", pprof.Handler("goroutine")) handler.Handle("/debug/pprof/heap", pprof.Handler("heap")) handler.Handle("/debug/pprof/threadcreate", pprof.Handler("threadcreate")) - return httpserver.New("pprof", address, handler, logger) + return httpserver.New("pprof", address, handler, logger, options...) } diff --git a/internal/pprof/server_test.go b/internal/pprof/server_test.go index 1f51309567..09f5642d9e 100644 --- a/internal/pprof/server_test.go +++ b/internal/pprof/server_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/ChainSafe/gossamer/internal/httpserver" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -27,7 +28,9 @@ func Test_Server(t *testing.T) { logger.EXPECT().Info(newRegexMatcher("^pprof http server listening on 127.0.0.1:[1-9][0-9]{0,4}$")) logger.EXPECT().Warn("pprof http server shutting down: context canceled") - server := NewServer(address, logger) + const httpServerShutdownTimeout = 10 * time.Second // 10s in case test worker is slow + server := NewServer(address, logger, + httpserver.ShutdownTimeout(httpServerShutdownTimeout)) require.NotNil(t, server) ctx, cancel := context.WithCancel(context.Background())