refactor(serverv2): remove unused interface methods, honuor context #22394
Conversation
# Conflicts: # server/v2/api/telemetry/server.go
testinginprod
requested review from
julienrbrt,
hieuvubk,
kocubinski and
a team
as code owners
github-actions
bot
added
C:server/v2
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several files, primarily focusing on 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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| go func() { | ||
| defer close(done) | ||
|
|
||
| f() | ||
| }() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
![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 (2)
server/v2/api/utils.go (1)
5-7: Enhance function documentation for better clarity
Consider adding more details about:
- The return value (error from context or nil)
- Whether this function blocks until completion
- Goroutine safety considerations
// DoUntilCtxExpired runs the given function until the context is expired or
// the function exits.
-// This forces context to be honored.
+// This forces context to be honored by running the function in a separate goroutine
+// and monitoring the context for cancellation.
+//
+// Returns ctx.Err() if the context expires before the function completes,
+// or nil if the function completes successfully.
+//
+// Note: This function blocks until either the context expires or the function completes.server/v2/api/utils_test.go (1)
11-35: Add test cases for edge scenarios.
The current test suite should be expanded to cover these edge cases:
- Already canceled context
- Nil context
- Nil callback function
Example additional test case:
t.Run("already canceled context", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel()
err := DoUntilCtxExpired(ctx, func() {
t.Fatal("callback should not be executed")
})
require.ErrorIs(t, err, context.Canceled)
})📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- runtime/v2/store.go (0 hunks)
- server/v2/api/grpc/server.go (3 hunks)
- server/v2/api/utils.go (1 hunks)
- server/v2/api/utils_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- runtime/v2/store.go
🧰 Additional context used
📓 Path-based instructions (3)
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/utils.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/utils_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (5)
server/v2/api/utils.go (2)
1-3: LGTM: Clean package structure and imports
Package declaration and imports are appropriately structured.
10-14: Regarding the security bot's non-determinism warning
While the security bot correctly flags goroutine usage as a potential source of non-determinism, in this case, it's a necessary implementation detail for handling concurrent context cancellation. The code properly manages the goroutine lifecycle through channels and context.
Let's verify the usage of this pattern in the codebase:
✅ Verification successful
Goroutine usage is properly managed with context and channels
The code in server/v2/api/utils.go implements a safe pattern for goroutine management:
- The goroutine's lifecycle is controlled through a
donechannel that is properly closed usingdefer - The function properly handles context cancellation through a
selectstatement that checks bothctx.Done()and thedonechannel - This pattern is consistently used across the codebase for similar scenarios (as seen in gRPC gateway implementations and other services)
- The goroutine cannot leak since either context cancellation or function completion will terminate it
- Error propagation is handled correctly by returning
ctx.Err()when the context is cancelled
The security bot's warning about non-determinism can be safely ignored in this case as the goroutine is used for proper concurrent context handling rather than non-deterministic computation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns of goroutine usage with context
ast-grep --pattern 'go func() {
$$$
<-ctx.Done()
$$$
}'
Length of output: 63
Script:
#!/bin/bash
# Let's check the broader context of the file to understand how context and channels are used
cat server/v2/api/utils.go
# Also search for similar goroutine patterns in the codebase
rg "go func\(\)" -A 5 -B 5
# Look for context usage patterns
rg "ctx.Done" -A 3 -B 3
Length of output: 80532
server/v2/api/utils_test.go (1)
1-9: LGTM! Clean imports following style guide.
The imports are well-organized with standard library imports grouped separately from external dependencies.
server/v2/api/grpc/server.go (2)
28-28: LGTM: Import addition is correct and well-placed.
The new import for the api package is properly organized with other internal imports and is required for the context handling improvements.
Line range hint 201-215: LGTM: Improved context handling in server startup.
The change to use (&net.ListenConfig{}).Listen(ctx, ...) is a significant improvement as it makes the server startup context-aware, allowing for proper cancellation and timeout handling. This follows Go best practices for context propagation.
| func DoUntilCtxExpired(ctx context.Context, f func()) error { | ||
| done := make(chan struct{}) | ||
| go func() { | ||
| defer close(done) | ||
|
|
||
| f() | ||
| }() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-done: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential goroutine leak on context cancellation
The current implementation has a potential goroutine leak. When the context is cancelled, the goroutine continues running without any way to stop it. This could lead to resource leaks in long-running applications.
Consider adding a way to signal the goroutine to stop:
func DoUntilCtxExpired(ctx context.Context, f func()) error {
done := make(chan struct{})
+ // Create a child context that we can cancel
+ runCtx, cancel := context.WithCancel(ctx)
+ defer cancel()
+
go func() {
defer close(done)
-
- f()
+ // Pass the cancellable context to the function
+ go func() {
+ f()
+ cancel()
+ }()
+
+ // Wait for either completion or cancellation
+ <-runCtx.Done()
}()
select {
case <-ctx.Done():
+ // Cancel the running function
+ cancel()
return ctx.Err()
case <-done:
return nil
}
}Committable suggestion was skipped due to low confidence.
| t.Run("success", func(t *testing.T) { | ||
| ctx := context.Background() | ||
|
|
||
| funcRan := false | ||
| err := DoUntilCtxExpired(ctx, func() { | ||
| funcRan = true | ||
| }) | ||
| require.NoError(t, err) | ||
| require.True(t, funcRan) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test robustness.
While the test correctly verifies the basic functionality, consider these improvements:
- Use a timeout context to prevent potential test hangs
- Add execution time assertions
- Document the test scenario
t.Run("success", func(t *testing.T) {
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+
+ start := time.Now()
funcRan := false
err := DoUntilCtxExpired(ctx, func() {
funcRan = true
})
require.NoError(t, err)
require.True(t, funcRan)
+ require.Less(t, time.Since(start), 5*time.Second)
})📝 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.
| t.Run("success", func(t *testing.T) { | |
| ctx := context.Background() | |
| funcRan := false | |
| err := DoUntilCtxExpired(ctx, func() { | |
| funcRan = true | |
| }) | |
| require.NoError(t, err) | |
| require.True(t, funcRan) | |
| }) | |
| t.Run("success", func(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| start := time.Now() | |
| funcRan := false | |
| err := DoUntilCtxExpired(ctx, func() { | |
| funcRan = true | |
| }) | |
| require.NoError(t, err) | |
| require.True(t, funcRan) | |
| require.Less(t, time.Since(start), 5*time.Second) | |
| }) |
| t.Run("context expired", func(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| funcRan := false | ||
| err := DoUntilCtxExpired(ctx, func() { | ||
| cancel() | ||
| funcRan = true | ||
| <-time.After(time.Second) | ||
| }) | ||
| require.ErrorIs(t, err, context.Canceled) | ||
| require.True(t, funcRan) | ||
| }) |
There was a problem hiding this comment.
Fix potential test flakiness and goroutine leak.
The current implementation has several issues:
- Hard-coded 1-second delay makes tests unnecessarily slow
time.Aftercreates a goroutine leak- No assertion that the function actually attempted to wait
t.Run("context expired", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
+ defer cancel()
+ waited := make(chan struct{})
funcRan := false
err := DoUntilCtxExpired(ctx, func() {
cancel()
funcRan = true
- <-time.After(time.Second)
+ close(waited)
+ <-ctx.Done()
})
+ <-waited
require.ErrorIs(t, err, context.Canceled)
require.True(t, funcRan)
})📝 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.
| t.Run("context expired", func(t *testing.T) { | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| funcRan := false | |
| err := DoUntilCtxExpired(ctx, func() { | |
| cancel() | |
| funcRan = true | |
| <-time.After(time.Second) | |
| }) | |
| require.ErrorIs(t, err, context.Canceled) | |
| require.True(t, funcRan) | |
| }) | |
| t.Run("context expired", func(t *testing.T) { | |
| ctx, cancel := context.WithCancel(context.Background()) | |
| defer cancel() | |
| waited := make(chan struct{}) | |
| funcRan := false | |
| err := DoUntilCtxExpired(ctx, func() { | |
| cancel() | |
| funcRan = true | |
| close(waited) | |
| <-ctx.Done() | |
| }) | |
| <-waited | |
| require.ErrorIs(t, err, context.Canceled) | |
| require.True(t, funcRan) | |
| }) |
| @@ -222,8 +223,7 @@ func (s *Server[T]) Stop(ctx context.Context) error { | |||
| } | |||
|
|
|||
| s.logger.Info("stopping gRPC server...", "address", s.config.Address) | |||
| s.grpcSrv.GracefulStop() | |||
| return nil | |||
| return api.DoUntilCtxExpired(ctx, s.grpcSrv.GracefulStop) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a configurable shutdown timeout.
While using DoUntilCtxExpired with GracefulStop is good for context-aware shutdown, consider adding a configurable timeout in the server config to prevent potentially long shutdown times with hanging connections.
Add a timeout configuration to the Config struct:
type Config struct {
// ... existing fields ...
ShutdownTimeout time.Duration `mapstructure:"shutdown_timeout"`
}Then use it in the Stop method:
func (s *Server[T]) Stop(ctx context.Context) error {
if !s.config.Enable {
return nil
}
s.logger.Info("stopping gRPC server...", "address", s.config.Address)
// Create a timeout context if shutdown timeout is configured
if s.config.ShutdownTimeout > 0 {
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(ctx, s.config.ShutdownTimeout)
defer cancel()
}
return api.DoUntilCtxExpired(ctx, s.grpcSrv.GracefulStop)
}
* main: (24 commits) build(deps): upgrade to iavl@v1.3.1 (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
Description
Cleans up unused interface methods and honours contexts.
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
DoUntilCtxExpired, for improved context management during function execution.DoUntilCtxExpiredfunction to ensure proper behavior under various conditions.