From 40cf0107d65c82eeef7ee70838cbe6887d88a355 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Fri, 6 Apr 2018 16:49:33 -0600 Subject: [PATCH] Ensure sig handler routine returns on shutdown, turn it off in most tests I noticed that when running the test suite, there would be a file server/log1.txt left. This file is created by one of the config reload test. Running this test individually was doing the proper cleanup. I noticed that the Signal test that was checking that files could be rotated was causing this side effect. It turns out that none of the config reload tests were disabling the signal handler (NoSigs=true), and since the go routine would be left running, running the TestSignalToReOpenLogFile() test would interact with an already finished test. I put a thread dump in handleSignals() to track all tests that were causing this function to start the go routine because NoSigs was not set to true. I fixed all those tests. At this time, there are only 2 tests that need to start the signal handler. I have also fixed the code so that the signal handler routine select on a server quitCh that is closed on shutdown so that this go routine exit and is waiting on using the grWG wait group. --- server/client_test.go | 1 + server/reload.go | 4 ++-- server/reload_test.go | 12 +++++++++++- server/route.go | 2 +- server/routes_test.go | 1 + server/server.go | 12 ++++++------ server/signal.go | 33 ++++++++++++++++++++------------- test/port_test.go | 2 +- 8 files changed, 43 insertions(+), 24 deletions(-) diff --git a/server/client_test.go b/server/client_test.go index 94ab88ce..3b77850a 100644 --- a/server/client_test.go +++ b/server/client_test.go @@ -698,6 +698,7 @@ func TestTLSCloseClientConnection(t *testing.T) { } opts.TLSTimeout = 100 opts.NoLog = true + opts.NoSigs = true s := RunServer(opts) defer s.Shutdown() diff --git a/server/reload.go b/server/reload.go index 330ee725..6ced93e6 100644 --- a/server/reload.go +++ b/server/reload.go @@ -570,8 +570,8 @@ func (s *Server) diffOptions(newOpts *Options) ([]option, error) { } } diffOpts = append(diffOpts, &clientAdvertiseOption{newValue: cliAdv}) - case "nolog": - // Ignore NoLog option since it's not parsed and only used in + case "nolog", "nosigs": + // Ignore NoLog ad NoSigs options since they are not parsed and only used in // testing. continue case "port": diff --git a/server/reload_test.go b/server/reload_test.go index 51fe7111..f3dddbe0 100644 --- a/server/reload_test.go +++ b/server/reload_test.go @@ -32,7 +32,7 @@ import ( // Ensure Reload returns an error when attempting to reload a server that did // not start with a config file. func TestConfigReloadNoConfigFile(t *testing.T) { - server := New(&Options{}) + server := New(&Options{NoSigs: true}) loaded := server.ConfigTime() if server.Reload() == nil { t.Fatal("Expected Reload to return an error") @@ -69,6 +69,7 @@ func TestConfigReloadUnsupported(t *testing.T) { Host: "localhost", Port: -1, }, + NoSigs: true, } processOptions(golden) @@ -158,6 +159,7 @@ func TestConfigReloadInvalidConfig(t *testing.T) { Host: "localhost", Port: -1, }, + NoSigs: true, } processOptions(golden) @@ -224,6 +226,7 @@ func TestConfigReload(t *testing.T) { Host: "localhost", Port: server.ClusterAddr().Port, }, + NoSigs: true, } processOptions(golden) @@ -1306,6 +1309,7 @@ func TestConfigReloadClusterRoutes(t *testing.T) { t.Fatalf("Error processing config file: %v", err) } srvcOpts.NoLog = true + srvcOpts.NoSigs = true srvc := RunServer(srvcOpts) defer srvc.Shutdown() @@ -1410,6 +1414,7 @@ func TestConfigReloadClusterAdvertise(t *testing.T) { t.Fatalf("Error processing config file: %v", err) } opts.NoLog = true + opts.NoSigs = true s := RunServer(opts) defer s.Shutdown() @@ -1491,6 +1496,7 @@ func TestConfigReloadClusterNoAdvertise(t *testing.T) { t.Fatalf("Error processing config file: %v", err) } opts.NoLog = true + opts.NoSigs = true s := RunServer(opts) defer s.Shutdown() @@ -1545,6 +1551,7 @@ func TestConfigReloadClientAdvertise(t *testing.T) { stackFatalf(t, "Error processing config file: %v", err) } opts.NoLog = true + opts.NoSigs = true s := RunServer(opts) defer s.Shutdown() @@ -1767,11 +1774,13 @@ func TestConfigReloadRotateFiles(t *testing.T) { func runServerWithSymlinkConfig(t *testing.T, symlinkName, configName string) (*Server, *Options, string) { opts, config := newOptionsWithSymlinkConfig(t, symlinkName, configName) opts.NoLog = true + opts.NoSigs = true return RunServer(opts), opts, config } func newServerWithSymlinkConfig(t *testing.T, symlinkName, configName string) (*Server, *Options, string) { opts, config := newOptionsWithSymlinkConfig(t, symlinkName, configName) + opts.NoSigs = true return New(opts), opts, config } @@ -1786,6 +1795,7 @@ func newOptionsWithSymlinkConfig(t *testing.T, symlinkName, configName string) ( if err != nil { t.Fatalf("Error processing config file: %v", err) } + opts.NoSigs = true return opts, config } diff --git a/server/route.go b/server/route.go index dc88bfcd..6cc18aad 100644 --- a/server/route.go +++ b/server/route.go @@ -812,7 +812,7 @@ func (s *Server) connectToRoute(rURL *url.URL, tryForEver bool) { } } select { - case <-s.rcQuit: + case <-s.quitCh: return case <-time.After(DEFAULT_ROUTE_CONNECT): continue diff --git a/server/routes_test.go b/server/routes_test.go index 18846cce..a3448f66 100644 --- a/server/routes_test.go +++ b/server/routes_test.go @@ -542,6 +542,7 @@ func TestTLSChainedSolicitWorks(t *testing.T) { func TestRouteTLSHandshakeError(t *testing.T) { optsSeed, _ := ProcessConfigFile("./configs/seed_tls.conf") optsSeed.NoLog = true + optsSeed.NoSigs = true srvSeed := RunServer(optsSeed) defer srvSeed.Shutdown() diff --git a/server/server.go b/server/server.go index a22c73c3..ee13809e 100644 --- a/server/server.go +++ b/server/server.go @@ -83,7 +83,7 @@ type Server struct { routeListener net.Listener routeInfo Info routeInfoJSON []byte - rcQuit chan bool + quitCh chan struct{} grMu sync.Mutex grTmpClients map[uint64]*client grRunning bool @@ -173,9 +173,9 @@ func New(opts *Options) *Server { s.routes = make(map[uint64]*client) s.remotes = make(map[string]*client) - // Used to kick out all of the route - // connect Go routines. - s.rcQuit = make(chan bool) + // Used to kick out all go routines possibly waiting on server + // to shutdown. + s.quitCh = make(chan struct{}) // Used to setup Authorization. s.configureAuthorization() @@ -381,8 +381,8 @@ func (s *Server) Shutdown() { s.profiler.Close() } - // Release the solicited routes connect go routines. - close(s.rcQuit) + // Release go routines that wait on that channel + close(s.quitCh) s.mu.Unlock() diff --git a/server/signal.go b/server/signal.go index d88bd81f..6a432f7c 100644 --- a/server/signal.go +++ b/server/signal.go @@ -37,21 +37,28 @@ func (s *Server) handleSignals() { signal.Notify(c, syscall.SIGINT, syscall.SIGUSR1, syscall.SIGHUP) + s.grWG.Add(1) go func() { - for sig := range c { - s.Debugf("Trapped %q signal", sig) - switch sig { - case syscall.SIGINT: - s.Noticef("Server Exiting..") - os.Exit(0) - case syscall.SIGUSR1: - // File log re-open for rotating file logs. - s.ReOpenLogFile() - case syscall.SIGHUP: - // Config reload. - if err := s.Reload(); err != nil { - s.Errorf("Failed to reload server configuration: %s", err) + defer s.grWG.Done() + for { + select { + case sig := <-c: + s.Debugf("Trapped %q signal", sig) + switch sig { + case syscall.SIGINT: + s.Noticef("Server Exiting..") + os.Exit(0) + case syscall.SIGUSR1: + // File log re-open for rotating file logs. + s.ReOpenLogFile() + case syscall.SIGHUP: + // Config reload. + if err := s.Reload(); err != nil { + s.Errorf("Failed to reload server configuration: %s", err) + } } + case <-s.quitCh: + return } } }() diff --git a/test/port_test.go b/test/port_test.go index 07e9d6bb..1c750384 100644 --- a/test/port_test.go +++ b/test/port_test.go @@ -22,7 +22,7 @@ import ( ) func TestResolveRandomPort(t *testing.T) { - opts := &server.Options{Host: "127.0.0.1", Port: server.RANDOM_PORT} + opts := &server.Options{Host: "127.0.0.1", Port: server.RANDOM_PORT, NoSigs: true} s := RunServer(opts) defer s.Shutdown()