feat(recipe/tableflip): Add Graceful shutdown to ensure continuous up…#2710
feat(recipe/tableflip): Add Graceful shutdown to ensure continuous up…#2710ReneWerner87 merged 1 commit intogofiber:masterfrom juls0730:patch-1
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. |
WalkthroughThe changes in this pull request focus on enhancing the shutdown process of the application in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant Main
User->>Main: Initiate shutdown
Main->>Server: Call Shutdown()
Server->>Main: Wait for connections to drain
Main->>Main: Start timeout (30 seconds)
Main->>Main: Log message if not shut down
Main->>Main: Force exit if timeout reached
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tableflip/main.go (2)
51-57: Consider making the shutdown timeout configurableThe timeout implementation is good, but could be improved for better maintainability and observability.
Consider these improvements:
const ( version = "v0.0.1" + shutdownTimeout = 30 * time.Second ) // ... rest of the code ... // Make sure to set a deadline on exiting the process // after upg.Exit() is closed. No new upgrades can be // performed if the parent doesn't exit. - time.AfterFunc(30*time.Second, func() { - log.Println("Graceful shutdown timed out") + time.AfterFunc(shutdownTimeout, func() { + log.Printf("Process did not exit within %s, forcing exit", shutdownTimeout) os.Exit(1) })
Line range hint
41-42: Consider handling errors from app.ListenerThe
app.Listeneris started in a goroutine but any errors it encounters would be silently ignored. This could mask important issues during operation.Consider using an error channel to handle potential errors:
+ errChan := make(chan error, 1) - go app.Listener(ln) + go func() { + if err := app.Listener(ln); err != nil { + log.Printf("Listener error: %v", err) + errChan <- err + upg.Stop() + } + }() // tableflip ready if err := upg.Ready(); err != nil { panic(err) } - <-upg.Exit() + select { + case <-upg.Exit(): + case err := <-errChan: + log.Printf("Shutting down due to listener error: %v", err) + }
…time In the original Cloudflare Tableflip http example, they use a graceful shutdown method to ensure a continuous service uptime. In contrast, with the original fiber recipe, if you send the upgrade signal to the fiber process, current connections will die out (you can test with `ab -c 50 -n 100000 http://127.0.0.1:8080/version` and then send an upgrade signal while the test is running and see how the service dies). This commit fixes this issue with the original recipe and now actually allows for a continuous service uptime.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tableflip/main.go (1)
52-62: Consider adding debug logs for shutdown sequenceTo aid in debugging shutdown-related issues, consider adding log statements to track the shutdown progress.
<-upg.Exit() +log.Println("Received exit signal, initiating graceful shutdown") // Make sure to set a deadline on exiting the process // after upg.Exit() is closed. No new upgrades can be // performed if the parent doesn't exit. time.AfterFunc(30*time.Second, func() { log.Println("Graceful shutdown timed out") os.Exit(1) }) // Wait for connections to drain. +log.Println("Waiting for connections to drain") app.ShutdownWithContext(context.Background()) +log.Println("Shutdown complete")🧰 Tools
🪛 golangci-lint (1.62.2)
62-62: Error return value of
app.ShutdownWithContextis not checked(errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tableflip/main.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
tableflip/main.go
62-62: Error return value of app.ShutdownWithContext is not checked
(errcheck)
🔇 Additional comments (2)
tableflip/main.go (2)
4-4: LGTM: Required imports added
The new imports are necessary for the graceful shutdown implementation.
Also applies to: 10-10
53-59: LGTM: Well-implemented timeout mechanism
The 30-second timeout is a good practice to prevent the process from hanging indefinitely during shutdown.
| // Wait for connections to drain. | ||
| app.ShutdownWithContext(context.Background()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and context management
The shutdown implementation could be improved in two ways:
- Handle the error returned by
ShutdownWithContext - Use a timeout context instead of
context.Background()
Here's the suggested implementation:
// Wait for connections to drain.
-app.ShutdownWithContext(context.Background())
+ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second)
+defer cancel()
+if err := app.ShutdownWithContext(ctx); err != nil {
+ log.Printf("Error during shutdown: %v", err)
+}Note: The context timeout (25s) is set slightly shorter than the process timeout (30s) to ensure orderly shutdown.
📝 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.
| // Wait for connections to drain. | |
| app.ShutdownWithContext(context.Background()) | |
| // Wait for connections to drain. | |
| ctx, cancel := context.WithTimeout(context.Background(), 25*time.Second) | |
| defer cancel() | |
| if err := app.ShutdownWithContext(ctx); err != nil { | |
| log.Printf("Error during shutdown: %v", err) | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
62-62: Error return value of app.ShutdownWithContext is not checked
(errcheck)
There was a problem hiding this comment.
my implementation is identical to how upstream handles it in their reference implementation, so YOLO.
There was a problem hiding this comment.
@juls0730, understood. Thank you for the clarification.
✏️ Learnings added
Learnt from: juls0730
PR: gofiber/recipes#2710
File: tableflip/main.go:61-62
Timestamp: 2024-12-01T01:15:47.983Z
Learning: In the GoFiber `tableflip` recipe (`tableflip/main.go`), the implementation matches the upstream reference implementation. Future code suggestions should consider maintaining this alignment to ensure consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! |
In the original Cloudflare Tableflip http example, they use a graceful shutdown method to ensure a continuous service uptime. In contrast, with the original fiber recipe, if you send the upgrade signal to the fiber process, current connections will die out (you can test with
ab -c 50 -n 100000 http://127.0.0.1:8080/versionand then send an upgrade signal while the test is running and see how the service dies). This commit fixes this issue with the original recipe and now actually allows for a continuous service uptime.Summary by CodeRabbit