From 809a16458f7483a2b762cd546493780fea6220ea Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 7 Feb 2018 13:41:15 +0530 Subject: Abort on critical error during server startup (#8204) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only a handful of critical errors are present in the codebase. They all occur during server startup (in `app.StartServer()`). Currently, when one of these critical error occurs, it is simpled mentionned in the logs – then the error is discarded, and the app attempts to continue the execution (and probably fails pretty quickly in a weird way). Rather than continuing operations in an unknow state, these errors should trigger a clean exit. This commit rewrites critical startup errors to be correctly propagated, logged, and then terminate the command execution. Additionnaly, it makes the server return a proper error code to the shell. --- api/apitestlib.go | 6 +++++- api4/apitestlib.go | 6 +++++- app/app_test.go | 3 ++- app/apptestlib.go | 6 +++++- app/server.go | 12 +++++++----- app/server_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ cmd/platform/server.go | 9 +++++++-- cmd/platform/test.go | 12 ++++++++++-- web/web_test.go | 5 ++++- 9 files changed, 95 insertions(+), 14 deletions(-) create mode 100644 app/server_test.go diff --git a/api/apitestlib.go b/api/apitestlib.go index 8d7f54902..bae00927a 100644 --- a/api/apitestlib.go +++ b/api/apitestlib.go @@ -105,7 +105,11 @@ func setupTestHelper(enterprise bool) *TestHelper { if testStore != nil { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" }) } - th.App.StartServer() + serverErr := th.App.StartServer() + if serverErr != nil { + panic(serverErr) + } + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) api4.Init(th.App, th.App.Srv.Router, false) Init(th.App, th.App.Srv.Router) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index a7e64ae84..ccdb4c206 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -113,7 +113,11 @@ func setupTestHelper(enterprise bool) *TestHelper { if testStore != nil { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" }) } - th.App.StartServer() + serverErr := th.App.StartServer() + if serverErr != nil { + panic(serverErr) + } + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) Init(th.App, th.App.Srv.Router, true) wsapi.Init(th.App, th.App.Srv.WebSocketRouter) diff --git a/app/app_test.go b/app/app_test.go index 25b19ead8..09f8725d7 100644 --- a/app/app_test.go +++ b/app/app_test.go @@ -51,7 +51,8 @@ func TestAppRace(t *testing.T) { a, err := New() require.NoError(t, err) a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" }) - a.StartServer() + serverErr := a.StartServer() + require.NoError(t, serverErr) a.Shutdown() } } diff --git a/app/apptestlib.go b/app/apptestlib.go index 09afc8f76..016a68bec 100644 --- a/app/apptestlib.go +++ b/app/apptestlib.go @@ -96,7 +96,11 @@ func setupTestHelper(enterprise bool) *TestHelper { if testStore != nil { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" }) } - th.App.StartServer() + serverErr := th.App.StartServer() + if serverErr != nil { + panic(serverErr) + } + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) th.App.Srv.Store.MarkSystemRanUnitTests() diff --git a/app/server.go b/app/server.go index 1659908b6..afa282ad6 100644 --- a/app/server.go +++ b/app/server.go @@ -17,6 +17,7 @@ import ( l4g "github.com/alecthomas/log4go" "github.com/gorilla/handlers" "github.com/gorilla/mux" + "github.com/pkg/errors" "golang.org/x/crypto/acme/autocert" "github.com/mattermost/mattermost-server/model" @@ -116,7 +117,7 @@ func redirectHTTPToHTTPS(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, url.String(), http.StatusFound) } -func (a *App) StartServer() { +func (a *App) StartServer() error { l4g.Info(utils.T("api.server.start_server.starting.info")) var handler http.Handler = &CorsWrapper{a.Config, a.Srv.Router} @@ -126,8 +127,7 @@ func (a *App) StartServer() { rateLimiter, err := NewRateLimiter(&a.Config().RateLimitSettings) if err != nil { - l4g.Critical(err.Error()) - return + return err } a.Srv.RateLimiter = rateLimiter @@ -151,8 +151,8 @@ func (a *App) StartServer() { listener, err := net.Listen("tcp", addr) if err != nil { - l4g.Critical(utils.T("api.server.start_server.starting.critical"), err) - return + errors.Wrapf(err, utils.T("api.server.start_server.starting.critical"), err) + return err } a.Srv.ListenAddr = listener.Addr().(*net.TCPAddr) @@ -214,6 +214,8 @@ func (a *App) StartServer() { } close(a.Srv.didFinishListen) }() + + return nil } type tcpKeepAliveListener struct { diff --git a/app/server_test.go b/app/server_test.go new file mode 100644 index 000000000..de358b976 --- /dev/null +++ b/app/server_test.go @@ -0,0 +1,50 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package app + +import ( + "testing" + + "github.com/mattermost/mattermost-server/model" + "github.com/stretchr/testify/require" +) + +func TestStartServerSuccess(t *testing.T) { + a, err := New() + require.NoError(t, err) + + a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" }) + serverErr := a.StartServer() + a.Shutdown() + require.NoError(t, serverErr) +} + +func TestStartServerRateLimiterCriticalError(t *testing.T) { + a, err := New() + require.NoError(t, err) + + // Attempt to use Rate Limiter with an invalid config + a.UpdateConfig(func(cfg *model.Config) { + *cfg.RateLimitSettings.Enable = true + *cfg.RateLimitSettings.MaxBurst = -100 + }) + + serverErr := a.StartServer() + a.Shutdown() + require.Error(t, serverErr) +} + +func TestStartServerPortUnavailable(t *testing.T) { + a, err := New() + require.NoError(t, err) + + // Attempt to listen on a system-reserved port + a.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.ListenAddress = ":21" + }) + + serverErr := a.StartServer() + a.Shutdown() + require.Error(t, serverErr) +} diff --git a/cmd/platform/server.go b/cmd/platform/server.go index e3742cef6..a8a6e8923 100644 --- a/cmd/platform/server.go +++ b/cmd/platform/server.go @@ -53,7 +53,7 @@ func runServer(configFileLocation string, disableConfigWatch bool) error { a, err := app.New(options...) if err != nil { - l4g.Error(err.Error()) + l4g.Critical(err.Error()) return err } defer a.Shutdown() @@ -87,7 +87,12 @@ func runServer(configFileLocation string, disableConfigWatch bool) error { } }) - a.StartServer() + serverErr := a.StartServer() + if serverErr != nil { + l4g.Critical(serverErr.Error()) + return serverErr + } + api4.Init(a, a.Srv.Router, false) api3 := api.Init(a, a.Srv.Router) wsapi.Init(a, a.Srv.WebSocketRouter) diff --git a/cmd/platform/test.go b/cmd/platform/test.go index 036df07de..9ab3fbb36 100644 --- a/cmd/platform/test.go +++ b/cmd/platform/test.go @@ -53,7 +53,11 @@ func webClientTestsCmdF(cmd *cobra.Command, args []string) error { defer a.Shutdown() utils.InitTranslations(a.Config().LocalizationSettings) - a.StartServer() + serverErr := a.StartServer() + if serverErr != nil { + return serverErr + } + api4.Init(a, a.Srv.Router, false) api.Init(a, a.Srv.Router) wsapi.Init(a, a.Srv.WebSocketRouter) @@ -71,7 +75,11 @@ func serverForWebClientTestsCmdF(cmd *cobra.Command, args []string) error { defer a.Shutdown() utils.InitTranslations(a.Config().LocalizationSettings) - a.StartServer() + serverErr := a.StartServer() + if serverErr != nil { + return serverErr + } + api4.Init(a, a.Srv.Router, false) api.Init(a, a.Srv.Router) wsapi.Init(a, a.Srv.WebSocketRouter) diff --git a/web/web_test.go b/web/web_test.go index 21a7968b3..c8d64c61d 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -44,7 +44,10 @@ func Setup() *app.App { } prevListenAddress := *a.Config().ServiceSettings.ListenAddress a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = ":0" }) - a.StartServer() + serverErr := a.StartServer() + if serverErr != nil { + panic(serverErr) + } a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) api4.Init(a, a.Srv.Router, false) api3 := api.Init(a, a.Srv.Router) -- cgit v1.2.3-1-g7c22