Merge pull request #498 from nats-io/stop_http_server_on_shutdown

[FIXED] Ensure Shutdown() always stops http server

.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

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

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)
 	}
 }