From bff2b5e735ae7fc80157b4108ddbe56be8acb752 Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 9 Oct 2017 14:59:48 -0700 Subject: Miscellaneous app cleanup (#7594) * app cleanup * whoops, forgot a file * some minor cleanup * longer container deadline * defensive checks --- app/admin.go | 10 ++++---- app/app.go | 65 +++++++++++++++++++++++++++++++++++-------------- app/apptestlib.go | 29 ++++++++++------------ app/oauth.go | 12 +++------ app/options.go | 29 ++++++++++++++++++++++ app/post.go | 2 +- app/server.go | 19 ++++++--------- app/webhook.go | 2 +- app/webrtc.go | 2 +- app/websocket_router.go | 7 ------ 10 files changed, 108 insertions(+), 69 deletions(-) create mode 100644 app/options.go (limited to 'app') diff --git a/app/admin.go b/app/admin.go index a3f7ffa2a..5994fc826 100644 --- a/app/admin.go +++ b/app/admin.go @@ -15,7 +15,6 @@ import ( l4g "github.com/alecthomas/log4go" "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/store" "github.com/mattermost/mattermost-server/store/sqlstore" "github.com/mattermost/mattermost-server/utils" ) @@ -187,12 +186,13 @@ func (a *App) RecycleDatabaseConnection() { oldStore := a.Srv.Store l4g.Warn(utils.T("api.admin.recycle_db_start.warn")) - a.Srv.Store = store.NewLayeredStore(sqlstore.NewSqlSupplier(utils.Cfg.SqlSettings, a.Metrics), a.Metrics, a.Cluster) - + a.Srv.Store = a.newStore() a.Jobs.Store = a.Srv.Store - time.Sleep(20 * time.Second) - oldStore.Close() + if a.Srv.Store != oldStore { + time.Sleep(20 * time.Second) + oldStore.Close() + } l4g.Warn(utils.T("api.admin.recycle_db_end.warn")) } diff --git a/app/app.go b/app/app.go index a250efe5c..7b6499b1f 100644 --- a/app/app.go +++ b/app/app.go @@ -4,17 +4,19 @@ package app import ( - "io/ioutil" "net/http" "sync/atomic" l4g "github.com/alecthomas/log4go" + "github.com/gorilla/mux" "github.com/mattermost/mattermost-server/einterfaces" ejobs "github.com/mattermost/mattermost-server/einterfaces/jobs" "github.com/mattermost/mattermost-server/jobs" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin/pluginenv" + "github.com/mattermost/mattermost-server/store" + "github.com/mattermost/mattermost-server/store/sqlstore" "github.com/mattermost/mattermost-server/utils" ) @@ -44,44 +46,69 @@ type App struct { Metrics einterfaces.MetricsInterface Mfa einterfaces.MfaInterface Saml einterfaces.SamlInterface + + newStore func() store.Store } var appCount = 0 // New creates a new App. You must call Shutdown when you're done with it. // XXX: For now, only one at a time is allowed as some resources are still shared. -func New() *App { +func New(options ...Option) *App { appCount++ if appCount > 1 { panic("Only one App should exist at a time. Did you forget to call Shutdown()?") } + l4g.Info(utils.T("api.server.new_server.init.info")) + app := &App{ goroutineExitSignal: make(chan struct{}, 1), Jobs: &jobs.JobServer{}, + Srv: &Server{ + Router: mux.NewRouter(), + }, } app.initEnterprise() + + for _, option := range options { + option(app) + } + + if app.newStore == nil { + app.newStore = func() store.Store { + return store.NewLayeredStore(sqlstore.NewSqlSupplier(utils.Cfg.SqlSettings, app.Metrics), app.Metrics, app.Cluster) + } + } + + app.Srv.Store = app.newStore() + app.Jobs.Store = app.Srv.Store + + app.Srv.Router.NotFoundHandler = http.HandlerFunc(app.Handle404) + + app.Srv.WebSocketRouter = &WebSocketRouter{ + app: app, + handlers: make(map[string]webSocketHandler), + } + return app } func (a *App) Shutdown() { appCount-- - if a.Srv != nil { - l4g.Info(utils.T("api.server.stop_server.stopping.info")) + l4g.Info(utils.T("api.server.stop_server.stopping.info")) - a.Srv.GracefulServer.Stop(TIME_TO_WAIT_FOR_CONNECTIONS_TO_CLOSE_ON_SERVER_SHUTDOWN) - <-a.Srv.GracefulServer.StopChan() - a.HubStop() + a.StopServer() + a.HubStop() - a.ShutDownPlugins() - a.WaitForGoroutines() + a.ShutDownPlugins() + a.WaitForGoroutines() - a.Srv.Store.Close() - a.Srv = nil + a.Srv.Store.Close() + a.Srv = nil - l4g.Info(utils.T("api.server.stop_server.stopped.info")) - } + l4g.Info(utils.T("api.server.stop_server.stopped.info")) } var accountMigrationInterface func(*App) einterfaces.AccountMigrationInterface @@ -232,9 +259,11 @@ func (a *App) WaitForGoroutines() { } } -func CloseBody(r *http.Response) { - if r.Body != nil { - ioutil.ReadAll(r.Body) - r.Body.Close() - } +func (a *App) Handle404(w http.ResponseWriter, r *http.Request) { + err := model.NewAppError("Handle404", "api.context.404.app_error", nil, "", http.StatusNotFound) + err.Translate(utils.T) + + l4g.Debug("%v: code=404 ip=%v", r.URL.Path, utils.GetIpAddress(r)) + + utils.RenderWebError(err, w, r) } diff --git a/app/apptestlib.go b/app/apptestlib.go index 29139ac39..09bf02d39 100644 --- a/app/apptestlib.go +++ b/app/apptestlib.go @@ -22,26 +22,23 @@ type TestHelper struct { } func setupTestHelper(enterprise bool) *TestHelper { + utils.TranslationsPreInit() + utils.LoadConfig("config.json") + utils.InitTranslations(utils.Cfg.LocalizationSettings) + th := &TestHelper{ App: New(), } - if th.App.Srv == nil { - utils.TranslationsPreInit() - utils.LoadConfig("config.json") - utils.InitTranslations(utils.Cfg.LocalizationSettings) - *utils.Cfg.TeamSettings.MaxUsersPerTeam = 50 - *utils.Cfg.RateLimitSettings.Enable = false - utils.DisableDebugLogForTest() - th.App.NewServer() - th.App.InitStores() - th.App.StartServer() - utils.InitHTML() - utils.EnableDebugLogForTest() - th.App.Srv.Store.MarkSystemRanUnitTests() - - *utils.Cfg.TeamSettings.EnableOpenServer = true - } + *utils.Cfg.TeamSettings.MaxUsersPerTeam = 50 + *utils.Cfg.RateLimitSettings.Enable = false + utils.DisableDebugLogForTest() + th.App.StartServer() + utils.InitHTML() + utils.EnableDebugLogForTest() + th.App.Srv.Store.MarkSystemRanUnitTests() + + *utils.Cfg.TeamSettings.EnableOpenServer = true utils.SetIsLicensed(enterprise) if enterprise { diff --git a/app/oauth.go b/app/oauth.go index 6e411138b..909d16628 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -7,7 +7,6 @@ import ( "bytes" b64 "encoding/base64" "io" - "io/ioutil" "net/http" "net/url" "strings" @@ -428,10 +427,7 @@ func (a *App) RevokeAccessToken(token string) *model.AppError { } func (a *App) CompleteOAuth(service string, body io.ReadCloser, teamId string, props map[string]string) (*model.User, *model.AppError) { - defer func() { - ioutil.ReadAll(body) - body.Close() - }() + defer body.Close() action := props["action"] @@ -688,11 +684,9 @@ func (a *App) AuthorizeOAuthUser(w http.ResponseWriter, r *http.Request, service if resp, err := utils.HttpClient(true).Do(req); err != nil { return nil, "", stateProps, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.token_failed.app_error", nil, err.Error(), http.StatusInternalServerError) } else { - bodyBytes, _ = ioutil.ReadAll(resp.Body) - resp.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes)) - ar = model.AccessResponseFromJson(resp.Body) - defer CloseBody(resp) + resp.Body.Close() + if ar == nil { return nil, "", stateProps, model.NewAppError("AuthorizeOAuthUser", "api.user.authorize_oauth_user.bad_response.app_error", nil, "response_body="+string(bodyBytes), http.StatusInternalServerError) } diff --git a/app/options.go b/app/options.go new file mode 100644 index 000000000..3058769d6 --- /dev/null +++ b/app/options.go @@ -0,0 +1,29 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package app + +import ( + "github.com/mattermost/mattermost-server/store" +) + +type Option func(a *App) + +// By default, the app will use the store specified by the configuration. This allows you to +// construct an app with a different store. +// +// The storeOrFactory parameter must be either a store.Store or func() store.Store. +func StoreOverride(storeOrFactory interface{}) Option { + return func(a *App) { + switch s := storeOrFactory.(type) { + case store.Store: + a.newStore = func() store.Store { + return s + } + case func() store.Store: + a.newStore = s + default: + panic("invalid StoreOverride") + } + } +} diff --git a/app/post.go b/app/post.go index fe9443177..bcba922a6 100644 --- a/app/post.go +++ b/app/post.go @@ -679,7 +679,7 @@ func GetOpenGraphMetadata(url string) *opengraph.OpenGraph { l4g.Error("GetOpenGraphMetadata request failed for url=%v with err=%v", url, err.Error()) return og } - defer CloseBody(res) + defer res.Body.Close() if err := og.ProcessHTML(res.Body); err != nil { l4g.Error("GetOpenGraphMetadata processing failed for url=%v with err=%v", url, err.Error()) diff --git a/app/server.go b/app/server.go index 8b09bfef0..3802c2eec 100644 --- a/app/server.go +++ b/app/server.go @@ -20,7 +20,6 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" - "github.com/mattermost/mattermost-server/store/sqlstore" "github.com/mattermost/mattermost-server/utils" ) @@ -78,16 +77,6 @@ func (cw *CorsWrapper) ServeHTTP(w http.ResponseWriter, r *http.Request) { const TIME_TO_WAIT_FOR_CONNECTIONS_TO_CLOSE_ON_SERVER_SHUTDOWN = time.Second -func (a *App) NewServer() { - l4g.Info(utils.T("api.server.new_server.init.info")) - - a.Srv = &Server{} -} - -func (a *App) InitStores() { - a.Srv.Store = store.NewLayeredStore(sqlstore.NewSqlSupplier(utils.Cfg.SqlSettings, a.Metrics), a.Metrics, a.Cluster) -} - type VaryBy struct{} func (m *VaryBy) Key(r *http.Request) string { @@ -211,3 +200,11 @@ func (a *App) StartServer() { } }() } + +func (a *App) StopServer() { + if a.Srv.GracefulServer != nil { + a.Srv.GracefulServer.Stop(TIME_TO_WAIT_FOR_CONNECTIONS_TO_CLOSE_ON_SERVER_SHUTDOWN) + <-a.Srv.GracefulServer.StopChan() + a.Srv.GracefulServer = nil + } +} diff --git a/app/webhook.go b/app/webhook.go index 9d9b24b10..d3d9bbf8b 100644 --- a/app/webhook.go +++ b/app/webhook.go @@ -109,7 +109,7 @@ func (a *App) TriggerWebhook(payload *model.OutgoingWebhookPayload, hook *model. if resp, err := utils.HttpClient(false).Do(req); err != nil { l4g.Error(utils.T("api.post.handle_webhook_events_and_forget.event_post.error"), err.Error()) } else { - defer CloseBody(resp) + defer resp.Body.Close() webhookResp := model.OutgoingWebhookResponseFromJson(resp.Body) if webhookResp != nil && webhookResp.Text != nil { diff --git a/app/webrtc.go b/app/webrtc.go index d2bfffbe0..65bbac7cd 100644 --- a/app/webrtc.go +++ b/app/webrtc.go @@ -62,7 +62,7 @@ func GetWebrtcToken(sessionId string) (string, *model.AppError) { if rp, err := utils.HttpClient(true).Do(rq); err != nil { return "", model.NewAppError("WebRTC.Token", "model.client.connecting.app_error", nil, err.Error(), http.StatusInternalServerError) } else if rp.StatusCode >= 300 { - defer CloseBody(rp) + defer rp.Body.Close() return "", model.AppErrorFromJson(rp.Body) } else { janusResponse := model.GatewayResponseFromJson(rp.Body) diff --git a/app/websocket_router.go b/app/websocket_router.go index cad53ade7..6bc3a6ff7 100644 --- a/app/websocket_router.go +++ b/app/websocket_router.go @@ -21,13 +21,6 @@ type WebSocketRouter struct { handlers map[string]webSocketHandler } -func (a *App) NewWebSocketRouter() *WebSocketRouter { - return &WebSocketRouter{ - app: a, - handlers: make(map[string]webSocketHandler), - } -} - func (wr *WebSocketRouter) Handle(action string, handler webSocketHandler) { wr.handlers[action] = handler } -- cgit v1.2.3-1-g7c22