fix(server/v2): respect context cancellation in start command #22346
Conversation
When running external tests that involve starting a v2 server, the only way to get the command to shut down was through sending an interrupt or termination signal, which is not possible to do independently through many concurrent serer commands within the same process. Respect the root context being cancelled, as a signal that the server needs to be stopped. Unfortunately there are no existing unit tests in the server/v2 package with running a start command, that can be expanded to cover this behavior.
mark-rushakoff
requested review from
julienrbrt,
hieuvubk and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
📜 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
|
|
@mark-rushakoff your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
server/v2/commands.go (1)
123-131: Add signal cleanup to prevent resource leaks.The implementation correctly handles both OS signals and context cancellation. However, the signal channel should be cleaned up to prevent potential resource leaks.
Consider adding signal cleanup:
go func() { sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigCh) select { case sig := <-sigCh: cancelFn() cmd.Printf("caught %s signal\n", sig.String()) case <-ctx.Done(): cancelFn() }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- server/v2/commands.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (1)
server/v2/commands.go (1)
123-131: 🛠️ Refactor suggestionConsider using sync.Once for cancelFn to prevent potential race conditions.
There's a potential race condition where cancelFn could be called twice if both a signal is received and the context is cancelled simultaneously. While this might not cause issues with the current context.CancelFunc implementation, it's better to be explicit about ensuring single execution.
Consider using sync.Once:
go func() { + var once sync.Once sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) + defer signal.Stop(sigCh) select { case sig := <-sigCh: - cancelFn() + once.Do(cancelFn) cmd.Printf("caught %s signal\n", sig.String()) case <-ctx.Done(): - cancelFn() + once.Do(cancelFn) }Let's verify the potential race condition:
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
When running external tests that involve starting a v2 server, the only way to get the command to shut down was through sending an interrupt or termination signal, which is not possible to do independently through many concurrent serer commands within the same process.
Respect the root context being cancelled, as a signal that the server needs to be stopped.
Unfortunately there are no existing unit tests in the server/v2 package with running a start command, that can be expanded to cover this behavior.
Summary by CodeRabbit
New Features
Bug Fixes