Repository: nspcc-dev/neo-go
Pull request #3661 (2024-11-05 08:10:19)

services/rpcsrv: Return a new server by pointer
### Problem
Before, a new server was returned by value which could cause a panic `unlock of unlocked mutex` on SIGHUP handling. It's because the new server overwrites a locked mutex of the already existing server.
```
oct‚ 22 13:51:15 node1 neo-go[1183338]: fatal error: sync: Unlock of unlocked RWMutex oct‚ 22 13:51:15 node1 neo-go[1183338]: goroutine 538 [running]: oct‚ 22 13:51:15 node1 neo-go[1183338]: sync.fatal({0xf83d64?, 0xc001085880?})
oct‚ 22 13:51:15 node1 neo-go[1183338]:         runtime/panic.go:1007 +0x18
oct‚ 22 13:51:15 node1 neo-go[1183338]: sync.(*RWMutex).Unlock(0xc00019a4c8)
oct‚ 22 13:51:15 node1 neo-go[1183338]:         sync/rwmutex.go:208 +0x45
oct‚ 22 13:51:15 node1 neo-go[1183338]: github.com/nspcc-dev/neo-go/pkg/services/rpcsrv.(*Server).dropSubscriber(0xc00019a2c8, 0xc000a77740)
oct‚ 22 13:51:15 node1 neo-go[1183338]:         github.com/nspcc-dev/neo-go/pkg/services/rpcsrv/server.go:825 +0xce
oct‚ 22 13:51:15 node1 neo-go[1183338]: github.com/nspcc-dev/neo-go/pkg/services/rpcsrv.(*Server).handleWsReads(0xc00019a2c8, 0xc0034478c0, 0xc000af5f80, 0xc000a7
7740)
oct‚ 22 13:51:15 node1 neo-go[1183338]:         github.com/nspcc-dev/neo-go/pkg/services/rpcsrv/server.go:810 +0x266
oct‚ 22 13:51:15 node1 neo-go[1183338]: github.com/nspcc-dev/neo-go/pkg/services/rpcsrv.(*Server).handleHTTPRequest(0xc00019a2c8, {0x11c3900, 0xc003437dc0}, 0xc00
31945a0)
oct‚ 22 13:51:15 node1 neo-go[1183338]:         github.com/nspcc-dev/neo-go/pkg/services/rpcsrv/server.go:582 +0x54a
oct‚ 22 13:51:15 node1 neo-go[1183338]: net/http.HandlerFunc.ServeHTTP(0x471779?, {0x11c3900?, 0xc003437dc0?}, 0xc000943b68?)
oct‚ 22 13:51:15 node1 neo-go[1183338]:         net/http/server.go:2171 +0x29
oct‚ 22 13:51:15 node1 neo-go[1183338]: net/http.serverHandler.ServeHTTP({0xc000a77680?}, {0x11c3900?, 0xc003437dc0?}, 0x6?)
oct‚ 22 13:51:15 node1 neo-go[1183338]:         net/http/server.go:3142 +0x8e
oct‚ 22 13:51:15 node1 neo-go[1183338]: net/http.(*conn).serve(0xc0032030e0, {0x11c5220, 0xc000a76960})
oct‚ 22 13:51:15 node1 neo-go[1183338]:         net/http/server.go:2044 +0x5e8
oct‚ 22 13:51:15 node1 neo-go[1183338]: created by net/http.(*Server).Serve in goroutine 534
oct‚ 22 13:51:15 node1 neo-go[1183338]:         net/http/server.go:3290 +0x4b4
```

### Solution
Return a new server by pointer.

alexey-savchuk: Additionally, it seems that `rpcsrv.Server` doesn't wait for all HTTP handler to complete, which could cause an overlap in the lifetimes of the old and new servers and, as a consequence, the behavior described in the PR. I was wondering if this behavior of the server is intended?

https://github.com/nspcc-dev/neo-go/blob/95098d4b2538e34af7fe4c1c4219deea61bca581/cli/server/server.go#L565-L566
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/nspcc-dev/neo-go/pull/3661?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev) Report
Attention: Patch coverage is `60.00000%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 83.07%. Comparing base [(`95098d4`)](https://app.codecov.io/gh/nspcc-dev/neo-go/commit/95098d4b2538e34af7fe4c1c4219deea61bca581?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev) to head [(`d06088f`)](https://app.codecov.io/gh/nspcc-dev/neo-go/commit/d06088f00de2a51d171a01a2c357365643182871?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev).

| [Files with missing lines](https://app.codecov.io/gh/nspcc-dev/neo-go/pull/3661?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev) | Patch % | Lines |
|---|---|---|
| [cli/server/server.go](https://app.codecov.io/gh/nspcc-dev/neo-go/pull/3661?src=pr&el=tree&filepath=cli%2Fserver%2Fserver.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev#diff-Y2xpL3NlcnZlci9zZXJ2ZXIuZ28=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/nspcc-dev/neo-go/pull/3661?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3661      +/-   ##
==========================================
- Coverage   83.13%   83.07%   -0.06%     
==========================================
  Files         334      334              
  Lines       46563    46563              
==========================================
- Hits        38708    38684      -24     
- Misses       6278     6307      +29     
+ Partials     1577     1572       -5     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/nspcc-dev/neo-go/pull/3661?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nspcc-dev).


Patch:
diff --git a/cli/server/server.go b/cli/server/server.go
index b3f9ec22b..64af4c528 100644
--- a/cli/server/server.go
+++ b/cli/server/server.go
@@ -505,7 +505,7 @@ func startServer(ctx *cli.Context) error {
 	}
 	errChan := make(chan error)
 	rpcServer := rpcsrv.New(chain, cfg.ApplicationConfiguration.RPC, serv, oracleSrv, log, errChan)
-	serv.AddService(&rpcServer)
+	serv.AddService(rpcServer)
 
 	serv.Start()
 	if !cfg.ApplicationConfiguration.RPC.StartWhenSynchronized {
@@ -561,10 +561,10 @@ Main:
 					logLevel.SetLevel(newLogLevel)
 					log.Warn("using new logging level", zap.Stringer("level", newLogLevel))
 				}
-				serv.DelService(&rpcServer)
+				serv.DelService(rpcServer)
 				rpcServer.Shutdown()
 				rpcServer = rpcsrv.New(chain, cfgnew.ApplicationConfiguration.RPC, serv, oracleSrv, log, errChan)
-				serv.AddService(&rpcServer)
+				serv.AddService(rpcServer)
 				if !cfgnew.ApplicationConfiguration.RPC.StartWhenSynchronized || serv.IsInSync() {
 					// Here similar to the initial run (see above for-loop), so async.
 					go rpcServer.Start()
diff --git a/pkg/services/rpcsrv/server.go b/pkg/services/rpcsrv/server.go
index f25beae04..ecd8b42b7 100644
--- a/pkg/services/rpcsrv/server.go
+++ b/pkg/services/rpcsrv/server.go
@@ -269,7 +269,7 @@ var rpcWsHandlers = map[string]func(*Server, params.Params, *subscriber) (any, *
 // New creates a new Server struct. Pay attention that orc is expected to be either
 // untyped nil or non-nil structure implementing OracleHandler interface.
 func New(chain Ledger, conf config.RPC, coreServer *network.Server,
-	orc OracleHandler, log *zap.Logger, errChan chan<- error) Server {
+	orc OracleHandler, log *zap.Logger, errChan chan<- error) *Server {
 	protoCfg := chain.GetConfig().ProtocolConfiguration
 	if conf.SessionEnabled {
 		if conf.SessionExpirationTime <= 0 {
@@ -339,7 +339,7 @@ func New(chain Ledger, conf config.RPC, coreServer *network.Server,
 		}
 	}
 
-	return Server{
+	return &Server{
 		http:  httpServers,
 		https: tlsServers,
 


Test patch:
diff --git a/internal/testcli/executor.go b/internal/testcli/executor.go
index 24957db0e..7ec80513c 100644
--- a/internal/testcli/executor.go
+++ b/internal/testcli/executor.go
@@ -178,7 +178,7 @@ func NewTestChain(t *testing.T, f func(*config.Config), run bool) (*core.Blockch
 	rpcServer := rpcsrv.New(chain, cfg.ApplicationConfiguration.RPC, netSrv, nil, logger, errCh)
 	rpcServer.Start()
 
-	return chain, &rpcServer, netSrv
+	return chain, rpcServer, netSrv
 }
 
 func NewExecutor(t *testing.T, needChain bool) *Executor {
diff --git a/pkg/services/rpcsrv/server_helper_test.go b/pkg/services/rpcsrv/server_helper_test.go
index 35ef5ae49..97065a13d 100644
--- a/pkg/services/rpcsrv/server_helper_test.go
+++ b/pkg/services/rpcsrv/server_helper_test.go
@@ -131,7 +131,7 @@ func wrapUnitTestChain(t testing.TB, chain *core.Blockchain, orc OracleHandler,
 	handler := http.HandlerFunc(rpcServer.handleHTTPRequest)
 	srv := httptest.NewServer(handler)
 	t.Cleanup(srv.Close)
-	return chain, &rpcServer, srv
+	return chain, rpcServer, srv
 }
 
 func initClearServerWithCustomConfig(t testing.TB, ccfg func(configuration *config.Config)) (*core.Blockchain, *Server, *httptest.Server) {