From ed864d598a7cfa56e925474d0553066eb217882d Mon Sep 17 00:00:00 2001 From: Carson Ip Date: Fri, 11 Oct 2024 11:11:16 +0100 Subject: [PATCH 1/3] Apply shutdown timeout to http server to limit reload delay Part of #14337 --- internal/beater/http.go | 4 ++-- internal/beater/server.go | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/beater/http.go b/internal/beater/http.go index 3c9993658a5..33e1ffbfac8 100644 --- a/internal/beater/http.go +++ b/internal/beater/http.go @@ -106,9 +106,9 @@ func (h *httpServer) start() error { return h.Serve(h.httpListener) } -func (h *httpServer) stop() { +func (h *httpServer) stop(ctx context.Context) { h.logger.Infof("Stop listening on: %s", h.Server.Addr) - if err := h.Shutdown(context.Background()); err != nil { + if err := h.Shutdown(ctx); err != nil { h.logger.Errorf("error stopping http server: %s", err.Error()) if err := h.Close(); err != nil { h.logger.Errorf("error closing http server: %s", err.Error()) diff --git a/internal/beater/server.go b/internal/beater/server.go index 397e21e5064..35efba8f483 100644 --- a/internal/beater/server.go +++ b/internal/beater/server.go @@ -222,10 +222,11 @@ func (s server) run(ctx context.Context) error { }) g.Go(func() error { <-ctx.Done() - // httpServer should stop before grpcServer to avoid a panic caused by placing a new connection into - // a closed grpc connection channel during shutdown. - // See https://github.com/elastic/gmux/issues/13 - s.httpServer.stop() + stopctx, cancel := context.WithTimeout(context.Background(), s.cfg.ShutdownTimeout) + defer cancel() + s.httpServer.stop(stopctx) + // FIXME: will httpServer.stop terminate the grpc connections, making grpcServer.GracefulStop a no-op? + // Will it be better to grpcServer.GracefulStop first as the gmux panic is fixed, so that GOAWAY is sent? s.grpcServer.GracefulStop() return nil }) From a700622f065347b8eceed2bb09bfa168c9511b05 Mon Sep 17 00:00:00 2001 From: Carson Ip Date: Fri, 11 Oct 2024 11:16:58 +0100 Subject: [PATCH 2/3] GracefulStop first --- internal/beater/server.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/beater/server.go b/internal/beater/server.go index 35efba8f483..973269cc8c7 100644 --- a/internal/beater/server.go +++ b/internal/beater/server.go @@ -222,12 +222,10 @@ func (s server) run(ctx context.Context) error { }) g.Go(func() error { <-ctx.Done() + s.grpcServer.GracefulStop() stopctx, cancel := context.WithTimeout(context.Background(), s.cfg.ShutdownTimeout) defer cancel() s.httpServer.stop(stopctx) - // FIXME: will httpServer.stop terminate the grpc connections, making grpcServer.GracefulStop a no-op? - // Will it be better to grpcServer.GracefulStop first as the gmux panic is fixed, so that GOAWAY is sent? - s.grpcServer.GracefulStop() return nil }) if err := g.Wait(); err != http.ErrServerClosed { From 21d0f48c049d03071dcb629ffd874459ebb6076d Mon Sep 17 00:00:00 2001 From: Carson Ip Date: Fri, 11 Oct 2024 11:29:19 +0100 Subject: [PATCH 3/3] Add changelog --- changelogs/head.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/changelogs/head.asciidoc b/changelogs/head.asciidoc index ab1a5e60bcd..c0c37664a00 100644 --- a/changelogs/head.asciidoc +++ b/changelogs/head.asciidoc @@ -7,6 +7,7 @@ https://github.com/elastic/apm-server/compare/8.15\...main[View commits] ==== Bug fixes - Track all bulk request response status codes {pull}13574[13574] +- Apply shutdown timeout to http server {pull}14339[14339] [float] ==== Breaking Changes