fix(server/v2): avoid server stop get call before start for multi components #22811
Conversation
…ponents
init httpServer early to avoid nil httpServer when stop
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x1050bd0f0]
goroutine 1 [running]:
net/http.(*Server).Shutdown(0x10851b518?, {0x108560af8?, 0x14003124720?})
net/http/server.go:3050 +0x30
cosmossdk.io/server/v2/api/rest.(*Server[...]).Stop(0xd, {0x108560af8?, 0x14003124720})
cosmossdk.io/server/v2@v2.0.0-20240718121635-a877e3e8048a/api/rest/server.go:93 +0xe4
cosmossdk.io/server/v2.(*Server[...]).Stop(0x1085be3c0, {0x108560af8, 0x14003124720})
cosmossdk.io/server/v2@v2.0.0-20240718121635-a877e3e8048a/server.go:119 +0x21c
cosmossdk.io/server/v2.createStartCommand[...].func1.2.1()
cosmossdk.io/server/v2@v2.0.0-20240718121635-a877e3e8048a/commands.go:103 +0xdc
cosmossdk.io/server/v2.createStartCommand[...].func1.2()
cosmossdk.io/server/v2@v2.0.0-20240718121635-a877e3e8048a/co
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
C:server/v2
mmsqe
requested review from
julienrbrt,
hieuvubk,
kocubinski and
a team
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/api/rest/server.go(2 hunks)systemtests/system.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
systemtests/system.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/rest/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
server/v2/api/rest/server.go (1)
49-52: LGTM: Early server initialization prevents nil pointer issues.
The early initialization of httpServer in New() is a good approach to prevent nil pointer dereference when Stop() is called before Start().
systemtests/system.go (1)
738-739: LGTM: Proper configuration of gateway addresses
The configuration properly sets unique addresses for grpc-gateway and rest endpoints for each full node, ensuring no port conflicts.
server/v2/api/rest/server.go
Outdated
| defer func() { | ||
| s.httpServer = &http.Server{ | ||
| Addr: s.config.Address, | ||
| Handler: s.router, | ||
| } | ||
| }() |
There was a problem hiding this comment.
Reconsider server reinitialization in Stop()
The deferred reinitialization of the httpServer after shutdown could be problematic:
- It's unclear why the server needs to be recreated after shutdown
- This could lead to resource leaks if the server is repeatedly stopped and started
- The reinitialization might mask other underlying issues
Consider removing the deferred reinitialization:
func (s *Server[T]) Stop(ctx context.Context) error {
if !s.config.Enable {
return nil
}
s.logger.Info("stopping HTTP server")
- defer func() {
- s.httpServer = &http.Server{
- Addr: s.config.Address,
- Handler: s.router,
- }
- }()
return s.httpServer.Shutdown(ctx)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer func() { | |
| s.httpServer = &http.Server{ | |
| Addr: s.config.Address, | |
| Handler: s.router, | |
| } | |
| }() | |
| return s.httpServer.Shutdown(ctx) |
server/v2/api/rest/server.go
Outdated
| @@ -89,6 +87,12 @@ func (s *Server[T]) Stop(ctx context.Context) error { | |||
| } | |||
|
|
|||
| s.logger.Info("stopping HTTP server") | |||
| defer func() { | |||
There was a problem hiding this comment.
If httpServer is set in New how come we need this?
There was a problem hiding this comment.
If we call Start again later after Stop, the old httpServer will fail since Shutdown was called.
There was a problem hiding this comment.
I see, but why would you do that? What is the use case?
I think we should just add in the server component docs, like in the std http server docs something like:
Once Stop has been called on a server, it may not be reused.
There was a problem hiding this comment.
Before we wanted to support server reloading, so then it would have made sense, but due to the config reloading limitation, we haven't gone that way. I think it makes server components simpler to write if you know once you call stop you don't need to make it re-startable. So setting the server in New instead of Start does makes sense, but imho, we don't need to change anything else in Stop.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
server/v2/api/rest/server.go (2)
62-67: Add documentation for the init methodThe initialization logic is well-encapsulated, but could benefit from documentation explaining its purpose and that it's safe to call multiple times.
Add documentation like:
+// init initializes the HTTP server with the current configuration. +// It is safe to call multiple times, especially after server shutdown. func (s *Server[T]) init() {
94-95: Consider making server restart handling more explicitWhile reinitializing the server after shutdown is necessary for supporting restart scenarios, the current implementation could be improved:
- The restart capability isn't obvious from the API
- There's no guarantee that the same config is still valid at restart time
Consider a more explicit approach:
func (s *Server[T]) Stop(ctx context.Context) error { if !s.config.Enable { return nil } s.logger.Info("stopping HTTP server") - defer s.init() + err := s.httpServer.Shutdown(ctx) + if err != nil { + return err + } + + // Prepare for potential restart + s.init() + return nil }Also consider adding a comment in the struct documentation about restart capability:
// Server implements a REST server that supports stopping and restarting // with the same configuration. type Server[T transaction.Tx] struct {server/v2/api/telemetry/server.go (1)
59-71: Add documentation for the init methodThe initialization logic is well-structured, combining route setup and server initialization. Consider adding documentation for clarity.
Add documentation like:
+// init initializes the HTTP server and sets up the routes. +// It is safe to call multiple times, especially after server shutdown. func (s *Server[T]) init() {
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
server/v2/api/rest/server.go(3 hunks)server/v2/api/telemetry/server.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
server/v2/api/telemetry/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/rest/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
server/v2/api/rest/server.go (1)
49-49: LGTM: Early initialization prevents nil pointer issues
The addition of srv.init() after config setup ensures the httpServer is properly initialized before any potential Stop calls, directly addressing the nil pointer dereference issue.
server/v2/api/telemetry/server.go (2)
55-55: LGTM: Consistent initialization pattern
The addition of srv.init() follows the same pattern as the REST server, ensuring proper initialization after all dependencies are set up.
105-105: Apply the same improvements as REST server
The server reinitialization pattern here matches the REST server. Consider applying the same improvements suggested for the REST server's Stop method to make the restart capability more explicit and safer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
server/v2/api/grpcgateway/server.go (2)
78-79: Consider using a more specific route pattern.The catch-all pattern "/" might be too permissive. Consider using a more specific base path that matches your API structure.
Example:
mux := http.NewServeMux() -mux.Handle("/", s.GRPCGatewayRouter) +mux.Handle("/api/", http.StripPrefix("/api", s.GRPCGatewayRouter))
Line range hint
116-125: Consider adding synchronization to ensure server is fully started.There's a potential race condition where
Stopcould be called afterStartreturns but before the server is fully listening. Consider adding a way to ensure the server is fully started.Example approach:
func (s *Server[T]) Start(ctx context.Context) error { if !s.config.Enable { s.logger.Info(fmt.Sprintf("%s server is disabled via config", s.Name())) return nil } // Create a channel to signal server start ready := make(chan struct{}) go func() { // Signal when listener is ready ln, err := net.Listen("tcp", s.config.Address) if err != nil { close(ready) return } close(ready) s.server.Serve(ln) }() // Wait for server to start or context to cancel select { case <-ready: return nil case <-ctx.Done(): return ctx.Err() } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
server/v2/api/grpcgateway/server.go(1 hunks)server/v2/api/rest/server.go(1 hunks)server/v2/api/telemetry/server.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/v2/api/telemetry/server.go
- server/v2/api/rest/server.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpcgateway/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation