From af1f6b7c891dfc2a5907e8479cf50a3474f9305b Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 26 Aug 2020 22:52:26 +0200 Subject: [PATCH 1/4] Fix server shutdown race Instead of using locks I simply initialize the servers during creation. In that way there are no writes while serving. --- services/horizon/internal/app.go | 5 ++- services/horizon/internal/httpx/server.go | 55 +++++++++++++---------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/services/horizon/internal/app.go b/services/horizon/internal/app.go index da690a5c90..6e5b9fd3b5 100644 --- a/services/horizon/internal/app.go +++ b/services/horizon/internal/app.go @@ -133,7 +133,7 @@ func (a *App) Serve() { }() go a.waitForDone() - err := a.webServer.Serve(uint16(a.config.Port), a.config.TLSCert, a.config.TLSKey, uint16(a.config.AdminPort)) + err := a.webServer.Serve() if err != nil && err != http.ErrServerClosed { log.Fatal(err) } @@ -466,7 +466,8 @@ func (a *App) init() error { } var err error - a.webServer, err = httpx.NewServer(webConfig) + a.webServer, err = httpx.NewServer( + webConfig, uint16(a.config.Port), a.config.TLSCert, a.config.TLSKey, uint16(a.config.AdminPort)) if err != nil { return err } diff --git a/services/horizon/internal/httpx/server.go b/services/horizon/internal/httpx/server.go index ed88667121..4f58c19719 100644 --- a/services/horizon/internal/httpx/server.go +++ b/services/horizon/internal/httpx/server.go @@ -27,10 +27,14 @@ type ServerMetrics struct { // Web contains the http server related fields for horizon: the Router, // rate limiter, etc. type Server struct { - Router *Router - Metrics *ServerMetrics - server *http.Server + Router *Router + Metrics *ServerMetrics + server *http.Server + tlsFiles *struct { + certFile, keyFile string + } internalServer *http.Server + sync.RWMutex } func init() { @@ -46,7 +50,7 @@ func init() { problem.RegisterError(db.ErrCancelled, hProblem.ServiceUnavailable) } -func NewServer(config *RouterConfig) (*Server, error) { +func NewServer(config *RouterConfig, port uint16, certFile, keyFile string, adminPort uint16) (*Server, error) { sm := &ServerMetrics{ RequestDurationSummary: prometheus.NewSummaryVec( prometheus.SummaryOpts{ @@ -56,47 +60,52 @@ func NewServer(config *RouterConfig) (*Server, error) { []string{"status", "route", "streaming", "method"}, ), } - router, err := NewRouter(config, sm) if err != nil { return nil, err } + addr := fmt.Sprintf(":%d", port) result := &Server{ Router: router, Metrics: sm, + server: &http.Server{ + Addr: addr, + Handler: router, + ReadTimeout: 5 * time.Second, + }, + } + if certFile != "" && keyFile != "" { + result.tlsFiles = &struct { + certFile, keyFile string + }{keyFile, certFile} + } + if adminPort != 0 { + adminAddr := fmt.Sprintf(":%d", adminPort) + result.internalServer = &http.Server{ + Addr: adminAddr, + Handler: result.Router.Internal, + ReadTimeout: 5 * time.Second, + } } return result, nil } -func (s *Server) Serve(port uint16, certFile, keyFile string, adminPort uint16) error { +func (s *Server) Serve() error { if s.server != nil { return errors.New("server already started") } - if adminPort != 0 { + if s.internalServer != nil { go func() { - adminAddr := fmt.Sprintf(":%d", adminPort) - log.Infof("Starting internal server on %s", adminAddr) - s.internalServer = &http.Server{ - Addr: adminAddr, - Handler: s.Router.Internal, - ReadTimeout: 5 * time.Second, - } + log.Infof("Starting internal server on %s", s.internalServer.Addr) if err := s.internalServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { log.Warn(errors.Wrap(err, "error in internalServer.ListenAndServe()")) } }() } - addr := fmt.Sprintf(":%d", port) - s.server = &http.Server{ - Addr: addr, - Handler: s.Router, - ReadTimeout: 5 * time.Second, - } - var err error - if certFile != "" { - err = s.server.ListenAndServeTLS(certFile, keyFile) + if s.tlsFiles != nil { + err = s.server.ListenAndServeTLS(s.tlsFiles.certFile, s.tlsFiles.keyFile) } else { err = s.server.ListenAndServe() } From 8f2d0b639d8197d61493d4b3dd4c9c9b724c8387 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 27 Aug 2020 19:04:53 +0200 Subject: [PATCH 2/4] Fix starting error (thanks bartek) --- services/horizon/internal/httpx/server.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/services/horizon/internal/httpx/server.go b/services/horizon/internal/httpx/server.go index 4f58c19719..680ac09825 100644 --- a/services/horizon/internal/httpx/server.go +++ b/services/horizon/internal/httpx/server.go @@ -90,10 +90,6 @@ func NewServer(config *RouterConfig, port uint16, certFile, keyFile string, admi return result, nil } func (s *Server) Serve() error { - if s.server != nil { - return errors.New("server already started") - } - if s.internalServer != nil { go func() { log.Infof("Starting internal server on %s", s.internalServer.Addr) From 8509df2156525a566fe8d5843ee5bf47fe29acd3 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 27 Aug 2020 19:56:26 +0200 Subject: [PATCH 3/4] Update services/horizon/internal/httpx/server.go Co-authored-by: Bartek Nowotarski --- services/horizon/internal/httpx/server.go | 1 - 1 file changed, 1 deletion(-) diff --git a/services/horizon/internal/httpx/server.go b/services/horizon/internal/httpx/server.go index 680ac09825..a319bd20ed 100644 --- a/services/horizon/internal/httpx/server.go +++ b/services/horizon/internal/httpx/server.go @@ -34,7 +34,6 @@ type Server struct { certFile, keyFile string } internalServer *http.Server - sync.RWMutex } func init() { From 49db819bda5fc4b66920f503bd86f92b04a2714b Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Fri, 28 Aug 2020 09:52:18 +0200 Subject: [PATCH 4/4] Address review feedback --- services/horizon/internal/app.go | 15 +++++++-- services/horizon/internal/httpx/server.go | 41 ++++++++++++----------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/services/horizon/internal/app.go b/services/horizon/internal/app.go index 6e5b9fd3b5..fadb8fa222 100644 --- a/services/horizon/internal/app.go +++ b/services/horizon/internal/app.go @@ -449,7 +449,7 @@ func (a *App) init() error { // txsub.metrics initTxSubMetrics(a) - webConfig := &httpx.RouterConfig{ + routerConfig := httpx.RouterConfig{ DBSession: a.historyQ.Session, TxSubmitter: a.submitter, RateQuota: a.config.RateQuota, @@ -466,8 +466,17 @@ func (a *App) init() error { } var err error - a.webServer, err = httpx.NewServer( - webConfig, uint16(a.config.Port), a.config.TLSCert, a.config.TLSKey, uint16(a.config.AdminPort)) + config := httpx.ServerConfig{ + Port: uint16(a.config.Port), + AdminPort: uint16(a.config.AdminPort), + } + if a.config.TLSCert != "" && a.config.TLSKey != "" { + config.TLSConfig = &httpx.TLSConfig{ + CertPath: a.config.TLSCert, + KeyPath: a.config.TLSKey, + } + } + a.webServer, err = httpx.NewServer(config, routerConfig) if err != nil { return err } diff --git a/services/horizon/internal/httpx/server.go b/services/horizon/internal/httpx/server.go index a319bd20ed..d2865ca6e2 100644 --- a/services/horizon/internal/httpx/server.go +++ b/services/horizon/internal/httpx/server.go @@ -24,15 +24,22 @@ type ServerMetrics struct { RequestDurationSummary *prometheus.SummaryVec } -// Web contains the http server related fields for horizon: the Router, +type TLSConfig struct { + CertPath, KeyPath string +} +type ServerConfig struct { + Port uint16 + TLSConfig *TLSConfig + AdminPort uint16 +} + +// Server contains the http server related fields for horizon: the Router, // rate limiter, etc. type Server struct { - Router *Router - Metrics *ServerMetrics - server *http.Server - tlsFiles *struct { - certFile, keyFile string - } + Router *Router + Metrics *ServerMetrics + server *http.Server + config ServerConfig internalServer *http.Server } @@ -49,7 +56,7 @@ func init() { problem.RegisterError(db.ErrCancelled, hProblem.ServiceUnavailable) } -func NewServer(config *RouterConfig, port uint16, certFile, keyFile string, adminPort uint16) (*Server, error) { +func NewServer(serverConfig ServerConfig, routerConfig RouterConfig) (*Server, error) { sm := &ServerMetrics{ RequestDurationSummary: prometheus.NewSummaryVec( prometheus.SummaryOpts{ @@ -59,11 +66,11 @@ func NewServer(config *RouterConfig, port uint16, certFile, keyFile string, admi []string{"status", "route", "streaming", "method"}, ), } - router, err := NewRouter(config, sm) + router, err := NewRouter(&routerConfig, sm) if err != nil { return nil, err } - addr := fmt.Sprintf(":%d", port) + addr := fmt.Sprintf(":%d", serverConfig.Port) result := &Server{ Router: router, Metrics: sm, @@ -73,13 +80,9 @@ func NewServer(config *RouterConfig, port uint16, certFile, keyFile string, admi ReadTimeout: 5 * time.Second, }, } - if certFile != "" && keyFile != "" { - result.tlsFiles = &struct { - certFile, keyFile string - }{keyFile, certFile} - } - if adminPort != 0 { - adminAddr := fmt.Sprintf(":%d", adminPort) + + if serverConfig.AdminPort != 0 { + adminAddr := fmt.Sprintf(":%d", serverConfig.AdminPort) result.internalServer = &http.Server{ Addr: adminAddr, Handler: result.Router.Internal, @@ -99,8 +102,8 @@ func (s *Server) Serve() error { } var err error - if s.tlsFiles != nil { - err = s.server.ListenAndServeTLS(s.tlsFiles.certFile, s.tlsFiles.keyFile) + if s.config.TLSConfig != nil { + err = s.server.ListenAndServeTLS(s.config.TLSConfig.CertPath, s.config.TLSConfig.KeyPath) } else { err = s.server.ListenAndServe() }