From ce8284c02716e5b8781b6d6dd95586d2f8d0bef0 Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Wed, 15 Jun 2022 07:56:15 -0400
Subject: [PATCH] p2p: accept should not abort on first error (backport #8759)
 (#8760)

---
 internal/p2p/router.go      | 14 +++---
 internal/p2p/router_test.go | 92 +++++++++++++------------------------
 2 files changed, 38 insertions(+), 68 deletions(-)

diff --git a/internal/p2p/router.go b/internal/p2p/router.go
index cee38e71a..b8a30b0f1 100644
--- a/internal/p2p/router.go
+++ b/internal/p2p/router.go
@@ -576,14 +576,14 @@ func (r *Router) acceptPeers(transport Transport) {
 	ctx := r.stopCtx()
 	for {
 		conn, err := transport.Accept()
-		switch err {
-		case nil:
-		case io.EOF:
-			r.logger.Debug("stopping accept routine", "transport", transport)
+		switch {
+		case errors.Is(err, io.EOF):
+			r.logger.Debug("stopping accept routine", "transport", transport, "err", "EOF")
 			return
-		default:
+		case err != nil:
+			// in this case we got an error from the net.Listener.
 			r.logger.Error("failed to accept connection", "transport", transport, "err", err)
-			return
+			continue
 		}
 
 		incomingIP := conn.RemoteEndpoint().IP
@@ -595,7 +595,7 @@ func (r *Router) acceptPeers(transport Transport) {
 				"close_err", closeErr,
 			)
 
-			return
+			continue
 		}
 
 		// Spawn a goroutine for the handshake, to avoid head-of-line blocking.
diff --git a/internal/p2p/router_test.go b/internal/p2p/router_test.go
index 436e3f004..e8494fdf4 100644
--- a/internal/p2p/router_test.go
+++ b/internal/p2p/router_test.go
@@ -413,72 +413,42 @@ func TestRouter_AcceptPeers(t *testing.T) {
 	}
 }
 
-func TestRouter_AcceptPeers_Error(t *testing.T) {
-	t.Cleanup(leaktest.Check(t))
+func TestRouter_AcceptPeers_Errors(t *testing.T) {
+	for _, err := range []error{io.EOF} {
+		t.Run(err.Error(), func(t *testing.T) {
+			t.Cleanup(leaktest.Check(t))
 
-	// Set up a mock transport that returns an error, which should prevent
-	// the router from calling Accept again.
-	mockTransport := &mocks.Transport{}
-	mockTransport.On("String").Maybe().Return("mock")
-	mockTransport.On("Protocols").Return([]p2p.Protocol{"mock"})
-	mockTransport.On("Accept").Once().Return(nil, errors.New("boom"))
-	mockTransport.On("Close").Return(nil)
+			// Set up a mock transport that returns io.EOF once, which should prevent
+			// the router from calling Accept again.
+			mockTransport := &mocks.Transport{}
+			mockTransport.On("String").Maybe().Return("mock")
+			mockTransport.On("Accept", mock.Anything).Once().Return(nil, err)
+			mockTransport.On("Listen", mock.Anything).Return(nil).Maybe()
+			mockTransport.On("Close").Return(nil)
+			mockTransport.On("Protocols").Return([]p2p.Protocol{"mock"})
+			// Set up and start the router.
+			peerManager, err := p2p.NewPeerManager(selfID, dbm.NewMemDB(), p2p.PeerManagerOptions{})
+			require.NoError(t, err)
 
-	// Set up and start the router.
-	peerManager, err := p2p.NewPeerManager(selfID, dbm.NewMemDB(), p2p.PeerManagerOptions{})
-	require.NoError(t, err)
-	defer peerManager.Close()
+			router, err := p2p.NewRouter(
+				log.TestingLogger(),
+				p2p.NopMetrics(),
+				selfInfo,
+				selfKey,
+				peerManager,
+				[]p2p.Transport{mockTransport},
+				p2p.RouterOptions{},
+			)
+			require.NoError(t, err)
 
-	router, err := p2p.NewRouter(
-		log.TestingLogger(),
-		p2p.NopMetrics(),
-		selfInfo,
-		selfKey,
-		peerManager,
-		[]p2p.Transport{mockTransport},
-		p2p.RouterOptions{},
-	)
-	require.NoError(t, err)
+			require.NoError(t, router.Start())
+			time.Sleep(time.Second)
+			require.NoError(t, router.Stop())
 
-	require.NoError(t, router.Start())
-	time.Sleep(time.Second)
-	require.NoError(t, router.Stop())
+			mockTransport.AssertExpectations(t)
 
-	mockTransport.AssertExpectations(t)
-}
-
-func TestRouter_AcceptPeers_ErrorEOF(t *testing.T) {
-	t.Cleanup(leaktest.Check(t))
-
-	// Set up a mock transport that returns io.EOF once, which should prevent
-	// the router from calling Accept again.
-	mockTransport := &mocks.Transport{}
-	mockTransport.On("String").Maybe().Return("mock")
-	mockTransport.On("Protocols").Return([]p2p.Protocol{"mock"})
-	mockTransport.On("Accept").Once().Return(nil, io.EOF)
-	mockTransport.On("Close").Return(nil)
-
-	// Set up and start the router.
-	peerManager, err := p2p.NewPeerManager(selfID, dbm.NewMemDB(), p2p.PeerManagerOptions{})
-	require.NoError(t, err)
-	defer peerManager.Close()
-
-	router, err := p2p.NewRouter(
-		log.TestingLogger(),
-		p2p.NopMetrics(),
-		selfInfo,
-		selfKey,
-		peerManager,
-		[]p2p.Transport{mockTransport},
-		p2p.RouterOptions{},
-	)
-	require.NoError(t, err)
-
-	require.NoError(t, router.Start())
-	time.Sleep(time.Second)
-	require.NoError(t, router.Stop())
-
-	mockTransport.AssertExpectations(t)
+		})
+	}
 }
 
 func TestRouter_AcceptPeers_HeadOfLineBlocking(t *testing.T) {