[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 nats-io#481
vissapra · May 25, 2017 · 773b25a · 773b25a
1 parent 1acdd23
commit 773b25a
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 66 deletions.
diff --git 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
@@ -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
@@ -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)
 	}
 }