🔥 feat: Add support for graceful shutdown timeout in ListenConfig #3220
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe pull request updates the Fiber package documentation to include a new configuration field, Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
ksw2000
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
after your feature and the release for v3 we can adjust |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
listen.go (1)
96-101: Documentation could be more descriptiveWhile the documentation explains the basic functionality, it could be enhanced with:
- A more detailed explanation of the shutdown process behavior when timeout occurs
- An example usage showing how to configure the timeout
Consider expanding the documentation like this:
- // When the graceful shutdown begins, use this field to set the timeout - // duration. If the timeout is reached, OnShutdownError will be called. - // The default value is 0, which means the timeout setting is disabled. + // GracefulShutdownTimeout sets a time limit for graceful shutdown operations. + // When a shutdown begins, the server will wait for existing connections to complete + // within this duration. If the timeout is reached before all connections are closed: + // 1. A context.DeadlineExceeded error will be triggered + // 2. The error will be passed to OnShutdownError callback + // 3. Any remaining connections will be forcefully closed + // + // Example usage: + // app.Listen(":3000", fiber.ListenConfig{ + // GracefulShutdownTimeout: 5 * time.Second, + // OnShutdownError: func(err error) { + // log.Printf("Shutdown error: %v", err) + // }, + // }) + // + // Default: 0 (timeout disabled)docs/api/fiber.md (2)
Line range hint
379-391: Enhance Server Shutdown documentation sectionThe Server Shutdown section should be updated to reflect the new timeout functionality and provide clear examples.
Add comprehensive documentation about the shutdown behavior:
## Server Shutdown Shutdown gracefully shuts down the server without interrupting any active connections. Shutdown works by first closing all open listeners and then waits indefinitely for all connections to return to idle before shutting down. -ShutdownWithTimeout will forcefully close any active connections after the timeout expires. +ShutdownWithTimeout will forcefully close any active connections after the specified timeout expires. This is equivalent to setting GracefulShutdownTimeout in ListenConfig. ShutdownWithContext shuts down the server including by force if the context's deadline is exceeded. ```go func (app *App) Shutdown() error func (app *App) ShutdownWithTimeout(timeout time.Duration) error func (app *App) ShutdownWithContext(ctx context.Context) error
+```go title="Example with GracefulShutdownTimeout"
+app := fiber.New()
+
+// Configure graceful shutdown with a 5-second timeout
+app.Listen(":8080", fiber.ListenConfig{
- GracefulShutdownTimeout: 5 * time.Second,
- OnShutdownError: func(err error) {
if err == context.DeadlineExceeded {log.Println("Graceful shutdown timed out")}- },
+})
+```--- `111-111`: **Add historical context and behavior change notes** To provide better context for users, consider adding a note about the behavioral changes. Add a note about the behavioral change: ```diff -| <Reference id="gracefulshutdowntimeout">GracefulShutdownTimeout</Reference>| `time.Duration` | Field to set the timeout for graceful shutdown. Set to 0 to disable the timeout. | `0` | +| <Reference id="gracefulshutdowntimeout">GracefulShutdownTimeout</Reference>| `time.Duration` | Field to set the timeout for graceful shutdown. Set to 0 to disable the timeout. When timeout occurs, OnShutdownError is called with context.DeadlineExceeded error, addressing a long-standing issue where OnShutdownSuccess was incorrectly called even when shutdown failed. | `0` |listen_test.go (1)
155-167: Consider refactoring duplicated code in the test setup and loopsThe server readiness checks and test loops in
Test_Listen_Graceful_Shutdown_Timeoutare similar to those inTest_Listen_Graceful_Shutdown. Refactoring common code into helper functions can reduce duplication and improve maintainability.Also applies to: 195-217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/api/fiber.md(1 hunks)listen.go(3 hunks)listen_test.go(1 hunks)
🔇 Additional comments (1)
listen.go (1)
481-491: 🛠️ Refactor suggestion
Address TODO comments and verify context implementation
The implementation looks solid, but there are two TODO comments about context implementation that need to be addressed.
Let's verify the context implementation in the codebase:
Consider refactoring the error handling to be more concise:
- var err error
-
- if cfg.GracefulShutdownTimeout != 0 {
- err = app.ShutdownWithTimeout(cfg.GracefulShutdownTimeout) //nolint:contextcheck // TODO: Implement it
- } else {
- err = app.Shutdown() //nolint:contextcheck // TODO: Implement it
- }
-
- if err != nil {
- cfg.OnShutdownError(err)
- return
- }
+ err := app.Shutdown()
+ if cfg.GracefulShutdownTimeout != 0 {
+ err = app.ShutdownWithTimeout(cfg.GracefulShutdownTimeout)
+ }
+
+ if err != nil {
+ cfg.OnShutdownError(err)
+ return
+ }
|
please always consider the comments of our ai, there are usually many, but this leads to an enormous increase in code quality |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
listen_test.go (4)
118-152: LGTM! Consider using more descriptive variable names.The test setup is well-structured with proper synchronization. However, consider renaming
errstoserverErrorsfor better clarity about its purpose.-errs := make(chan error) +serverErrors := make(chan error)
154-167: Extract magic numbers into named constants.The retry mechanism is well-implemented, but the magic numbers should be constants for better maintainability.
+const ( + maxRetries = 10 + retryDelay = 100 * time.Millisecond +) -for i := 0; i < 10; i++ { +for i := 0; i < maxRetries; i++ { conn, err := ln.Dial() if err == nil { _ = conn break } - time.Sleep(100 * time.Millisecond) + time.Sleep(retryDelay) - if i == 9 { + if i == maxRetries-1 { t.Fatalf("Server did not become ready in time: %v", err) } }
169-193: Consider adding edge cases to improve test coverage.The test cases effectively cover the basic success and timeout scenarios. Consider adding these edge cases:
- Timeout exactly at the boundary (500ms)
- Multiple concurrent connections
- Zero timeout value
testCases := []struct { ExpectedErr error ExpectedShutdownError error ExpectedBody string Time time.Duration ExpectedStatusCode int ExpectedShutdownSuccess bool }{ // ... existing cases ... + { + Time: 500 * time.Millisecond, + ExpectedBody: "", + ExpectedStatusCode: StatusOK, + ExpectedErr: errors.New("connection closed"), + ExpectedShutdownError: context.DeadlineExceeded, + ExpectedShutdownSuccess: false, + }, + { + Time: 0, + ExpectedBody: "example.com", + ExpectedStatusCode: StatusOK, + ExpectedErr: nil, + ExpectedShutdownError: nil, + ExpectedShutdownSuccess: false, + }, }
195-228: Consider enabling parallel test execution.The test execution is well-structured with proper error handling and resource cleanup. Consider enabling parallel test execution to improve test performance.
func Test_Listen_Graceful_Shutdown_Timeout(t *testing.T) { + t.Parallel() var mu sync.Mutex // ... rest of the test }Note: Ensure all test dependencies are properly isolated before enabling parallel execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docs/api/fiber.md(1 hunks)listen_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api/fiber.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
listen_test.go (2)
159-159: Add a comment explaining why the connection is not closedThe blank identifier usage could be clearer. Consider adding a comment explaining that we're intentionally keeping the connection open to test the timeout scenario.
- _ = conn + // Intentionally keep connection open to test shutdown timeout + _ = conn
154-167: Consider extracting the server readiness check into a helper functionThis readiness check logic is duplicated across multiple tests. Consider extracting it into a helper function to improve maintainability.
+func waitForServerReady(t *testing.T, ln *fasthttputil.InmemoryListener, keepConnOpen bool) { + for i := 0; i < 10; i++ { + conn, err := ln.Dial() + if err == nil { + if !keepConnOpen { + conn.Close() + } + return + } + time.Sleep(100 * time.Millisecond) + if i == 9 { + t.Fatalf("Server did not become ready in time: %v", err) + } + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
listen_test.go(2 hunks)
🔇 Additional comments (2)
listen_test.go (2)
88-88: LGTM! Good improvement in error handling
The change from string error to fasthttputil.ErrInmemoryListenerClosed follows Go's best practices for error handling.
169-218: LGTM! Well-structured test cases and proper validation
The test implementation is thorough and well-structured:
- Comprehensive test cases covering both success and timeout scenarios
- Thread-safe state validation using mutex
- Proper error handling using
ErrorIs
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/api/fiber.md (1)
111-111: LGTM! Consider adding cross-references to related fields.The documentation for
GracefulShutdownTimeoutis clear, comprehensive, and well-structured. It effectively explains the purpose, behavior, and default value of the field.Consider adding cross-references to related fields like
GracefulContext,OnShutdownError, andOnShutdownSuccessto help users understand how these fields work together. For example:-| <Reference id="gracefulshutdowntimeout">GracefulShutdownTimeout</Reference>| `time.Duration` | Specifies the maximum duration to wait for the server to gracefully shutdown. When the timeout is reached, the graceful shutdown process is interrupted and forcibly terminated, and the `context.DeadlineExceeded` error is passed to the `OnShutdownError` callback. Set to 0 (default) to disable the timeout and wait indefinitely. | `0` | +| <Reference id="gracefulshutdowntimeout">GracefulShutdownTimeout</Reference>| `time.Duration` | Specifies the maximum duration to wait for the server to gracefully shutdown. When the timeout is reached, the graceful shutdown process is interrupted and forcibly terminated, and the `context.DeadlineExceeded` error is passed to the <Reference id="onshutdownerror">OnShutdownError</Reference> callback. Can be used in conjunction with <Reference id="gracefulcontext">GracefulContext</Reference> for more control over the shutdown process. Set to 0 (default) to disable the timeout and wait indefinitely. | `0` |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3220 +/- ##
==========================================
+ Coverage 82.73% 82.86% +0.13%
==========================================
Files 115 115
Lines 11287 11295 +8
==========================================
+ Hits 9338 9360 +22
+ Misses 1547 1537 -10
+ Partials 402 398 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/api/fiber.md(1 hunks)listen.go(3 hunks)listen_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- listen_test.go
- docs/api/fiber.md
🔇 Additional comments (2)
listen.go (2)
21-21: Import of the 'time' package is appropriate
The addition of the time package is necessary for using time.Duration in the ShutdownTimeout field.
96-101: Consider setting a non-zero default value for ShutdownTimeout
Currently, ShutdownTimeout defaults to 0, which disables the timeout and could lead to indefinite waits during graceful shutdown. This might cause issues like the application being forcibly terminated by external systems (e.g., Docker sends a SIGKILL after a default timeout).
Consider setting a reasonable default value, such as 10 * time.Second, to prevent indefinite blocking during shutdown.
| err = app.ShutdownWithTimeout(cfg.ShutdownTimeout) //nolint:contextcheck // TODO: Implement it | ||
| } else { | ||
| err = app.Shutdown() //nolint:contextcheck // TODO: Implement it |
There was a problem hiding this comment.
Address the TODO comments and implement context handling in shutdown methods
The // TODO: Implement it comments indicate that app.ShutdownWithTimeout and app.Shutdown may not fully handle the context as intended. Ensure that these methods are properly implemented to facilitate graceful shutdown within the specified timeout.
Additionally, after implementation, remove the //nolint:contextcheck directives to comply with linting standards.
Would you like assistance in implementing the context handling for ShutdownWithTimeout and Shutdown methods?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
listen.go (1)
482-492: Consider error handling enhancement.The error handling in gracefulShutdown could be more descriptive. Consider wrapping the error with additional context about the timeout duration when it occurs.
if err != nil { + if cfg.ShutdownTimeout != 0 && err == context.DeadlineExceeded { + err = fmt.Errorf("shutdown timeout after %v: %w", cfg.ShutdownTimeout, err) + } cfg.OnShutdownError(err) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docs/api/fiber.md(1 hunks)listen.go(5 hunks)
🔇 Additional comments (3)
listen.go (2)
96-101: LGTM: Well-documented field with clear description.
The ShutdownTimeout field is properly documented with clear explanation of its purpose and behavior.
484-488:
Critical: TODO comments need to be addressed.
The TODO comments indicate that context handling is not yet implemented in ShutdownWithTimeout and Shutdown methods. This could lead to improper resource cleanup during shutdown.
Let's verify the implementation of these methods:
docs/api/fiber.md (1)
113-113: LGTM: Documentation is accurate and comprehensive.
The documentation for ShutdownTimeout is well-written, clearly explaining:
- The purpose of the field
- The behavior when timeout occurs
- The interaction with OnShutdownError
- The default value
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
Implement the graceful shutdown timeout in
ListenConfig. This feature has been left idle as a TODO comment since September 2020.Users can use the
GracefulShutdownTimeoutfield inListenConfigto set a time limit for the graceful shutdown. When a timeout occurs, acontext.DeadlineExceedederror will be passed to theOnShutdownErrordefined inListenConfig.Additionally, in the original implementation, the
OnShutdownSuccessfunction was called even when an error occurred, which is unexpected behavior.Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md