From ec3d8b590a4f69746fbc759aa0d0d4f7ad044d84 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Fri, 30 Apr 2021 10:55:59 +0200 Subject: [PATCH] Server: Implement timeout waiting for server to shut down (#33333) * tests: Undo cleanup in goroutine Signed-off-by: Arve Knudsen * Server: Implement timeout waiting for it to shut down Signed-off-by: Arve Knudsen --- pkg/cmd/grafana-server/main.go | 13 ++++++++++--- pkg/server/server.go | 18 ++++++++++++------ pkg/server/server_test.go | 15 ++++++++++++++- pkg/tests/testinfra/testinfra.go | 10 +++++++++- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/grafana-server/main.go b/pkg/cmd/grafana-server/main.go index cf0fdf58015..541f036652f 100644 --- a/pkg/cmd/grafana-server/main.go +++ b/pkg/cmd/grafana-server/main.go @@ -1,6 +1,7 @@ package main import ( + "context" "errors" "flag" "fmt" @@ -156,7 +157,9 @@ func executeServer(configFile, homePath, pidFile, packaging string, traceDiagnos return err } - go listenToSystemSignals(s) + ctx := context.Background() + + go listenToSystemSignals(ctx, s) if err := s.Run(); err != nil { code := s.ExitCode(err) @@ -179,7 +182,7 @@ func validPackaging(packaging string) string { return "unknown" } -func listenToSystemSignals(s *server.Server) { +func listenToSystemSignals(ctx context.Context, s *server.Server) { signalChan := make(chan os.Signal, 1) sighupChan := make(chan os.Signal, 1) @@ -193,7 +196,11 @@ func listenToSystemSignals(s *server.Server) { fmt.Fprintf(os.Stderr, "Failed to reload loggers: %s\n", err) } case sig := <-signalChan: - s.Shutdown(fmt.Sprintf("System signal: %s", sig)) + ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + if err := s.Shutdown(ctx, fmt.Sprintf("System signal: %s", sig)); err != nil { + fmt.Fprintf(os.Stderr, "Timed out waiting for server to shut down\n") + } return } } diff --git a/pkg/server/server.go b/pkg/server/server.go index dc3744aab60..b3053e5d7b0 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -222,17 +222,23 @@ func (s *Server) Run() error { // Shutdown initiates Grafana graceful shutdown. This shuts down all // running background services. Since Run blocks Shutdown supposed to // be run from a separate goroutine. -func (s *Server) Shutdown(reason string) { +func (s *Server) Shutdown(ctx context.Context, reason string) error { + var err error s.shutdownOnce.Do(func() { s.log.Info("Shutdown started", "reason", reason) // Call cancel func to stop services. s.shutdownFn() - // Can introduce termination timeout here if needed over incoming Context, - // but this will require changing Shutdown method signature to accept context - // and return an error - caller can exit with code > 0 then. - // I.e. sth like server.Shutdown(context.WithTimeout(...), reason). - <-s.shutdownFinished + // Wait for server to shut down + select { + case <-s.shutdownFinished: + s.log.Debug("Finished waiting for server to shut down") + case <-ctx.Done(): + s.log.Warn("Timed out while waiting for server to shut down") + err = fmt.Errorf("timeout waiting for shutdown") + } }) + + return err } // ExitCode returns an exit code for a given error. diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 0e112165f22..09b4c0c30f0 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "testing" + "time" "github.com/grafana/grafana/pkg/registry" @@ -83,6 +84,8 @@ func TestServer_Run_Error(t *testing.T) { } func TestServer_Shutdown(t *testing.T) { + ctx := context.Background() + s := testServer() services := []*registry.Descriptor{ { @@ -100,14 +103,24 @@ func TestServer_Shutdown(t *testing.T) { services: services, } + ch := make(chan error) + go func() { + defer close(ch) + // Wait until all services launched. for _, svc := range services { <-svc.Instance.(*testService).started } - s.Shutdown("test interrupt") + ctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + err := s.Shutdown(ctx, "test interrupt") + ch <- err }() err := s.Run() require.NoError(t, err) require.Zero(t, s.ExitCode(err)) + + err = <-ch + require.NoError(t, err) } diff --git a/pkg/tests/testinfra/testinfra.go b/pkg/tests/testinfra/testinfra.go index 9077f71435b..36b1642c2d1 100644 --- a/pkg/tests/testinfra/testinfra.go +++ b/pkg/tests/testinfra/testinfra.go @@ -1,6 +1,7 @@ package testinfra import ( + "context" "fmt" "io/ioutil" "net" @@ -9,6 +10,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/grafana/grafana/pkg/infra/fs" "github.com/grafana/grafana/pkg/models" @@ -25,6 +27,8 @@ import ( func StartGrafana(t *testing.T, grafDir, cfgPath string, sqlStore *sqlstore.SQLStore) string { t.Helper() + ctx := context.Background() + origSQLStore := registry.GetService(sqlstore.ServiceName) t.Cleanup(func() { registry.Register(origSQLStore) @@ -58,7 +62,11 @@ func StartGrafana(t *testing.T, grafDir, cfgPath string, sqlStore *sqlstore.SQLS } }() t.Cleanup(func() { - go server.Shutdown("test cleanup") + ctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + if err := server.Shutdown(ctx, "test cleanup"); err != nil { + t.Error("Timed out waiting on server to shut down") + } }) // Wait for Grafana to be ready