refactor(server): improve logging and handle shutdown signals with ease #39
Conversation
WalkthroughThe pull request modifies the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
cmd/capture/main.go (1)
Server timeouts need configuration flexibility
The timeouts are currently hardcoded:
- ReadHeaderTimeout: 5 seconds
- Shutdown timeout: 5 seconds
These should be configurable via environment variables since:
- The application already uses environment-based configuration (PORT, API_SECRET)
- The Config struct in
internal/config/config.gocan be extended- Fixed timeout values may not suit all deployment scenarios
Recommendations:
- Add timeout configuration to the Config struct
- Use environment variables like
READ_HEADER_TIMEOUTandSHUTDOWN_TIMEOUT- Provide sensible defaults (current 5-second values) when env vars are not set
🔗 Analysis chain
Line range hint
41-43: Verify timeout values for security implicationsThe implementation uses two different timeouts:
- ReadHeaderTimeout: 5 seconds for incoming requests
- Shutdown context timeout: 5 seconds for graceful shutdown
These values might need adjustment based on:
- Production load characteristics
- Average request processing time
- Connection pool cleanup requirements
Also applies to: 61-62
Let me search for any configuration handling or environment variables that might be used for these timeouts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other timeout configurations in the codebase for consistency rg -i 'timeout|duration' --type go # Look for any existing configuration handling of these timeouts ast-grep --pattern 'time.Second'Length of output: 278
Script:
#!/bin/bash # Search for environment variables or configuration related to timeouts rg -i "env|config.*timeout" --type go -A 3 # Look for any configuration structs or types ast-grep --pattern 'type $_ struct { $$$ }'Length of output: 3831
🧹 Nitpick comments (2)
cmd/capture/main.go (2)
58-58: LGTM! But will need adjustmentThe improved log message is more descriptive and clearer. However, this line should be removed if you implement the suggested force shutdown changes, as the logging will be handled in the signal handling goroutine.
64-64: Consider enhancing error messages furtherWhile the error messages have been improved, they could be even more informative:
- log.Fatal("server shutdown error:", err) + log.Fatal("server shutdown error (timeout after 5s):", err) - log.Println("server has been shutdown.") + log.Println("server has been successfully shutdown.") - log.Fatalf("listen error: %s\n", srvErr) + log.Fatalf("failed to start server on port %s: %s\n", appConfig.Port, srvErr)Also applies to: 69-69, 75-75
…ction - When received a signal early exit if there is no request
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/capture/main.go (2)
54-54: Enhance error message with more contextWhile the error message is improved, consider adding more context about the server's address to help with debugging.
-log.Fatalf("listen error: %s\n", srvErr) +log.Fatalf("failed to start server on %s: %v\n", srv.Addr, srvErr)
46-46: Consider making shutdown timeout configurableThe shutdown timeout is currently hardcoded to 5 seconds. Consider making this configurable through the application config to allow adjustment based on deployment requirements.
Suggested approach:
- Add
ShutdownTimeoutto the config structure- Allow override via environment variable
- Provide a reasonable default value
Example:
// in config/config.go type Config struct { Port string APISecret string + ShutdownTimeout time.Duration } func NewConfig(port, apiSecret string) *Config { + shutdownTimeout := 5 * time.Second + if timeout := os.Getenv("SHUTDOWN_TIMEOUT"); timeout != "" { + if d, err := time.ParseDuration(timeout); err == nil { + shutdownTimeout = d + } + } return &Config{ Port: port, APISecret: apiSecret, + ShutdownTimeout: shutdownTimeout, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/capture/main.go(1 hunks)
🔇 Additional comments (1)
cmd/capture/main.go (1)
46-47:
Force shutdown functionality is missing
While the graceful shutdown implementation is good, it doesn't fulfill the PR objective of handling force shutdown (repeated termination signals). The current implementation will not forcefully close the application on repeated signals.
Consider implementing the force shutdown logic as previously suggested:
func main() {
server := &http.Server{...}
go serve(server)
- if err := gracefulShutdown(server, 5*time.Second); err != nil {
- log.Fatalln("graceful shutdown error", err)
+ if err := gracefulShutdown(server, 5*time.Second, true); err != nil {
+ log.Fatalln("shutdown error:", err)
}
}
-func gracefulShutdown(srv *http.Server, timeout time.Duration) error {
+func gracefulShutdown(srv *http.Server, timeout time.Duration, enableForceShutdown bool) error {
quit := make(chan os.Signal, 1)
+ forceQuit := make(chan struct{})
+ shutdownCount := 0
signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
+ if enableForceShutdown {
+ go func() {
+ for range quit {
+ shutdownCount++
+ if shutdownCount == 1 {
+ log.Println("received exit signal, gracefully shutting down")
+ } else {
+ log.Println("received second exit signal, forcing shutdown!")
+ close(forceQuit)
+ os.Exit(0)
+ }
+ }
+ }()
+ }
- sig := <-quit
- log.Printf("signal received: %v", sig)
+ select {
+ case sig := <-quit:
+ log.Printf("signal received: %v", sig)
+ case <-forceQuit:
+ return nil
+ }Likely invalid or redundant comment.
mertssmnoglu
changed the title
|
Tested it in various scenarios and it works as expected. |
PR #11 missing task will be done with these changes.