Repository: nats-io/nats-server
Pull request #1504 (2020-07-06 23:18:24)

Fixed accept loops that could leave connections opened
This was discovered with the test TestLeafNodeWithGatewaysServerRestart
that was sometimes failing. Investigation showed that when cluster B
was shutdown, one of the server on A that had a connection from B
that just broke tried to reconnect (as part of reconnect retries of
implicit gateways) to a server in B that was in the process of shuting down.
The connection had been accepted but createGateway not called because
the server's running boolean had been set to false as part of the shutdown.
However, the connection was not closed so the server on A had a valid
connection to a dead server from cluster B. When the B cluster (now single
server) was restarted and a LeafNode connection connected to it, then
the gateway from B to A was created, that server on A did not create outbound
connection to that B server because it already had one (the zombie one).

So this PR strengthens the starting of accept loops and also make sure
that if a connection (all type of connections) is not accepted because
the server is shuting down, that connection is properly closed.

Since all accept loops had almost same code, made a generic function
that accept functions to call specific create connection functions.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>



Patch:
diff --git a/server/gateway.go b/server/gateway.go
index 036bbcc7..f095b632 100644
--- a/server/gateway.go
+++ b/server/gateway.go
@@ -398,10 +398,7 @@ func (g *srvGateway) rejectUnknown() bool {
 // the cluster to form and this server gathers gateway URLs for this
 // cluster in order to send that as part of the connect/info process.
 func (s *Server) startGateways() {
-	// Spin up the accept loop
-	ch := make(chan struct{})
-	go s.gatewayAcceptLoop(ch)
-	<-ch
+	s.startGatewayAcceptLoop()
 
 	// Delay start of creation of gateways to give a chance
 	// to the local cluster to form.
@@ -422,18 +419,9 @@ func (s *Server) startGateways() {
 	})
 }
 
-// This is the gateways accept loop. This runs as a go-routine.
-// The listen specification is resolved (if use of random port),
-// then a listener is started. After that, this routine enters
-// a loop (until the server is shutdown) accepting incoming
-// gateway connections.
-func (s *Server) gatewayAcceptLoop(ch chan struct{}) {
-	defer func() {
-		if ch != nil {
-			close(ch)
-		}
-	}()
-
+// This starts the gateway accept loop in a go routine, unless it
+// is detected that the server has already been shutdown.
+func (s *Server) startGatewayAcceptLoop() {
 	// Snapshot server options.
 	opts := s.getOpts()
 
@@ -442,9 +430,15 @@ func (s *Server) gatewayAcceptLoop(ch chan struct{}) {
 		port = 0
 	}
 
+	s.mu.Lock()
+	if s.shutdown {
+		s.mu.Unlock()
+		return
+	}
 	hp := net.JoinHostPort(opts.Gateway.Host, strconv.Itoa(port))
 	l, e := net.Listen("tcp", hp)
 	if e != nil {
+		s.mu.Unlock()
 		s.Fatalf("Error listening on gateway port: %d - %v", opts.Gateway.Port, e)
 		return
 	}
@@ -452,7 +446,6 @@ func (s *Server) gatewayAcceptLoop(ch chan struct{}) {
 	s.Noticef("Listening for gateways connections on %s",
 		net.JoinHostPort(opts.Gateway.Host, strconv.Itoa(l.Addr().(*net.TCPAddr).Port)))
 
-	s.mu.Lock()
 	tlsReq := opts.Gateway.TLSConfig != nil
 	authRequired := opts.Gateway.Username != ""
 	info := &Info{
@@ -497,28 +490,8 @@ func (s *Server) gatewayAcceptLoop(ch chan struct{}) {
 	if warn {
 		s.Warnf(gatewayTLSInsecureWarning)
 	}
+	go s.acceptConnections(l, "Gateway", func(conn net.Conn) { s.createGateway(nil, nil, conn) }, nil)
 	s.mu.Unlock()
-
-	// Let them know we are up
-	close(ch)
-	ch = nil
-
-	tmpDelay := ACCEPT_MIN_SLEEP
-
-	for s.isRunning() {
-		conn, err := l.Accept()
-		if err != nil {
-			tmpDelay = s.acceptError("Gateway", err, tmpDelay)
-			continue
-		}
-		tmpDelay = ACCEPT_MIN_SLEEP
-		s.startGoRoutine(func() {
-			s.createGateway(nil, nil, conn)
-			s.grWG.Done()
-		})
-	}
-	s.Debugf("Gateway accept loop exiting..")
-	s.done <- true
 }
 
 // Similar to setInfoHostPortAndGenerateJSON, but for gatewayInfo.
diff --git a/server/leafnode.go b/server/leafnode.go
index 69e20a45..d3dcade4 100644
--- a/server/leafnode.go
+++ b/server/leafnode.go
@@ -342,18 +342,9 @@ func (cfg *leafNodeCfg) saveUserPassword(u *url.URL) {
 	}
 }
 
-// This is the leafnode's accept loop. This runs as a go-routine.
-// The listen specification is resolved (if use of random port),
-// then a listener is started. After that, this routine enters
-// a loop (until the server is shutdown) accepting incoming
-// leaf node connections from remote servers.
-func (s *Server) leafNodeAcceptLoop(ch chan struct{}) {
-	defer func() {
-		if ch != nil {
-			close(ch)
-		}
-	}()
-
+// This starts the leafnode accept loop in a go routine, unless it
+// is detected that the server has already been shutdown.
+func (s *Server) startLeafNodeAcceptLoop() {
 	// Snapshot server options.
 	opts := s.getOpts()
 
@@ -362,9 +353,15 @@ func (s *Server) leafNodeAcceptLoop(ch chan struct{}) {
 		port = 0
 	}
 
+	s.mu.Lock()
+	if s.shutdown {
+		s.mu.Unlock()
+		return
+	}
 	hp := net.JoinHostPort(opts.LeafNode.Host, strconv.Itoa(port))
 	l, e := net.Listen("tcp", hp)
 	if e != nil {
+		s.mu.Unlock()
 		s.Fatalf("Error listening on leafnode port: %d - %v", opts.LeafNode.Port, e)
 		return
 	}
@@ -372,7 +369,6 @@ func (s *Server) leafNodeAcceptLoop(ch chan struct{}) {
 	s.Noticef("Listening for leafnode connections on %s",
 		net.JoinHostPort(opts.LeafNode.Host, strconv.Itoa(l.Addr().(*net.TCPAddr).Port)))
 
-	s.mu.Lock()
 	tlsRequired := opts.LeafNode.TLSConfig != nil
 	tlsVerify := tlsRequired && opts.LeafNode.TLSConfig.ClientAuth == tls.RequireAndVerifyClientCert
 	info := Info{
@@ -426,28 +422,8 @@ func (s *Server) leafNodeAcceptLoop(ch chan struct{}) {
 	if warn {
 		s.Warnf(leafnodeTLSInsecureWarning)
 	}
+	go s.acceptConnections(l, "Leafnode", func(conn net.Conn) { s.createLeafNode(conn, nil) }, nil)
 	s.mu.Unlock()
-
-	// Let them know we are up
-	close(ch)
-	ch = nil
-
-	tmpDelay := ACCEPT_MIN_SLEEP
-
-	for s.isRunning() {
-		conn, err := l.Accept()
-		if err != nil {
-			tmpDelay = s.acceptError("LeafNode", err, tmpDelay)
-			continue
-		}
-		tmpDelay = ACCEPT_MIN_SLEEP
-		s.startGoRoutine(func() {
-			s.createLeafNode(conn, nil)
-			s.grWG.Done()
-		})
-	}
-	s.Debugf("Leafnode accept loop exiting..")
-	s.done <- true
 }
 
 // RegEx to match a creds file with user JWT and Seed.
diff --git a/server/route.go b/server/route.go
index 645adcdc..dac469b3 100644
--- a/server/route.go
+++ b/server/route.go
@@ -1612,13 +1612,10 @@ func (s *Server) updateRouteSubscriptionMap(acc *Account, sub *subscription, del
 	}
 }
 
-func (s *Server) routeAcceptLoop(ch chan struct{}) {
-	defer func() {
-		if ch != nil {
-			close(ch)
-		}
-	}()
-
+// This starts the route accept loop in a go routine, unless it
+// is detected that the server has already been shutdown.
+// It will also start soliciting explicit routes.
+func (s *Server) startRouteAcceptLoop() {
 	// Snapshot server options.
 	opts := s.getOpts()
 
@@ -1629,7 +1626,15 @@ func (s *Server) routeAcceptLoop(ch chan struct{}) {
 		port = 0
 	}
 
-	s.Noticef("Cluster name is %s", s.ClusterName())
+	// This requires lock, so do this outside of may block.
+	clusterName := s.ClusterName()
+
+	s.mu.Lock()
+	if s.shutdown {
+		s.mu.Unlock()
+		return
+	}
+	s.Noticef("Cluster name is %s", clusterName)
 	if s.isClusterNameDynamic() {
 		s.Warnf("Cluster name was dynamically generated, consider setting one")
 	}
@@ -1637,13 +1642,13 @@ func (s *Server) routeAcceptLoop(ch chan struct{}) {
 	hp := net.JoinHostPort(opts.Cluster.Host, strconv.Itoa(port))
 	l, e := net.Listen("tcp", hp)
 	if e != nil {
+		s.mu.Unlock()
 		s.Fatalf("Error listening on router port: %d - %v", opts.Cluster.Port, e)
 		return
 	}
 	s.Noticef("Listening for route connections on %s",
 		net.JoinHostPort(opts.Cluster.Host, strconv.Itoa(l.Addr().(*net.TCPAddr).Port)))
 
-	s.mu.Lock()
 	proto := RouteProtoV2
 	// For tests, we want to be able to make this server behave
 	// as an older server so check this option to see if we should override
@@ -1713,28 +1718,14 @@ func (s *Server) routeAcceptLoop(ch chan struct{}) {
 	if tlsReq && opts.Cluster.TLSConfig.InsecureSkipVerify {
 		s.Warnf(clusterTLSInsecureWarning)
 	}
-	s.mu.Unlock()
 
-	// Let them know we are up
-	close(ch)
-	ch = nil
+	// Start the accept loop in a different go routine.
+	go s.acceptConnections(l, "Route", func(conn net.Conn) { s.createRoute(conn, nil) }, nil)
 
-	tmpDelay := ACCEPT_MIN_SLEEP
+	// Solicit Routes if applicable. This will not block.
+	s.solicitRoutes(opts.Routes)
 
-	for s.isRunning() {
-		conn, err := l.Accept()
-		if err != nil {
-			tmpDelay = s.acceptError("Route", err, tmpDelay)
-			continue
-		}
-		tmpDelay = ACCEPT_MIN_SLEEP
-		s.startGoRoutine(func() {
-			s.createRoute(conn, nil)
-			s.grWG.Done()
-		})
-	}
-	s.Debugf("Router accept loop exiting..")
-	s.done <- true
+	s.mu.Unlock()
 }
 
 // Similar to setInfoHostPortAndGenerateJSON, but for routeInfo.
@@ -1766,13 +1757,9 @@ func (s *Server) StartRouting(clientListenReady chan struct{}) {
 	// and the possible ephemeral ports to be selected.
 	<-clientListenReady
 
-	// Spin up the accept loop
-	ch := make(chan struct{})
-	go s.routeAcceptLoop(ch)
-	<-ch
+	// Start the accept loop and solicitation of explicit routes (if applicable)
+	s.startRouteAcceptLoop()
 
-	// Solicit Routes if needed.
-	s.solicitRoutes(s.getOpts().Routes)
 }
 
 func (s *Server) reConnectToRoute(rURL *url.URL, rtype RouteType) {
diff --git a/server/server.go b/server/server.go
index c5f7de10..9e6edd8d 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1390,12 +1390,9 @@ func (s *Server) Start() {
 
 	// Start up listen if we want to accept leaf node connections.
 	if opts.LeafNode.Port != 0 {
-		// Spin up the accept loop if needed
-		ch := make(chan struct{})
-		go s.leafNodeAcceptLoop(ch)
-		// This ensure that we have resolved or assigned the advertise
-		// address for the leafnode listener. We need that in StartRouting().
-		<-ch
+		// Will resolve or assign the advertise address for the leafnode listener.
+		// We need that in StartRouting().
+		s.startLeafNodeAcceptLoop()
 	}
 
 	// Solicit remote servers for leaf node connections.
@@ -1403,6 +1400,15 @@ func (s *Server) Start() {
 		s.solicitLeafNodeRemotes(opts.LeafNode.Remotes)
 	}
 
+	// TODO (ik): I wanted to refactor this by starting the client
+	// accept loop first, that is, it would resolve listen spec
+	// in place, but start the accept-for-loop in a different go
+	// routine. This would get rid of the synchronization between
+	// this function and StartRouting, which I also would have wanted
+	// to refactor, but both AcceptLoop() and StartRouting() have
+	// been exported and not sure if that would break users using them.
+	// We could mark them as deprecated and remove in a release or two...
+
 	// The Routing routine needs to wait for the client listen
 	// port to be opened and potential ephemeral port selected.
 	clientListenReady := make(chan struct{})
@@ -1596,9 +1602,16 @@ func (s *Server) AcceptLoop(clr chan struct{}) {
 	// Snapshot server options.
 	opts := s.getOpts()
 
+	// Setup state that can enable shutdown
+	s.mu.Lock()
+	if s.shutdown {
+		s.mu.Unlock()
+		return
+	}
 	hp := net.JoinHostPort(opts.Host, strconv.Itoa(opts.Port))
 	l, e := net.Listen("tcp", hp)
 	if e != nil {
+		s.mu.Unlock()
 		s.Fatalf("Error listening on port: %s, %q", hp, e)
 		return
 	}
@@ -1613,9 +1626,6 @@ func (s *Server) AcceptLoop(clr chan struct{}) {
 	s.Noticef("Server id is %s", s.info.ID)
 	s.Noticef("Server is ready")
 
-	// Setup state that can enable shutdown
-	s.mu.Lock()
-
 	// If server was started with RANDOM_PORT (-1), opts.Port would be equal
 	// to 0 at the beginning this function. So we need to get the actual port
 	if opts.Port == 0 {
@@ -1635,33 +1645,48 @@ func (s *Server) AcceptLoop(clr chan struct{}) {
 	// Keep track of client connect URLs. We may need them later.
 	s.clientConnectURLs = s.getClientConnectURLs()
 	s.listener = l
+
+	go s.acceptConnections(l, "Client", func(conn net.Conn) { s.createClient(conn, nil) },
+		func(_ error) bool {
+			if s.isLameDuckMode() {
+				// Signal that we are not accepting new clients
+				s.ldmCh <- true
+				// Now wait for the Shutdown...
+				<-s.quitCh
+				return true
+			}
+			return false
+		})
 	s.mu.Unlock()
 
 	// Let the caller know that we are ready
 	close(clr)
 	clr = nil
+}
 
+func (s *Server) acceptConnections(l net.Listener, acceptName string, createFunc func(conn net.Conn), errFunc func(err error) bool) {
 	tmpDelay := ACCEPT_MIN_SLEEP
 
-	for s.isRunning() {
+	for {
 		conn, err := l.Accept()
 		if err != nil {
-			if s.isLameDuckMode() {
-				// Signal that we are not accepting new clients
-				s.ldmCh <- true
-				// Now wait for the Shutdown...
-				<-s.quitCh
+			if errFunc != nil && errFunc(err) {
 				return
 			}
-			tmpDelay = s.acceptError("Client", err, tmpDelay)
+			if tmpDelay = s.acceptError(acceptName, err, tmpDelay); tmpDelay < 0 {
+				break
+			}
 			continue
 		}
 		tmpDelay = ACCEPT_MIN_SLEEP
-		s.startGoRoutine(func() {
-			s.createClient(conn, nil)
+		if !s.startGoRoutine(func() {
+			createFunc(conn)
 			s.grWG.Done()
-		})
+		}) {
+			conn.Close()
+		}
 	}
+	s.Debugf(acceptName + " accept loop exiting..")
 	s.done <- true
 }
 
@@ -1699,13 +1724,20 @@ func (s *Server) StartProfiler() {
 		port = 0
 	}
 
+	s.mu.Lock()
+	if s.shutdown {
+		s.mu.Unlock()
+		return
+	}
 	hp := net.JoinHostPort(opts.Host, strconv.Itoa(port))
 
 	l, err := net.Listen("tcp", hp)
 	s.Noticef("profiling port: %d", l.Addr().(*net.TCPAddr).Port)
 
 	if err != nil {
+		s.mu.Unlock()
 		s.Fatalf("error starting profiler: %s", err)
+		return
 	}
 
 	srv := &http.Server{
@@ -1713,11 +1745,8 @@ func (s *Server) StartProfiler() {
 		Handler:        http.DefaultServeMux,
 		MaxHeaderBytes: 1 << 20,
 	}
-
-	s.mu.Lock()
 	s.profiler = l
 	s.profilingServer = srv
-	s.mu.Unlock()
 
 	// Enable blocking profile
 	runtime.SetBlockProfileRate(1)
@@ -1736,6 +1765,7 @@ func (s *Server) StartProfiler() {
 		srv.Close()
 		s.done <- true
 	}()
+	s.mu.Unlock()
 }
 
 // StartHTTPMonitoring will enable the HTTP monitoring port.
@@ -1996,6 +2026,14 @@ func (s *Server) createClient(conn net.Conn, ws *websocket) *client {
 	// to bail out now otherwise the readLoop started down there would not
 	// be interrupted. Skip also if in lame duck mode.
 	if !s.running || s.ldm {
+		// There are some tests that create a server but don't start it,
+		// and use "async" clients and perform the parsing manually. Such
+		// clients would branch here (since server is not running). However,
+		// when a server was really running and has been shutdown, we must
+		// close this connection.
+		if s.shutdown {
+			conn.Close()
+		}
 		s.mu.Unlock()
 		return c
 	}
@@ -2471,13 +2509,16 @@ func (s *Server) Name() string {
 	return s.info.Name
 }
 
-func (s *Server) startGoRoutine(f func()) {
+func (s *Server) startGoRoutine(f func()) bool {
+	var started bool
 	s.grMu.Lock()
 	if s.grRunning {
 		s.grWG.Add(1)
 		go f()
+		started = true
 	}
 	s.grMu.Unlock()
+	return started
 }
 
 func (s *Server) numClosedConns() int {
@@ -2973,20 +3014,24 @@ func (s *Server) sendLDMToClients() {
 // delay and double it, but cap it to ACCEPT_MAX_SLEEP. The sleep is
 // interrupted if the server is shutdown.
 // An error message is displayed depending on the type of error.
-// Returns the new (or unchanged) delay.
+// Returns the new (or unchanged) delay, or a negative value if the
+// server has been or is being shutdown.
 func (s *Server) acceptError(acceptName string, err error, tmpDelay time.Duration) time.Duration {
+	if !s.isRunning() {
+		return -1
+	}
 	if ne, ok := err.(net.Error); ok && ne.Temporary() {
 		s.Errorf("Temporary %s Accept Error(%v), sleeping %dms", acceptName, ne, tmpDelay/time.Millisecond)
 		select {
 		case <-time.After(tmpDelay):
 		case <-s.quitCh:
-			return tmpDelay
+			return -1
 		}
 		tmpDelay *= 2
 		if tmpDelay > ACCEPT_MAX_SLEEP {
 			tmpDelay = ACCEPT_MAX_SLEEP
 		}
-	} else if s.isRunning() {
+	} else {
 		s.Errorf("%s Accept error: %v", acceptName, err)
 	}
 	return tmpDelay
diff --git a/server/websocket.go b/server/websocket.go
index 8c167830..41f64f6d 100644
--- a/server/websocket.go
+++ b/server/websocket.go
@@ -82,6 +82,9 @@ var decompressorPool sync.Pool
 // From https://tools.ietf.org/html/rfc6455#section-1.3
 var wsGUID = []byte("258EAFA5-E914-47DA-95CA-C5AB0DC85B11")
 
+// Can be set for tests
+var testWebsocketAllowNonTLS = false
+
 type websocket struct {
 	frames     net.Buffers
 	fs         int64
@@ -732,7 +735,7 @@ func validateWebsocketOptions(o *Options) error {
 		return nil
 	}
 	// Enforce TLS...
-	if wo.TLSConfig == nil {
+	if !testWebsocketAllowNonTLS && wo.TLSConfig == nil {
 		return errors.New("websocket requires TLS configuration")
 	}
 	// Make sure that allowed origins, if specified, can be parsed.
@@ -832,6 +835,11 @@ func (s *Server) startWebsocketServer() {
 	// that we expect users to send JWTs with bearer tokens and we want to
 	// avoid the possibility of it being "intercepted".
 
+	s.mu.Lock()
+	if s.shutdown {
+		s.mu.Unlock()
+		return
+	}
 	if o.TLSConfig != nil {
 		proto = "wss"
 		config := o.TLSConfig.Clone()
@@ -841,12 +849,12 @@ func (s *Server) startWebsocketServer() {
 		hl, err = net.Listen("tcp", hp)
 	}
 	if err != nil {
+		s.mu.Unlock()
 		s.Fatalf("Unable to listen for websocket connections: %v", err)
 		return
 	}
 	s.Noticef("Listening for websocket clients on %s://%s:%d", proto, o.Host, port)
 
-	s.mu.Lock()
 	s.websocket.tls = proto == "wss"
 	if port == 0 {
 		s.opts.Websocket.Port = hl.Addr().(*net.TCPAddr).Port
@@ -875,11 +883,7 @@ func (s *Server) startWebsocketServer() {
 	}
 	s.websocket.server = hs
 	s.websocket.listener = hl
-	s.mu.Unlock()
-
-	s.startGoRoutine(func() {
-		defer s.grWG.Done()
-
+	go func() {
 		if err := hs.Serve(hl); err != http.ErrServerClosed {
 			s.Fatalf("websocket listener error: %v", err)
 		}
@@ -891,7 +895,8 @@ func (s *Server) startWebsocketServer() {
 			return
 		}
 		s.done <- true
-	})
+	}()
+	s.mu.Unlock()
 }
 
 type wsCaptureHTTPServerLog struct {


Test patch:
diff --git a/server/gateway_test.go b/server/gateway_test.go
index a61df3dd..b6f4f823 100644
--- a/server/gateway_test.go
+++ b/server/gateway_test.go
@@ -715,32 +715,40 @@ func TestGatewaySolicitShutdown(t *testing.T) {
 	}
 }
 
-func TestGatewayListenError(t *testing.T) {
-	o2 := testDefaultOptionsForGateway("B")
-	s2 := runGatewayServer(o2)
-	defer s2.Shutdown()
-
-	o1 := testDefaultOptionsForGateway("A")
-	o1.Gateway.Port = s2.GatewayAddr().Port
-	s1 := New(o1)
-	defer s1.Shutdown()
+func testFatalErrorOnStart(t *testing.T, o *Options, errTxt string) {
+	t.Helper()
+	s := New(o)
+	defer s.Shutdown()
+	l := &captureFatalLogger{fatalCh: make(chan string, 1)}
+	s.SetLogger(l, false, false)
 	wg := sync.WaitGroup{}
 	wg.Add(1)
 	go func() {
-		s1.Start()
+		s.Start()
 		wg.Done()
 	}()
-	// We call Fatalf on listen error, but since there is no actual logger
-	// associated, we just check that the listener is not created.
-	time.Sleep(100 * time.Millisecond)
-	addr := s1.GatewayAddr()
-	if addr != nil {
-		t.Fatal("Listener should not have been created")
+	select {
+	case e := <-l.fatalCh:
+		if !strings.Contains(e, errTxt) {
+			t.Fatalf("Error should contain %q, got %s", errTxt, e)
+		}
+	case <-time.After(time.Second):
+		t.Fatal("Should have got a fatal error")
 	}
-	s1.Shutdown()
+	s.Shutdown()
 	wg.Wait()
 }
 
+func TestGatewayListenError(t *testing.T) {
+	o2 := testDefaultOptionsForGateway("B")
+	s2 := runGatewayServer(o2)
+	defer s2.Shutdown()
+
+	o1 := testDefaultOptionsForGateway("A")
+	o1.Gateway.Port = s2.GatewayAddr().Port
+	testFatalErrorOnStart(t, o1, "listening on")
+}
+
 func TestGatewayWithListenToAny(t *testing.T) {
 	confB1 := createConfFile(t, []byte(`
 		listen: "127.0.0.1:-1"
@@ -874,23 +882,7 @@ func TestGatewayAdvertise(t *testing.T) {
 func TestGatewayAdvertiseErr(t *testing.T) {
 	o1 := testDefaultOptionsForGateway("A")
 	o1.Gateway.Advertise = "wrong:address"
-	s1 := New(o1)
-	defer s1.Shutdown()
-	wg := sync.WaitGroup{}
-	wg.Add(1)
-	go func() {
-		s1.Start()
-		wg.Done()
-	}()
-	// We call Fatalf on listen error, but since there is no actual logger
-	// associated, we just check that the listener is not created.
-	time.Sleep(100 * time.Millisecond)
-	addr := s1.GatewayAddr()
-	if addr != nil {
-		t.Fatal("Listener should not have been created")
-	}
-	s1.Shutdown()
-	wg.Wait()
+	testFatalErrorOnStart(t, o1, "Gateway.Advertise")
 }
 
 func TestGatewayAuth(t *testing.T) {
diff --git a/server/routes_test.go b/server/routes_test.go
index d7b7cc06..5347e180 100644
--- a/server/routes_test.go
+++ b/server/routes_test.go
@@ -147,29 +147,7 @@ func TestClusterAdvertiseErrorOnStartup(t *testing.T) {
 	opts := DefaultOptions()
 	// Set invalid address
 	opts.Cluster.Advertise = "addr:::123"
-	s := New(opts)
-	defer s.Shutdown()
-	dl := &DummyLogger{}
-	s.SetLogger(dl, false, false)
-
-	// Start will keep running, so start in a go-routine.
-	wg := &sync.WaitGroup{}
-	wg.Add(1)
-	go func() {
-		s.Start()
-		wg.Done()
-	}()
-	checkFor(t, 2*time.Second, 15*time.Millisecond, func() error {
-		dl.Lock()
-		msg := dl.msg
-		dl.Unlock()
-		if strings.Contains(msg, "Cluster.Advertise") {
-			return nil
-		}
-		return fmt.Errorf("Did not get expected error, got %v", msg)
-	})
-	s.Shutdown()
-	wg.Wait()
+	testFatalErrorOnStart(t, opts, "Cluster.Advertise")
 }
 
 func TestClientAdvertise(t *testing.T) {
diff --git a/server/server_test.go b/server/server_test.go
index bc095734..1c0b13b8 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -399,21 +399,7 @@ func TestClientAdvertiseErrorOnStartup(t *testing.T) {
 	opts := DefaultOptions()
 	// Set invalid address
 	opts.ClientAdvertise = "addr:::123"
-	s := New(opts)
-	defer s.Shutdown()
-	l := &captureFatalLogger{fatalCh: make(chan string, 1)}
-	s.SetLogger(l, false, false)
-
-	// Expect this to return due to failure
-	s.Start()
-	select {
-	case msg := <-l.fatalCh:
-		if !strings.Contains(msg, "ClientAdvertise") {
-			t.Fatalf("Unexpected error: %v", msg)
-		}
-	case <-time.After(time.Second):
-		t.Fatalf("Should have failed to start")
-	}
+	testFatalErrorOnStart(t, opts, "ClientAdvertise")
 }
 
 func TestNoDeadlockOnStartFailure(t *testing.T) {
@@ -428,7 +414,16 @@ func TestNoDeadlockOnStartFailure(t *testing.T) {
 
 	// This should return since it should fail to start a listener
 	// on x.x.x.x:4222
-	s.Start()
+	ch := make(chan struct{})
+	go func() {
+		s.Start()
+		close(ch)
+	}()
+	select {
+	case <-ch:
+	case <-time.After(time.Second):
+		t.Fatalf("Start() should have returned due to failure to start listener")
+	}
 
 	// We should be able to shutdown
 	s.Shutdown()
@@ -1182,6 +1177,175 @@ func TestAcceptError(t *testing.T) {
 	if dur := time.Since(start); dur >= ACCEPT_MAX_SLEEP {
 		t.Fatalf("Shutdown took too long: %v", dur)
 	}
+	wg.Wait()
+	if d := s.acceptError("Test", ne, orgDelay); d >= 0 {
+		t.Fatalf("Expected delay to be negative, got %v", d)
+	}
+}
+
+func TestAcceptLoopsDoNotLeaveOpenedConn(t *testing.T) {
+	testWebsocketAllowNonTLS = true
+	defer func() { testWebsocketAllowNonTLS = false }()
+
+	for _, test := range []struct {
+		name string
+		url  func(o *Options) (string, int)
+	}{
+		{"client", func(o *Options) (string, int) { return o.Host, o.Port }},
+		{"route", func(o *Options) (string, int) { return o.Cluster.Host, o.Cluster.Port }},
+		{"gateway", func(o *Options) (string, int) { return o.Gateway.Host, o.Gateway.Port }},
+		{"leafnode", func(o *Options) (string, int) { return o.LeafNode.Host, o.LeafNode.Port }},
+		{"websocket", func(o *Options) (string, int) { return o.Websocket.Host, o.Websocket.Port }},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			o := DefaultOptions()
+			o.DisableShortFirstPing = true
+			o.Accounts = []*Account{NewAccount("$SYS")}
+			o.SystemAccount = "$SYS"
+			o.Cluster.Name = "abc"
+			o.Cluster.Host = "127.0.0.1"
+			o.Cluster.Port = -1
+			o.Gateway.Name = "abc"
+			o.Gateway.Host = "127.0.0.1"
+			o.Gateway.Port = -1
+			o.LeafNode.Host = "127.0.0.1"
+			o.LeafNode.Port = -1
+			o.Websocket.Host = "127.0.0.1"
+			o.Websocket.Port = -1
+			o.Websocket.HandshakeTimeout = 1
+			s := RunServer(o)
+			defer s.Shutdown()
+
+			host, port := test.url(o)
+			url := fmt.Sprintf("%s:%d", host, port)
+			var conns []net.Conn
+
+			wg := sync.WaitGroup{}
+			wg.Add(1)
+			done := make(chan struct{}, 1)
+			go func() {
+				defer wg.Done()
+				for {
+					c, err := net.Dial("tcp", url)
+					if err != nil {
+						return
+					}
+					conns = append(conns, c)
+					select {
+					case <-done:
+						return
+					default:
+					}
+				}
+			}()
+			time.Sleep(15 * time.Millisecond)
+			s.Shutdown()
+			close(done)
+			wg.Wait()
+			for _, c := range conns {
+				c.SetReadDeadline(time.Now().Add(2 * time.Second))
+				br := bufio.NewReader(c)
+				// Read INFO for connections that were accepted
+				_, _, err := br.ReadLine()
+				if err == nil {
+					// After that, the connection should be closed,
+					// so we should get an error here.
+					_, _, err = br.ReadLine()
+				}
+				// We expect an io.EOF or any other error indicating the use of a closed
+				// connection, but we should not get the timeout error.
+				if ne, ok := err.(net.Error); ok && ne.Timeout() {
+					err = nil
+				}
+				if err == nil {
+					var buf [10]byte
+					c.SetDeadline(time.Now().Add(2 * time.Second))
+					c.Write([]byte("C"))
+					_, err = c.Read(buf[:])
+					if ne, ok := err.(net.Error); ok && ne.Timeout() {
+						err = nil
+					}
+				}
+				if err == nil {
+					t.Fatalf("Connection should have been closed")
+				}
+				c.Close()
+			}
+		})
+	}
+}
+
+func TestServerShutdownDuringStart(t *testing.T) {
+	testWebsocketAllowNonTLS = true
+	defer func() { testWebsocketAllowNonTLS = false }()
+
+	o := DefaultOptions()
+	o.DisableShortFirstPing = true
+	o.Accounts = []*Account{NewAccount("$SYS")}
+	o.SystemAccount = "$SYS"
+	o.Cluster.Name = "abc"
+	o.Cluster.Host = "127.0.0.1"
+	o.Cluster.Port = -1
+	o.Gateway.Name = "abc"
+	o.Gateway.Host = "127.0.0.1"
+	o.Gateway.Port = -1
+	o.LeafNode.Host = "127.0.0.1"
+	o.LeafNode.Port = -1
+	o.Websocket.Host = "127.0.0.1"
+	o.Websocket.Port = -1
+	o.Websocket.HandshakeTimeout = 1
+
+	// We are going to test that if the server is shutdown
+	// while Start() runs (in this case, before), we don't
+	// start the listeners and therefore leave accept loops
+	// hanging.
+	s, err := NewServer(o)
+	if err != nil {
+		t.Fatalf("Error creating server: %v", err)
+	}
+	s.Shutdown()
+
+	// Start() should not block, but just in case, start in
+	// different go routine.
+	ch := make(chan struct{}, 1)
+	go func() {
+		s.Start()
+		close(ch)
+	}()
+	select {
+	case <-ch:
+	case <-time.After(time.Second):
+		t.Fatal("Start appear to have blocked after server was shutdown")
+	}
+	// Now make sure that none of the listeners have been created
+	listeners := []string{}
+	s.mu.Lock()
+	if s.listener != nil {
+		listeners = append(listeners, "client")
+	}
+	if s.routeListener != nil {
+		listeners = append(listeners, "route")
+	}
+	if s.gatewayListener != nil {
+		listeners = append(listeners, "gateway")
+	}
+	if s.leafNodeListener != nil {
+		listeners = append(listeners, "leafnode")
+	}
+	if s.websocket.listener != nil {
+		listeners = append(listeners, "websocket")
+	}
+	s.mu.Unlock()
+	if len(listeners) > 0 {
+		lst := ""
+		for i, l := range listeners {
+			if i > 0 {
+				lst += ", "
+			}
+			lst += l
+		}
+		t.Fatalf("Following listeners have been created: %s", lst)
+	}
 }
 
 type myDummyDNSResolver struct {
@@ -1324,6 +1488,9 @@ func TestInsecureSkipVerifyWarning(t *testing.T) {
 			s.Start()
 			wg.Done()
 		}()
+		if !s.ReadyForConnections(time.Second) {
+			t.Fatal("Unable to start the server")
+		}
 		select {
 		case w := <-l.warn:
 			if !strings.Contains(w, expectedWarn) {