🔥 feat: Improve and Optimize ShutdownWithContext Func #3162
Conversation
- Reorder mutex lock acquisition to the start of the function - Early return if server is not running - Use defer for executing shutdown hooks - Simplify nil check for hooks - Remove TODO comment This commit improves the readability, robustness, and execution order of the shutdown process. It ensures consistent state throughout the shutdown and guarantees hook execution even in error cases.
- Add shutdown hook verification - Implement better synchronization with channels - Improve error handling and assertions - Adjust timeouts for more consistent results - Add server state check after shutdown attempt - Include comments explaining expected behavior This commit improves the comprehensiveness and reliability of the ShutdownWithContext test, ensuring proper verification of shutdown hooks, timeout behavior, and server state during long-running requests.
|
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 |
JIeJaitt
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3162 +/- ##
==========================================
- Coverage 84.15% 84.14% -0.01%
==========================================
Files 116 116
Lines 11551 11558 +7
==========================================
+ Hits 9721 9726 +5
- Misses 1399 1401 +2
Partials 431 431
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
app_test.go
Outdated
| }() | ||
|
|
||
| <-clientDone | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
why do we need a sleep here? i think it's arbitrary since we already wait for clientDone channel
There was a problem hiding this comment.
why do we need a sleep here? i think it's arbitrary since we already wait for clientDone channel
This sleep is to ensure that our server has started processing the request. Although we have confirmed that the client has sent the request through <-clientDone, this means that the request has been sent to the server's listening port, and there is no guarantee that the server's processing coroutine has been sent. Start processing the request, so I think sleep 100 * Millisecond is required here as well.
JIeJaitt
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Rate limit exceeded@JIeJaitt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes enhance the Fiber web framework by introducing a new method 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 (
|
gaby
changed the title
gaby
changed the title
…e/improve-shutdown-with-context
…ub.com:JIeJaitt/fiber into jiejaitt-feature/improve-shutdown-with-context
![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 (1)
docs/api/fiber.md (1)
208-208: Approve with minor suggestions for improvementThe added description for
ShutdownWithContextis informative and aligns well with the PR objectives. It provides important details about the method's behavior, especially regarding the execution of shutdown hooks. To enhance clarity and readability, consider the following suggestions:
- Add a comma after "including" for better sentence structure.
- Split the long sentence into two for improved readability.
Here's a suggested revision:
- ShutdownWithContext shuts down the server including by force if the context's deadline is exceeded.Shutdown hooks will still be executed, even if an error occurs during the shutdown process, as they are deferred to ensure cleanup happens regardless of errors. + ShutdownWithContext shuts down the server, including by force if the context's deadline is exceeded. Shutdown hooks will still be executed, even if an error occurs during the shutdown process, as they are deferred to ensure cleanup happens regardless of errors.🧰 Tools
🪛 LanguageTool
[uncategorized] ~208-~208: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- app.go (1 hunks)
- app_test.go (1 hunks)
- docs/api/fiber.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/api/fiber.md
[uncategorized] ~208-~208: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (1)
app.go (1)
846-855: Improved shutdown process with deferred hook executionThe changes to the
ShutdownWithContextmethod enhance the shutdown process in several ways:
- Thread-safety is ensured by acquiring a mutex lock at the beginning of the method.
- A nil check for the server prevents potential panics if the server hasn't been initialized.
- The deferral of
executeOnShutdownHooksguarantees that shutdown hooks are executed even if an error occurs during the shutdown process.These improvements contribute to a more robust and reliable shutdown mechanism.
app_test.go
Outdated
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) | ||
| defer cancel() | ||
| shutdownErr <- app.ShutdownWithContext(ctx) | ||
| }() |
There was a problem hiding this comment.
Ensure Proper Context Cancellation
The context created with context.WithTimeout should have its cancellation function deferred immediately to prevent potential resource leaks, even if an error occurs before the defer statement is reached.
Suggested Improvement
Move the defer cancel() directly after context creation.
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+ defer cancel()
shutdownErr <- app.ShutdownWithContext(ctx)
- defer cancel()📝 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.
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) | |
| defer cancel() | |
| shutdownErr <- app.ShutdownWithContext(ctx) | |
| }() | |
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) | |
| defer cancel() | |
| shutdownErr <- app.ShutdownWithContext(ctx) | |
| }() |
app_test.go
Outdated
| case err := <-serverErr: | ||
| assert.NoError(t, err, "Server should have shut down without error") | ||
| default: | ||
| // Server is still running, which is expected as the long-running request prevented full shutdown | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Skipping Server Error Handling
Using a default case in the select statement may cause the test to proceed without waiting for the server to shut down, potentially leading to flaky tests. It's important to handle the server error explicitly.
Suggested Fix: Remove the default Case
This ensures the test waits for serverErr to receive an error or completes the shutdown process.
select {
case err := <-serverErr:
assert.NoError(t, err, "Server should have shut down without error")
- default:
- // Server is still running, which is expected as the long-running request prevented full 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.
| case err := <-serverErr: | |
| assert.NoError(t, err, "Server should have shut down without error") | |
| default: | |
| // Server is still running, which is expected as the long-running request prevented full shutdown | |
| } | |
| case err := <-serverErr: | |
| assert.NoError(t, err, "Server should have shut down without error") |
app_test.go
Outdated
| shutdownHookCalled := false | ||
| app.Hooks().OnShutdown(func() error { | ||
| shutdownHookCalled = true | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Possible Data Race on shutdownHookCalled Variable
The shutdownHookCalled boolean is accessed by multiple goroutines without proper synchronization, which could lead to a data race. In Go, variables shared between goroutines should be protected using synchronization mechanisms like channels, mutexes, or atomic operations.
Suggested Fix: Use an Atomic Variable
Replace shutdownHookCalled with an int32 and use atomic.StoreInt32 and atomic.LoadInt32 for thread-safe operations.
+import "sync/atomic"
- shutdownHookCalled := false
+ var shutdownHookCalled int32
app.Hooks().OnShutdown(func() error {
- shutdownHookCalled = true
+ atomic.StoreInt32(&shutdownHookCalled, 1)
return nil
})
// ...
- assert.True(t, shutdownHookCalled, "Shutdown hook was not called")
+ assert.Equal(t, int32(1), atomic.LoadInt32(&shutdownHookCalled), "Shutdown hook was not called")Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
hooks_test.go (1)
202-248: Improve synchronization in error propagation test.While the test effectively validates error propagation, consider enhancing the synchronization:
- Add a timeout to channel operations to prevent potential deadlocks
- Use a WaitGroup to ensure proper server shutdown before test completion
t.Run("should execute post shutdown hook with error", func(t *testing.T) { app := New() expectedErr := errors.New("test shutdown error") + var wg sync.WaitGroup + wg.Add(1) // Use channels to synchronize and pass results hookResult := make(chan struct { err error called bool - }, 1) + }, 1) // Buffer size of 1 prevents deadlock + defer close(hookResult) app.Hooks().OnPostShutdown(func(err error) error { hookResult <- struct { err error called bool }{ err: err, called: true, } return nil }) // Use channel to make sure the server is up serverReady := make(chan struct{}) + defer close(serverReady) go func() { + defer wg.Done() serverReady <- struct{}{} // Signal that the server is ready to start if err := app.Listen(":0"); err != nil { t.Errorf("Failed to start listener: %v", err) } }() select { case <-serverReady: // Wait for the server to be ready + case <-time.After(5 * time.Second): + t.Fatal("Server startup timeout") } app.hooks.executeOnPostShutdownHooks(expectedErr) // Wait for the hook to finish executing and get the result - result := <-hookResult + var result struct { + err error + called bool + } + select { + case result = <-hookResult: + case <-time.After(5 * time.Second): + t.Fatal("Hook execution timeout") + } if !result.called { t.Fatal("hook was not called") } if !errors.Is(result.err, expectedErr) { t.Fatalf("hook received wrong error: want %v, got %v", expectedErr, result.err) } + wg.Wait() // Ensure server goroutine completes })app_test.go (1)
993-994: Replace sleep with proper synchronization.Using
time.Sleepfor test synchronization can lead to flaky tests. Consider using a channel to signal when the request has started processing.+ requestStarted := make(chan struct{}) app.Get("/", func(c Ctx) error { + close(requestStarted) time.Sleep(2 * time.Second) return c.SendString("OK") }) // ... connection setup ... - time.Sleep(100 * time.Millisecond) // Wait for request to start + select { + case <-requestStarted: + // Request has started processing + case <-time.After(time.Second): + t.Fatal("Request did not start in time") + }docs/api/hooks.md (1)
190-204: Add usage examples for the new shutdown hooks.The documentation would benefit from examples showing how to use
OnPreShutdownandOnPostShutdownhooks, similar to other hook examples in the document.// Example for OnPreShutdown app.Hooks().OnPreShutdown(func() error { log.Println("Preparing for shutdown...") return nil }) // Example for OnPostShutdown app.Hooks().OnPostShutdown(func(err error) error { if err != nil { log.Printf("Shutdown completed with error: %v\n", err) } else { log.Println("Shutdown completed successfully") } return nil })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app_test.go(3 hunks)docs/api/hooks.md(3 hunks)docs/whats_new.md(2 hunks)hooks_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
hooks_test.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
app_test.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
🪛 LanguageTool
docs/whats_new.md
[grammar] ~170-~170: It appears that hyphens are missing.
Context: ...ecated OnShutdown in favor of the new pre/post shutdown hooks - Improved shutdown hook executio...
(PRE_AND_POST_NN)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: Analyse
🔇 Additional comments (7)
hooks_test.go (3)
184-200: LGTM! Well-structured test for pre-shutdown hook.The test effectively validates the pre-shutdown hook execution and cleanup. Good use of bytebufferpool for efficient memory management.
250-274: LGTM! Well-structured test for hook execution order.The test effectively validates that multiple hooks are executed in the correct order.
276-287: LGTM! Good error handling test.The test properly verifies that hook errors are handled gracefully without causing panics.
app_test.go (3)
849-858: Well-implemented server startup synchronization!The use of a channel to coordinate server startup is a good practice that helps avoid race conditions and ensures proper test timing.
892-925: Well-structured test with proper error handling and cleanup!The test case effectively validates the basic shutdown functionality with:
- Proper context handling and cleanup
- Comprehensive error checks
- Clear request/response flow
927-961: Excellent hook execution order test with proper thread safety!The test effectively validates the shutdown hook functionality:
- Uses mutex to ensure thread-safe access to shared state
- Verifies correct execution order of pre/post shutdown hooks
- Includes proper error handling
docs/whats_new.md (1)
163-185: Excellent migration guide with clear examples!The documentation effectively explains the changes to the shutdown hooks system with:
- Clear explanation of new features
- Helpful code examples showing before/after changes
- Important usage notes about goroutines
🧰 Tools
🪛 LanguageTool
[grammar] ~170-~170: It appears that hyphens are missing.
Context: ...ecatedOnShutdownin favor of the new pre/post shutdown hooks - Improved shutdown hook executio...(PRE_AND_POST_NN)
|
@ReneWerner87 @efectn @gaby I've updated what_new.md and hooks.md, but I'm now experiencing a large number of failed Github Action tests, it appears to be a performance issue, the code is something I don't have much of a clue about at the moment. |
|
we will have a look |
very thankful |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hooks_test.go (1)
239-239: 🛠️ Refactor suggestionReplace sleep with proper synchronization.
Using
time.Sleepfor synchronization is unreliable and can lead to flaky tests. Consider using a channel to signal when the server is ready.- // Wait a bit for the server to start - time.Sleep(100 * time.Millisecond) + // Use channel to signal server readiness + ready := make(chan struct{}) + go func() { + defer close(ready) + if err := app.Listen(":0"); err != nil { + if !errors.Is(err, errors.New("server closed")) { + t.Errorf("unexpected error: %v", err) + } + } + }() + <-ready
🧹 Nitpick comments (1)
hooks_test.go (1)
232-236: Improve error handling in Listen.The error handling for
app.Listencan be simplified using testify'srequirepackage.- if err := app.Listen(":0"); err != nil { - if !errors.Is(err, errors.New("server closed")) { - t.Errorf("unexpected error: %v", err) - } - } + err := app.Listen(":0") + require.True(t, err == nil || errors.Is(err, errors.New("server closed")), "unexpected error: %v", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hooks_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
hooks_test.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.23.x, ubuntu-latest)
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: Compare
🔇 Additional comments (1)
hooks_test.go (1)
185-201: LGTM! Clear and focused test for pre-shutdown hook.The test case effectively validates the pre-shutdown hook execution and maintains a clear, single responsibility.
|
@ReneWerner87 @efectn @gaby all github action check succeed. Please review the latest code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/api/fiber.md (1)
231-231: Add a comma for better readability.Add a comma after "server" to improve readability.
Apply this diff:
-ShutdownWithContext shuts down the server including by force if the context's deadline is exceeded. +ShutdownWithContext shuts down the server, including by force if the context's deadline is exceeded.🧰 Tools
🪛 LanguageTool
[uncategorized] ~231-~231: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/api/fiber.md(2 hunks)listen.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
listen.go
[warning] 500-500: listen.go#L500
Added line #L500 was not covered by tests
🪛 LanguageTool
docs/api/fiber.md
[uncategorized] ~231-~231: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
listen.go (1)
94-94: LGTM!The documentation update correctly reflects the new hook-based error handling approach.
docs/api/fiber.md (1)
114-114: LGTM!The documentation for
ShutdownTimeoutis clear, comprehensive, and accurately describes the timeout behavior and its impact on hooks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/api/hooks.md (1)
180-186: Clarify Deprecation of OnShutdown Hook
The section begins by noting that "Since v3, theOnShutdownhook is split intoOnPreShutdownandOnPostShutdown." However, the documentation still includes a signature forOnShutdown(i.e.func (h *Hooks) OnShutdown(handler ...OnShutdownHandler)). To avoid potential confusion, consider marking this hook as deprecated or removing it entirely if it is no longer supported by the framework.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/api/hooks.md(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.23.x, ubuntu-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: Analyse
🔇 Additional comments (4)
docs/api/hooks.md (4)
19-20: New Hooks Listed: OnPreShutdown and OnPostShutdown
The navigation list now includes the two new hooks. Please verify that the link anchors (#onpreshutdownand#onpostshutdown) correctly match the corresponding section headings later in the document.
33-34: New Handler Type Declarations
The new type declarations forOnPreShutdownHandlerandOnPostShutdownHandlerare well-defined. The signature forOnPreShutdownHandler(no parameters, returns an error) and forOnPostShutdownHandler(accepting an error and returning an error) clearly convey their intended use.
190-194: Documentation for OnPreShutdown Hook
The new OnPreShutdown section clearly documents its purpose and provides an accurate code signature. This enhancement helps users understand how to register pre-shutdown hooks that execute before the shutdown process begins.
195-199: Documentation for OnPostShutdown Hook
The OnPostShutdown documentation is clearly presented, indicating that the hook is executed after shutdown and is intended for error handling. This addition improves the overall clarity of the shutdown hook mechanism and guides users on how to implement post-shutdown logic.
|
@JIeJaitt last two doc changes and then i will merge |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
listen_test.go (3)
92-94: Improve error handling in connection close.Consider wrapping the error with additional context to aid debugging.
-if err := conn.Close(); err != nil { - t.Logf("error closing connection: %v", err) -} +if err := conn.Close(); err != nil { + t.Logf("error closing test connection during readiness check: %v", err) +}
113-130: Consider adding more test coverage.While the current test cases cover basic scenarios, consider adding edge cases such as:
- Server shutdown during an active request
- Multiple concurrent requests during shutdown
- Very large response bodies
157-160: Add comments explaining timeout behavior.The nanosecond timeout test case could benefit from comments explaining why this specific timeout value was chosen and what behavior it's testing.
+// Using 1 nanosecond timeout to force a context.DeadlineExceeded error, +// verifying that the shutdown process respects the timeout configuration if shutdownTimeout == 1*time.Nanosecond { require.Error(t, receivedErr) require.ErrorIs(t, receivedErr, context.DeadlineExceeded) }docs/api/hooks.md (2)
18-19: New Shutdown Hooks in Bullet ListThe updated bullet list now correctly includes the new OnPreShutdown and OnPostShutdown hooks. Ensure that any references to the obsolete shutdown hook are fully removed from the documentation.
178-181: Address Multiple Consecutive Blank LinesStatic analysis flagged multiple consecutive blank lines (MD012) around this section (lines 179-180). Please remove the extra blank lines to comply with markdownlint guidelines. For example, consolidate the blank lines to a single one.
🧰 Tools
🪛 GitHub Check: markdownlint
[failure] 179-179: Multiple consecutive blank lines
docs/api/hooks.md:179 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
[failure] 180-180: Multiple consecutive blank lines
docs/api/hooks.md:180 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md🪛 GitHub Actions: markdownlint
[error] 179-179: MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/api/hooks.md(3 hunks)listen_test.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: markdownlint
docs/api/hooks.md
[failure] 180-180: Multiple consecutive blank lines
docs/api/hooks.md:180 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
🪛 GitHub Actions: markdownlint
docs/api/hooks.md
[error] 179-179: MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (5)
listen_test.go (2)
40-52: Well-structured test organization!The test has been effectively split into three distinct scenarios with clear names and appropriate timeout values, improving readability and maintainability.
54-56: LGTM! Follows test helper best practices.The helper function is properly declared with t.Helper() and has clear parameter names.
docs/api/hooks.md (3)
32-33: New Handler Type DeclarationsThe new type definitions for
OnPreShutdownHandlerandOnPostShutdownHandlerare added and follow the expected signatures. This change aligns well with the split of the shutdown hook functionality.
180-185: Documentation for OnPreShutdown HookThe OnPreShutdown section clearly describes the hook and provides an accurate function signature for registering pre-shutdown callbacks. This documentation meets the new design requirements and clarifies the hook’s intent.
🧰 Tools
🪛 GitHub Check: markdownlint
[failure] 180-180: Multiple consecutive blank lines
docs/api/hooks.md:180 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md
187-191: Documentation for OnPostShutdown HookThe OnPostShutdown section is properly updated with a clear description and corresponding signature. This correctly informs users of its purpose in executing post-shutdown callbacks.
…ub.com:JIeJaitt/fiber into jiejaitt-feature/improve-shutdown-with-context
I have modified the document according to the discussion. |
Description
Optimize
ShutdownWithContextfunction and elegant shutdown flow of fiber, streamlineListenstructureChanges introduced
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.