From f8dc5ec44f5ed855ed78c82fe03479eb2da22c19 Mon Sep 17 00:00:00 2001
From: Ekaterina Pavlova <ekt@morphbits.io>
Date: Wed, 21 Feb 2024 18:07:28 +0300
Subject: [PATCH] network: change server Start() behavior

Previously user should Start server in a separate goroutine. Now
separate goroutine is created inside the Start(). For normal server
operation, the caller should wait for Start to finish. Also, fixed
TestTryInitStateSync test which was exiting earlier than logs are
called.

Close #3112

Signed-off-by: Ekaterina Pavlova <ekt@morphbits.io>
---
 cli/server/server.go                     | 2 +-
 internal/testcli/executor.go             | 2 +-
 pkg/network/server.go                    | 4 ++--
 pkg/network/server_test.go               | 8 ++++----
 pkg/services/rpcsrv/subscription_test.go | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/cli/server/server.go b/cli/server/server.go
index f2670d599..d51b70159 100644
--- a/cli/server/server.go
+++ b/cli/server/server.go
@@ -498,7 +498,7 @@ func startServer(ctx *cli.Context) error {
 	rpcServer := rpcsrv.New(chain, cfg.ApplicationConfiguration.RPC, serv, oracleSrv, log, errChan)
 	serv.AddService(&rpcServer)
 
-	go serv.Start()
+	serv.Start()
 	if !cfg.ApplicationConfiguration.RPC.StartWhenSynchronized {
 		// Run RPC server in a separate routine. This is necessary to avoid a potential
 		// deadlock: Start() can write errors to errChan which is not yet read in the
diff --git a/internal/testcli/executor.go b/internal/testcli/executor.go
index 0dfa5be13..5fec5e06a 100644
--- a/internal/testcli/executor.go
+++ b/internal/testcli/executor.go
@@ -164,7 +164,7 @@ func NewTestChain(t *testing.T, f func(*config.Config), run bool) (*core.Blockch
 	})
 	require.NoError(t, err)
 	netSrv.AddConsensusService(cons, cons.OnPayload, cons.OnTransaction)
-	go netSrv.Start()
+	netSrv.Start()
 	errCh := make(chan error, 2)
 	rpcServer := rpcsrv.New(chain, cfg.ApplicationConfiguration.RPC, netSrv, nil, logger, errCh)
 	rpcServer.Start()
diff --git a/pkg/network/server.go b/pkg/network/server.go
index e000c18c5..0e919cbdd 100644
--- a/pkg/network/server.go
+++ b/pkg/network/server.go
@@ -262,7 +262,7 @@ func (s *Server) ID() uint32 {
 }
 
 // Start will start the server and its underlying transport. Calling it twice
-// is an error.
+// is an error. Caller should wait for Start to finish for normal server operation.
 func (s *Server) Start() {
 	s.log.Info("node started",
 		zap.Uint32("blockHeight", s.chain.BlockHeight()),
@@ -285,7 +285,7 @@ func (s *Server) Start() {
 	setServerAndNodeVersions(s.UserAgent, strconv.FormatUint(uint64(s.id), 10))
 	setNeoGoVersion(config.Version)
 	setSeverID(strconv.FormatUint(uint64(s.id), 10))
-	s.run()
+	go s.run()
 }
 
 // Shutdown disconnects all peers and stops listening. Calling it twice is an error,
diff --git a/pkg/network/server_test.go b/pkg/network/server_test.go
index 2ca95885f..7dbce881d 100644
--- a/pkg/network/server_test.go
+++ b/pkg/network/server_test.go
@@ -90,7 +90,7 @@ func TestServerStartAndShutdown(t *testing.T) {
 	t.Run("no consensus", func(t *testing.T) {
 		s := newTestServer(t, ServerConfig{})
 
-		go s.Start()
+		s.Start()
 		p := newLocalPeer(t, s)
 		s.register <- p
 		require.Eventually(t, func() bool { return 1 == s.PeerCount() }, time.Second, time.Millisecond*10)
@@ -110,7 +110,7 @@ func TestServerStartAndShutdown(t *testing.T) {
 		cons := new(fakeConsensus)
 		s.AddConsensusService(cons, cons.OnPayload, cons.OnTransaction)
 
-		go s.Start()
+		s.Start()
 		p := newLocalPeer(t, s)
 		s.register <- p
 
@@ -312,7 +312,7 @@ func TestServerNotSendsVerack(t *testing.T) {
 	s.id = 1
 	finished := make(chan struct{})
 	go func() {
-		s.run()
+		go s.run()
 		close(finished)
 	}()
 	t.Cleanup(func() {
@@ -389,7 +389,7 @@ func startTestServer(t *testing.T, protocolCfg ...func(*config.Blockchain)) *Ser
 }
 
 func startWithCleanup(t *testing.T, s *Server) {
-	go s.Start()
+	s.Start()
 	t.Cleanup(func() {
 		s.Shutdown()
 	})
diff --git a/pkg/services/rpcsrv/subscription_test.go b/pkg/services/rpcsrv/subscription_test.go
index 857f3b2d1..d5a291903 100644
--- a/pkg/services/rpcsrv/subscription_test.go
+++ b/pkg/services/rpcsrv/subscription_test.go
@@ -99,7 +99,7 @@ func TestSubscriptions(t *testing.T) {
 	defer chain.Close()
 	defer rpcSrv.Shutdown()
 
-	go rpcSrv.coreServer.Start()
+	rpcSrv.coreServer.Start()
 	defer rpcSrv.coreServer.Shutdown()
 
 	for _, feed := range subFeeds {
@@ -395,7 +395,7 @@ func TestFilteredNotaryRequestSubscriptions(t *testing.T) {
 	}
 
 	chain, rpcSrv, c, respMsgs, finishedFlag := initCleanServerAndWSClient(t)
-	go rpcSrv.coreServer.Start()
+	rpcSrv.coreServer.Start()
 
 	defer chain.Close()
 	defer rpcSrv.Shutdown()