From 773b25af852cdbddf5bf5b9c1f44a56d3802f537 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Thu, 25 May 2017 17:01:35 -0600 Subject: [PATCH] [FIXED] Shutdown stops http server when started manually In case one creates a server instance with New() and then starts the http server manually (s.StartHTTPMonitoring()), calling s.Shutdown() would not stop the http server because Shutdown() would return without doing anything if `running` was not true. This boolean was set to true only in `s.Start()`. Also added StartMonitoring() to perform the options check and selectively start http or https server to replace individual calls. This is useful for NATS Streaming server that will now be able to call s.StartMonitoring() without having to duplicate code about options checks and http server code. This is related to PR #481 --- .travis.yml | 4 +-- server/server.go | 66 +++++++++++++++++++++++---------------- test/monitor_test.go | 73 ++++++++++++++++++++++---------------------- 3 files changed, 77 insertions(+), 66 deletions(-) diff --git a/.travis.yml b/.travis.yml index fadb17e0..50217784 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,8 +1,8 @@ language: go go: - 1.6.4 -- 1.7.5 -- 1.8.1 +- 1.7.6 +- 1.8.3 install: - go get github.com/nats-io/go-nats - go get github.com/mattn/goveralls diff --git a/server/server.go b/server/server.go index 1495c90e..58c301ed 100644 --- a/server/server.go +++ b/server/server.go @@ -57,6 +57,7 @@ type Server struct { trace bool debug bool running bool + shutdown bool listener net.Listener clients map[uint64]*client routes map[uint64]*client @@ -223,26 +224,12 @@ func (s *Server) Start() { s.logPid() } - // Specifying both HTTP and HTTPS ports is a misconfiguration - if s.opts.HTTPPort != 0 && s.opts.HTTPSPort != 0 { - Fatalf("Can't specify both HTTP (%v) and HTTPs (%v) ports", s.opts.HTTPPort, s.opts.HTTPSPort) + // Start monitoring if needed + if err := s.StartMonitoring(); err != nil { + Fatalf("Can't start monitoring: %v", err) return } - // Start up the http server if needed. - if s.opts.HTTPPort != 0 { - s.StartHTTPMonitoring() - } - - // Start up the https server if needed. - if s.opts.HTTPSPort != 0 { - if s.opts.TLSConfig == nil { - Fatalf("TLS cert and key required for HTTPS") - return - } - s.StartHTTPSMonitoring() - } - // The Routing routine needs to wait for the client listen // port to be opened and potential ephemeral port selected. clientListenReady := make(chan struct{}) @@ -269,11 +256,11 @@ func (s *Server) Shutdown() { s.mu.Lock() // Prevent issues with multiple calls. - if !s.running { + if s.shutdown { s.mu.Unlock() return } - + s.shutdown = true s.running = false s.grMu.Lock() s.grRunning = false @@ -436,15 +423,35 @@ func (s *Server) StartProfiler() { } // StartHTTPMonitoring will enable the HTTP monitoring port. +// DEPRECATED: Should use StartMonitoring. func (s *Server) StartHTTPMonitoring() { s.startMonitoring(false) } // StartHTTPSMonitoring will enable the HTTPS monitoring port. +// DEPRECATED: Should use StartMonitoring. func (s *Server) StartHTTPSMonitoring() { s.startMonitoring(true) } +// StartMonitoring starts the HTTP or HTTPs server if needed. +func (s *Server) StartMonitoring() error { + // Specifying both HTTP and HTTPS ports is a misconfiguration + if s.opts.HTTPPort != 0 && s.opts.HTTPSPort != 0 { + return fmt.Errorf("can't specify both HTTP (%v) and HTTPs (%v) ports", s.opts.HTTPPort, s.opts.HTTPSPort) + } + var err error + if s.opts.HTTPPort != 0 { + err = s.startMonitoring(false) + } else if s.opts.HTTPSPort != 0 { + if s.opts.TLSConfig == nil { + return fmt.Errorf("TLS cert and key required for HTTPS") + } + err = s.startMonitoring(true) + } + return err +} + // HTTP endpoints const ( RootPath = "/" @@ -456,7 +463,7 @@ const ( ) // Start the monitoring server -func (s *Server) startMonitoring(secure bool) { +func (s *Server) startMonitoring(secure bool) error { // Used to track HTTP requests s.httpReqStats = map[string]uint64{ @@ -467,25 +474,27 @@ func (s *Server) startMonitoring(secure bool) { SubszPath: 0, } - var hp string - var err error + var ( + hp string + err error + httpListener net.Listener + ) if secure { hp = net.JoinHostPort(s.opts.HTTPHost, strconv.Itoa(s.opts.HTTPSPort)) Noticef("Starting https monitor on %s", hp) config := util.CloneTLSConfig(s.opts.TLSConfig) config.ClientAuth = tls.NoClientCert - s.http, err = tls.Listen("tcp", hp, config) + httpListener, err = tls.Listen("tcp", hp, config) } else { hp = net.JoinHostPort(s.opts.HTTPHost, strconv.Itoa(s.opts.HTTPPort)) Noticef("Starting http monitor on %s", hp) - s.http, err = net.Listen("tcp", hp) + httpListener, err = net.Listen("tcp", hp) } if err != nil { - Fatalf("Can't listen to the monitor port: %v", err) - return + return fmt.Errorf("can't listen to the monitor port: %v", err) } mux := http.NewServeMux() @@ -513,17 +522,20 @@ func (s *Server) startMonitoring(secure bool) { MaxHeaderBytes: 1 << 20, } s.mu.Lock() + s.http = httpListener s.httpHandler = mux s.mu.Unlock() go func() { - srv.Serve(s.http) + srv.Serve(httpListener) srv.Handler = nil s.mu.Lock() s.httpHandler = nil s.mu.Unlock() s.done <- true }() + + return nil } // HTTPHandler returns the http.Handler object used to handle monitoring diff --git a/test/monitor_test.go b/test/monitor_test.go index 2b011ec5..e81e1c7f 100644 --- a/test/monitor_test.go +++ b/test/monitor_test.go @@ -650,10 +650,14 @@ func TestMonitorNoTLSConfig(t *testing.T) { opts.HTTPSPort = MONITOR_PORT s := server.New(&opts) defer s.Shutdown() + // Check with manually starting the monitoring, which should return an error + if err := s.StartMonitoring(); err == nil || !strings.Contains(err.Error(), "TLS") { + t.Fatalf("Expected error about missing TLS config, got %v", err) + } + // Also check by calling Start(), which should produce a fatal error dl := &dummyLogger{} s.SetLogger(dl, false, false) defer s.SetLogger(nil, false, false) - // This should produce a fatal error due to lack of TLS config s.Start() if !strings.Contains(dl.msg, "TLS") { t.Fatalf("Expected error about missing TLS config, got %v", dl.msg) @@ -670,35 +674,8 @@ func TestMonitorErrorOnListen(t *testing.T) { opts.HTTPPort = MONITOR_PORT s2 := server.New(&opts) defer s2.Shutdown() - dl := &dummyLogger{} - s2.SetLogger(dl, false, false) - defer s2.SetLogger(nil, false, false) - wg := &sync.WaitGroup{} - wg.Add(1) - go func() { - defer wg.Done() - // This will block until server is shutdown - s2.Start() - }() - // Wait for the error to be produced - timeout := time.Now().Add(3 * time.Second) - ok := false - for time.Now().Before(timeout) { - dl.Lock() - msg := dl.msg - dl.Unlock() - if msg == "" { - continue - } - if strings.Contains(msg, "listen") { - ok = true - break - } - } - s2.Shutdown() - wg.Wait() - if !ok { - t.Fatalf("Should have produced a fatal error") + if err := s2.StartMonitoring(); err == nil || !strings.Contains(err.Error(), "listen") { + t.Fatalf("Expected error about not able to start listener, got %v", err) } } @@ -710,12 +687,34 @@ func TestMonitorBothPortsConfigured(t *testing.T) { opts.HTTPSPort = MONITOR_PORT + 1 s := server.New(&opts) defer s.Shutdown() - dl := &dummyLogger{} - s.SetLogger(dl, false, false) - defer s.SetLogger(nil, false, false) - // This should produce a fatal error - s.Start() - if !strings.Contains(dl.msg, "specify both") { - t.Fatalf("Expected error about ports configured, got %v", dl.msg) + if err := s.StartMonitoring(); err == nil || !strings.Contains(err.Error(), "specify both") { + t.Fatalf("Expected error about ports configured, got %v", err) + } +} + +func TestMonitorStop(t *testing.T) { + resetPreviousHTTPConnections() + opts := DefaultTestOptions + opts.Port = CLIENT_PORT + opts.HTTPHost = "localhost" + opts.HTTPPort = MONITOR_PORT + url := fmt.Sprintf("http://%v:%d/", opts.HTTPHost, MONITOR_PORT) + // Create a server instance and start only the monitoring http server. + s := server.New(&opts) + if err := s.StartMonitoring(); err != nil { + t.Fatalf("Error starting monitoring: %v", err) + } + // Make sure http server is started + resp, err := http.Get(url + "varz") + if err != nil { + t.Fatalf("Error on http request: %v", err) + } + resp.Body.Close() + // Although the server itself was not started (we did not call s.Start()), + // Shutdown() should stop the http server. + s.Shutdown() + // HTTP request should now fail + if resp, err := http.Get(url + "varz"); err == nil { + t.Fatalf("Expected error: Got %+v\n", resp) } }