fix(server/v2): improve server stop #22455
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across various files, primarily focusing on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Tool Failures:Tool Failure Count:📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
julienrbrt
force-pushed
the
julien/fix-close
branch
from
f0ec56a
to
6d94ecc
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
julienrbrt
requested review from
a team,
kocubinski,
facundomedica,
sontrinh16 and
hieuvubk
as code owners
julienrbrt
added
the
backport/v0.52.x
|
@julienrbrt your pull request is missing a changelog! |
|
adding backport label for simapp/v2 and cometbft server changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
server/v2/store/server.go (1)
Line range hint
27-51: Consider implementing a coordinated shutdown mechanismThe current changes appear to be moving away from proper shutdown handling. Consider:
- Using a WaitGroup to coordinate shutdown across components
- Implementing a shutdown hook that ensures all cleanup operations complete
- Adding logging to track shutdown progress
- Using context with timeout for graceful shutdown
This would help ensure that:
- All components have a chance to clean up
- The control thread doesn't exit prematurely
- Resources are properly released
- Shutdown progress can be monitored
server/v2/api/rest/server.go (1)
Line range hint
89-96: Consider enhancing shutdown robustness.Given the PR objectives around improving server shutdown, consider these enhancements:
- Add timeout configuration for graceful shutdown
- Add detailed logging about shutdown stages
- Handle context cancellation explicitly
Here's a suggested implementation:
func (s *Server[T]) Stop(ctx context.Context) error { if !s.config.Enable { return nil } s.logger.Info("stopping HTTP server") - return s.httpServer.Shutdown(ctx) + shutdownComplete := make(chan error, 1) + go func() { + shutdownComplete <- s.httpServer.Shutdown(ctx) + }() + + select { + case err := <-shutdownComplete: + if err != nil { + s.logger.Error("error during HTTP server shutdown", "error", err) + return fmt.Errorf("HTTP server shutdown error: %w", err) + } + s.logger.Info("HTTP server shutdown completed successfully") + return nil + case <-ctx.Done(): + return fmt.Errorf("HTTP server shutdown timeout: %w", ctx.Err()) + } }server/v2/api/telemetry/server.go (1)
Line range hint
93-100: Consider wrapping the shutdown error with additional context.While the Stop implementation is solid, consider wrapping the error from
Shutdownto provide more context about the failure point.func (s *Server[T]) Stop(ctx context.Context) error { if !s.config.Enable || s.server == nil { return nil } s.logger.Info("stopping telemetry server...", "address", s.config.Address) - return s.server.Shutdown(ctx) + if err := s.server.Shutdown(ctx); err != nil { + return fmt.Errorf("failed to shutdown telemetry server: %w", err) + } + return nil }server/v2/commands.go (1)
68-68: Consider context handling in deferred cleanup.The enhanced shutdown logic correctly orders operations (stop server then close app) and handles errors appropriately. However, there's a potential issue with using
cmd.Context()in the deferred function.Since the context might be canceled by the time the deferred function runs (either by the signal handler or natural command completion), this could prevent proper server shutdown.
Consider creating a separate context for shutdown:
defer func() { - if err := server.Stop(cmd.Context()); err != nil { + shutdownCtx := context.Background() + if err := server.Stop(shutdownCtx); err != nil { cmd.PrintErrln("failed to stop servers:", err) } if err := appCloser.Close(); err != nil { cmd.PrintErrln("failed to close application:", err) } }()Also applies to: 94-102
server/v2/server.go (1)
109-119: Consider improving Stop method robustnessWhile the synchronous stopping is safer, consider these improvements:
- Add timeout handling to prevent hanging on slow components
- Check context cancellation between stops
- Consider parallel stopping with proper error handling for faster shutdown
Here's a suggested implementation:
func (s *Server[T]) Stop(ctx context.Context) error { logger := GetLoggerFromContext(ctx).With(log.ModuleKey, s.Name()) logger.Info("stopping servers...") - var err error - for _, mod := range s.components { - err = errors.Join(err, mod.Stop(ctx)) + var errs []error + for _, mod := range s.components { + if ctx.Err() != nil { + errs = append(errs, fmt.Errorf("context cancelled while stopping components: %w", ctx.Err())) + break + } + + // Create timeout context for each component + stopCtx, cancel := context.WithTimeout(ctx, 30*time.Second) + err := mod.Stop(stopCtx) + cancel() + + if err != nil { + errs = append(errs, fmt.Errorf("failed to stop %s: %w", mod.Name(), err)) + } } - return err + return errors.Join(errs...) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (14)
server/v2/api/grpc/server.go(1 hunks)server/v2/api/rest/server.go(1 hunks)server/v2/api/telemetry/server.go(1 hunks)server/v2/api/utils.go(0 hunks)server/v2/api/utils_test.go(0 hunks)server/v2/cometbft/server.go(2 hunks)server/v2/commands.go(5 hunks)server/v2/go.mod(1 hunks)server/v2/server.go(3 hunks)server/v2/server_test.go(0 hunks)server/v2/store/server.go(2 hunks)simapp/v2/app_di.go(1 hunks)simapp/v2/simdv2/cmd/commands.go(4 hunks)simapp/v2/simdv2/cmd/config.go(1 hunks)
💤 Files with no reviewable changes (3)
- server/v2/api/utils.go
- server/v2/api/utils_test.go
- server/v2/server_test.go
🧰 Additional context used
📓 Path-based instructions (10)
server/v2/api/grpc/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.
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/cometbft/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/store/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/app_di.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/commands.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (16)
server/v2/store/server.go (2)
Line range hint 30-39: LGTM: Constructor properly handles the type change
The constructor is well-implemented with proper error handling and follows Go idioms.
27-27:
Verify if Backend interface includes Close method
The change from RootStore to Backend type might prevent access to the Close() method during shutdown if Backend interface doesn't include this method. This appears to contradict the PR objective of ensuring proper cleanup during shutdown.
Let's verify the Backend interface definition:
server/v2/api/rest/server.go (2)
21-22: LGTM! Field ordering follows best practices.
The struct field ordering follows logical grouping and maintains consistent alignment.
30-30: LGTM! Parameter ordering is logical.
The parameter reordering in the New function and corresponding struct initialization follows a clear pattern: infrastructure components (logger) followed by business logic components (appManager).
Also applies to: 36-36
server/v2/api/telemetry/server.go (1)
25-25: LGTM! Field ordering maintains consistency.
The reordering of the config field after logger aligns with the consistent structure being established across different server implementations.
simapp/v2/simdv2/cmd/config.go (1)
82-82: Enhanced logging configuration looks good!
The expanded logging configuration improves observability across critical components, particularly for debugging server shutdown issues. The addition of store:info level is especially relevant for tracking store closure operations mentioned in the PR objectives.
Let's verify that all the new log components are valid:
✅ Verification successful
Let me gather more specific information about the logging components being used.
Based on the search results, I can now provide a final response regarding the logging configuration verification.
All logging components in the configuration are valid and properly used
The expanded logging configuration includes standard components that are well-integrated within the codebase:
telemetry- Used by the telemetry server (server/v2/api/telemetry)grpc- Used by the gRPC server (server/v2/api/grpc)rest- Used by the REST server (server/v2/api/rest)grpc-gateway- Used by the gRPC-gateway server (server/v2/api/grpcgateway)comet- Used by the CometBFT server (server/v2/cometbft)store- Used by the store service and indexer components
Each component follows the standard logging pattern using the ModuleKey constant for module identification, and the log levels align with the application's logging infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for the usage of new logging components to ensure they are valid
# Test: Look for logger initialization or usage of these components
rg -g '*.go' 'NewLogger|WithLogger' -A 3
Length of output: 8869
Script:
#!/bin/bash
# Search for logging level configurations and module keys
rg -g '*.go' 'ModuleKey|LogLevel.*=.*".*:' -A 2
Length of output: 23065
server/v2/go.mod (1)
104-104: LGTM! The addition of golang.org/x/sync aligns with improving server shutdown.
The golang.org/x/sync package provides synchronization primitives that will be useful for managing server shutdown operations. However, consider if version v0.9.0 is the optimal choice, as there might be newer versions available.
Let's check if there are any newer versions available:
✅ Verification successful
LGTM! The golang.org/x/sync v0.9.0 dependency is correctly used and up-to-date
The golang.org/x/sync package is actively used in the codebase, particularly the errgroup package for parallel error handling in several critical components:
- Server startup and shutdown coordination
- Store migration management
- Network test utilities
- Government module genesis handling
- Store root operations
The version v0.9.0 is currently the latest stable release, making it an appropriate choice for the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the latest version of golang.org/x/sync
gh api repos/golang/sync/releases/latest --jq .tag_name
Length of output: 212
Script:
#!/bin/bash
# Let's try a different approach to check the version using go list
go list -m -versions golang.org/x/sync
# As a backup, let's also check if there are any other usages of this package
rg "golang.org/x/sync" -A 2 -B 2
Length of output: 75570
Script:
#!/bin/bash
# Let's check the actual usage of golang.org/x/sync in the codebase
rg "golang.org/x/sync/errgroup" -l
Length of output: 199
server/v2/commands.go (2)
6-6: LGTM: Import addition is correct.
The io import is required for the new io.Closer interface and is properly placed.
26-26: LGTM: Parameter addition improves shutdown handling.
The addition of appCloser parameter aligns with the PR objectives to ensure proper cleanup during shutdown.
Let's verify that all callers of AddCommands have been updated:
Also applies to: 36-36
✅ Verification successful
Let me check the implementation details of these AddCommands functions to understand the relationship between them.
Let me try a different approach to get the implementation details.
Based on the code analysis, I can now provide a final response.
Parameter addition requires updates in all callers
The addition of appCloser parameter in server/v2/commands.go requires updates in all callers of AddCommands. From the code:
simapp/v2/simdv2/cmd/commands.goalready passesio.NopCloser(nil)orsimAppas theappCloserparameterserver/util.goversion ofAddCommandshas a different signature and is used for a different purpose (it's the v1 version)
The change is properly handled in all callers and maintains backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AddCommands function calls to ensure they include the new parameter
rg -A 2 "AddCommands\["
Length of output: 705
Script:
#!/bin/bash
# Get the full implementation of AddCommands functions
ast-grep --pattern 'func AddCommands[$_]($_) {
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Get the content of files containing AddCommands
echo "=== server/util.go ==="
cat server/util.go
echo -e "\n=== server/v2/commands.go ==="
cat server/v2/commands.go
echo -e "\n=== simapp/v2/simdv2/cmd/commands.go ==="
cat simapp/v2/simdv2/cmd/commands.go
Length of output: 30747
simapp/v2/app_di.go (2)
201-208: LGTM! Well-structured cleanup implementation.
The Close method implementation follows a correct cleanup order by closing the store before calling the base Close method. This ensures proper resource cleanup as dependent resources are closed first.
201-208: Verify integration with server shutdown flow.
While the Close implementation is correct, we should verify that this method is properly called during server shutdown, given the control thread issues mentioned in the PR objectives.
Let's verify the integration:
✅ Verification successful
Let me gather more information about the shutdown flow integration.
Close() is properly integrated into the shutdown flow
The verification shows that the Close() implementation is correctly integrated into the server shutdown sequence:
- The app's
Close()is called during server shutdown inserver/v2/commands.gothrough theappCloserinterface - The shutdown is triggered by:
- OS signals (SIGINT/SIGTERM) handling
- Context cancellation
- The implementation properly closes both:
- The store via
app.store.Close() - The base app via
app.App.Close()
- The store via
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Close() is properly integrated into shutdown flow
# Look for server shutdown handling that should call Close()
rg -A 5 "func.*Stop.*\("
# Look for signal handling that might trigger shutdown
rg -A 5 "os\.Signal"
# Look for other Close() method calls
ast-grep --pattern 'Close()'
Length of output: 5244
Script:
#!/bin/bash
# Check for server shutdown sequence and Close() integration
rg -A 10 "func.*Start.*\(" server/v2/
rg -A 10 "func.*Run.*\(" server/v2/
# Check for app.Close() calls
rg -A 5 "app.*Close\(\)"
# Look for shutdown context handling
rg -A 5 "context.*cancel" server/v2/
Length of output: 17371
simapp/v2/simdv2/cmd/commands.go (4)
5-5: LGTM: Import addition is appropriate.
The io package import is correctly placed and necessary for the io.NopCloser usage.
128-128: Verify SimApp implements io.Closer interface.
SimApp is now passed as the app closer to AddCommands. This requires SimApp to implement the io.Closer interface.
#!/bin/bash
# Check if SimApp implements Close method
ast-grep --pattern 'type SimApp struct {
$$$
}
$$$
func (app *SimApp) Close() error {
$$$
}'97-97: Verify impact of rest.New parameter reordering.
The parameter order in rest.New has changed (logger is now first). This is a breaking change that requires verification of all callers.
#!/bin/bash
# Find all callers of rest.New to ensure they're updated
rg "rest\.New\(" --type go75-75: Verify the usage of NopCloser in server shutdown.
The addition of io.NopCloser(nil) appears to be part of the server stop improvements. Let's verify its integration with the server shutdown logic.
✅ Verification successful
Usage of io.NopCloser(nil) is appropriate for server initialization
The addition of io.NopCloser(nil) is correct and aligns with the server architecture:
- When SimApp is nil (client-only mode), a nil closer is appropriate since there's no app to close
- The server framework properly handles both nil and non-nil app closers during shutdown
- The SimApp implementation has a proper Close() method that cleans up resources when needed
The code shows a clean separation between client and server modes, where NopCloser is used only when running in client-only mode without a full application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how NopCloser is used in the server shutdown flow
rg -A 5 "AddCommands.*NopCloser"
rg -A 10 "Stop.*Close" server/v2/
Length of output: 100
Script:
#!/bin/bash
# Search for NopCloser usage in server and app initialization
rg -A 5 "NopCloser"
# Look for server stop/close related code
rg -A 10 "func.*Stop" server/v2/
rg -A 10 "func.*Close" server/v2/
# Check AddCommands implementation
fd -e go -E "vendor/*" -E "testutil/*" --exec grep -l "AddCommands" {} \; | xargs cat
Length of output: 59147
Script:
#!/bin/bash
# Search for server stop/close related code in the app
ast-grep --pattern 'func (app *SimApp) Close() error {
$$$
}'
# Look for AddCommands usage with NopCloser
rg -A 10 'AddCommands.*NopCloser'
# Check server start/stop flow
rg -A 10 'func.*Start.*Context' server/v2/
Length of output: 5522
server/v2/cometbft/server.go (1)
99-105: LGTM: Improved error handling implementation
The change from panic to proper error handling is a significant improvement. The error messages are descriptive and properly wrapped, allowing for better error propagation and debugging.
| go func() { | ||
| resCh <- mod.Start(ctx) | ||
| }() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
(cherry picked from commit 6eca7eb) # Conflicts: # server/v2/api/grpc/server.go # server/v2/api/rest/server.go # server/v2/api/telemetry/server.go # server/v2/commands.go # server/v2/go.mod # server/v2/server.go # server/v2/server_test.go # server/v2/store/server.go
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
Description
Closes: #22405
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
Closemethod forSimAppto improve resource management during application shutdown.Improvements
Bug Fixes
Documentation