Skip to content

Commit

Permalink
network: ensure server is started and shut down only once
Browse files Browse the repository at this point in the history
Use started atomic.Bool field to ensure that the node server shutdown
procedure is executed only once. Prevent the following panic caused by
server double-shutdown in testing code:
```
--- FAIL: TestServerRegisterPeer (0
.06s)
 panic: closed twice
 goroutine 60 [running]:
 testing.tRunner.func1.2({0x104c40b20, 0x104d0ec90})
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1545 +0x1c8
 testing.tRunner.func1()
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1548 +0x360
 panic({0x104c40b20?, 0x104d0ec90?})
 	/opt/homebrew/opt/go/libexec/src/runtime/panic.go:914 +0x218
 github.com/nspcc-dev/neo-go/pkg/network.(*fakeTransp).Close
 (0x14000159e08?)
 	/Users/ekaterinapavlova/Workplace/neo-go/pkg/network
 	/discovery_test.go:83 +0x54
 github.com/nspcc-dev/neo-go/pkg/network.(*Server).Shutdown
 (0x14000343400)
 	/Users/ekaterinapavlova/Workplace/neo-go/pkg/network/server.go:299
 	 +0x104
 github.com/nspcc-dev/neo-go/pkg/network.startWithCleanup.func1()
 	/Users/ekaterinapavlova/Workplace/neo-go/pkg/network/server_test
 	.go:408 +0x20
 testing.(*common).Cleanup.func1()
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1169 +0x110
 testing.(*common).runCleanup(0x1400032c340, 0x14000159d80?)
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1347 +0xd8
 testing.tRunner.func2()
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1589 +0x2c
 testing.tRunner(0x1400032c340, 0x104d0c5d0)
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1601 +0x114
 created by testing.(*T).Run in goroutine 1
 	/opt/homebrew/opt/go/libexec/src/testing/testing.go:1648 +0x33c
```

Signed-off-by: Ekaterina Pavlova <ekt@morphbits.io>
  • Loading branch information
AliceInHunterland committed Feb 21, 2024
1 parent c78337b commit cf6330c
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 4 deletions.
18 changes: 14 additions & 4 deletions pkg/network/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ type (

log *zap.Logger

// started used for Start and Shutdown server only once.
started atomic.Bool
// runLoopStarted indicates that the server's main run loop has started.
runLoopStarted atomic.Bool
}
Expand Down Expand Up @@ -266,9 +268,14 @@ func (s *Server) ID() uint32 {
return s.id
}

// Start will start the server and its underlying transport. Calling it twice
// is an error.
// Start will start the server and its underlying transport. Can be called
// only once. The same instance of the Server can't be started again after
// it's been stopped.
func (s *Server) Start() {
if !s.started.CompareAndSwap(false, true) {
s.log.Info("node server already started")
return
}
s.log.Info("node started",
zap.Uint32("blockHeight", s.chain.BlockHeight()),
zap.Uint32("headerHeight", s.chain.HeaderHeight()))
Expand All @@ -294,9 +301,12 @@ func (s *Server) Start() {
s.run()
}

// Shutdown disconnects all peers and stops listening. Calling it twice is an error,
// once stopped the same intance of the Server can't be started again by calling Start.
// Shutdown disconnects all peers and stops listening. Can be called only once.
// Once stopped the same instance of the Server can't be started again by calling Start.
func (s *Server) Shutdown() {
if !s.started.CompareAndSwap(true, false) {
return
}
s.log.Info("shutting down server", zap.Int("peers", s.PeerCount()))
for _, tr := range s.transports {
tr.Close()
Expand Down
28 changes: 28 additions & 0 deletions pkg/network/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,12 @@ func TestServerStartAndShutdown(t *testing.T) {
require.Eventually(t, func() bool { return 1 == s.PeerCount() }, time.Second, time.Millisecond*10)

assert.True(t, s.transports[0].(*fakeTransp).started.Load())
require.True(t, s.started.Load())
assert.Nil(t, s.txCallback)

s.Shutdown()

require.False(t, s.started.Load())
require.True(t, s.transports[0].(*fakeTransp).closed.Load())
err, ok := p.droppedWith.Load().(error)
require.True(t, ok)
Expand All @@ -115,11 +117,37 @@ func TestServerStartAndShutdown(t *testing.T) {
s.register <- p

assert.True(t, s.services["fake"].(*fakeConsensus).started.Load())
require.True(t, s.started.Load())

s.Shutdown()

require.False(t, s.started.Load())
require.True(t, s.services["fake"].(*fakeConsensus).stopped.Load())
})
t.Run("double start", func(t *testing.T) {
s := newTestServer(t, ServerConfig{})
startWithCleanup(t, s)

// Attempt to start the server again.
s.Start()

require.True(t, s.started.Load(), "server should still be marked as started after second Start call")
})
t.Run("double shutdown", func(t *testing.T) {
s := newTestServer(t, ServerConfig{})
go s.Start()
require.Eventually(t, func() bool {
return s.runLoopStarted.Load()
}, time.Second, 10*time.Millisecond)

s.Shutdown()

require.False(t, s.started.Load(), "server should be marked as not started after second Shutdown call")
// Attempt to shutdown the server again.
s.Shutdown()
// Verify the server state remains unchanged and is still considered shutdown.
require.False(t, s.started.Load(), "server should remain shutdown after second call")
})
}

func TestServerRegisterPeer(t *testing.T) {
Expand Down

0 comments on commit cf6330c

Please sign in to comment.