🔥 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 82.76% 82.78% +0.02%
==========================================
Files 115 115
Lines 11377 11381 +4
==========================================
+ Hits 9416 9422 +6
+ Misses 1555 1554 -1
+ Partials 406 405 -1
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
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.
…ub.com:JIeJaitt/fiber into jiejaitt-feature/improve-shutdown-with-context
…/improve-shutdown-with-context
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)
- app.go (1 hunks)
- app_test.go (2 hunks)
- docs/api/fiber.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/api/fiber.md
🔇 Additional comments (7)
app.go (2)
883-887: LGTM! Improved concurrency safety and error handling.The changes correctly acquire the mutex lock at the start and add an early return if the server is not running. This prevents unnecessary processing and ensures thread safety.
888-892: LGTM! Robust shutdown hook execution.The changes ensure that shutdown hooks are executed even if errors occur during shutdown by:
- Using
deferto guarantee hook execution- Adding a nil check for hooks
- Maintaining clean code structure with proper spacing
app_test.go (5)
23-23: Importing 'sync/atomic' for safe concurrencyThe import of
sync/atomicis appropriate to enable atomic operations on shared variables, ensuring thread safety in concurrent environments.
864-868: Proper use of atomic variables to avoid data racesThe variable
shutdownHookCalledis correctly declared asatomic.Int32, and atomic methodsStoreandLoadare used to prevent data races, addressing previous concurrency concerns.
899-900: Proper placement ofdefer cancel()after context creationPlacing
defer cancel()immediately after creating the context ensures that resources are released promptly, preventing potential leaks even if an error occurs.
905-909: Improved test reliability by removing thedefaultcase inselectBy removing the
defaultcase, the test now correctly waits for either the shutdown to complete or the timeout to occur, ensuring accurate and reliable test behavior.
912-912: Correctly verifying shutdown hook executionThe assertion confirms that the
shutdownHookCalledvariable is set, verifying that the shutdown hook was indeed called during the shutdown process.
…/improve-shutdown-with-context
JIeJaitt
changed the title
|
false positive for the benchmark because of the same naming @gaby can you check your last comments again to see if it is ready to merge |
|
@JIeJaitt can you make another comment in the whats new markdown? |
No problem, I will add it in what new later |
…/improve-shutdown-with-context
| @@ -205,7 +205,7 @@ Shutdown gracefully shuts down the server without interrupting any active connec | |||
|
|
|||
| ShutdownWithTimeout will forcefully close any active connections after the timeout expires. | |||
|
|
|||
| 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. 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. | |||
There was a problem hiding this comment.
Not sure about this behavior. I think we should only execute them during successful shutdown. Maybe we can OnShutdownError hook. What do you think @gaby @ReneWerner87
There was a problem hiding this comment.
I'm not sure either. Also not sure about the defer of the hook
There was a problem hiding this comment.
I would prefer to add Post Pre hook
There was a problem hiding this comment.
The add Post Pre hook is a good idea, but how do I tell which hooks are pre closed and which are post closed? @efectn
There was a problem hiding this comment.
You can add new hook called PostShutdown, PreShutdown and replace the old one with them
There was a problem hiding this comment.
sound good, @JIeJaitt can you do the suggested change
There was a problem hiding this comment.
This means making a change below the type Hooks struct to add these two fields that mean PreShutdownHandler and PostShutdownHandler respectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gaby
There was a problem hiding this comment.
This means making a change below the
type Hooks structto add these two fields that meanPreShutdownHandlerandPostShutdownHandlerrespectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gaby
Yes it is. You can also add err parameter to PostShutdown hook, so that we can also get rid of OnShutdownError, OnShutdownSuccess properties of ListenerConfig.
There was a problem hiding this comment.
This means making a change below the
type Hooks structto add these two fields that meanPreShutdownHandlerandPostShutdownHandlerrespectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gabyYes it is. You can also add err parameter to PostShutdown hook, so that we can also get rid of OnShutdownError, OnShutdownSuccess properties of ListenerConfig.
OK, I will press on with this idea to realize it again in my free time, by the way what is the v3 deadline as I am busy with my final exams these days.
There was a problem hiding this comment.
This means making a change below the
type Hooks structto add these two fields that meanPreShutdownHandlerandPostShutdownHandlerrespectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gabyYes it is. You can also add err parameter to PostShutdown hook, so that we can also get rid of OnShutdownError, OnShutdownSuccess properties of ListenerConfig.
OK, I will press on with this idea to realize it again in my free time, by the way what is the v3 deadline as I am busy with my final exams these days.
We want to release v3 rc1 in the beginning of January. If you can't look at the PR this week, i can also look no problem
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app_test.go (1)
913-918: Consider removing commented codeThe commented
defaultcase on line 917 should be removed as it's no longer needed.- // default: - // Server is still running, which is expected as the long-running request prevented full shutdown
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app.go(1 hunks)app_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.go
🧰 Additional context used
📓 Learnings (1)
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.
🔇 Additional comments (6)
app_test.go (6)
24-24: LGTM: Import atomic package for thread-safe operations
The addition of the sync/atomic import is appropriate for implementing thread-safe operations in the test.
865-869: LGTM: Thread-safe shutdown hook tracking
The use of atomic.Int32 for tracking the shutdown hook call is a thread-safe approach, ensuring accurate state tracking in concurrent scenarios.
878-881: LGTM: Proper error handling with buffered channel
Using a buffered channel for server errors prevents potential goroutine leaks and ensures errors are properly captured.
885-891: LGTM: Proper client request setup with synchronization
The client request is properly set up with error handling and synchronization using a channel to signal completion.
894-896: Consider removing the sleep after clientDone
The sleep after the clientDone channel might be unnecessary since we already have synchronization through the channel.
Based on the past review comments and learnings, the clientDone channel is sufficient for synchronization, making the sleep redundant.
898-911: LGTM: Robust shutdown handling with timeout
The shutdown handling is well-implemented with:
- Proper context timeout
- Deferred context cancellation
- Clear error assertions for timeout scenarios
JIeJaitt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
hooks.go (2)
105-111: Remove commented-out codeThe TODO comment and commented-out
OnShutdowncode should be removed since it's being replaced by the new pre/post shutdown hooks.-// TODO：To be deleted, replaced by OnPreShutdown and OnPostShutdown -// OnShutdown is a hook to execute user functions after Shutdown. -// func (h *Hooks) OnShutdown(handler ...OnShutdownHandler) { -// h.app.mutex.Lock() -// h.onShutdown = append(h.onShutdown, handler...) -// h.app.mutex.Unlock() -// }
223-229: Consider adding error return values to hook execution methodsThe hook execution methods currently log errors but don't return them. Consider returning an error to allow callers to handle failures appropriately.
-func (h *Hooks) executeOnPreShutdownHooks() { +func (h *Hooks) executeOnPreShutdownHooks() error { + var lastErr error for _, v := range h.onPreShutdown { if err := v(); err != nil { log.Errorf("failed to call pre shutdown hook: %v", err) + lastErr = err } } + return lastErr } -func (h *Hooks) executeOnPostShutdownHooks(err error) { +func (h *Hooks) executeOnPostShutdownHooks(err error) error { + var lastErr error for _, v := range h.onPostShutdown { if err := v(err); err != nil { log.Errorf("failed to call post shutdown hook: %v", err) + lastErr = err } } + return lastErr }Also applies to: 231-235
app_test.go (1)
918-919: Remove commented-out codeThe commented-out default case should be removed as it's no longer needed.
- // default: - // Server is still running, which is expected as the long-running request prevented full shutdown
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app.go(1 hunks)app_test.go(2 hunks)hooks.go(5 hunks)hooks_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- hooks_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- app.go
🧰 Additional context used
📓 Learnings (1)
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.
🔇 Additional comments (4)
hooks.go (2)
14-16: LGTM: Clear type definitions for shutdown hooks
The new pre and post shutdown handler types are well-defined and follow a logical separation of concerns.
32-34: LGTM: Consistent hook storage in Hooks struct
The hook storage fields follow the same pattern as other hooks in the struct.
app_test.go (2)
879-882: LGTM: Proper error handling for server shutdown
The serverErr channel is correctly used to capture and verify server errors.
895-897: 🛠️ Refactor suggestion
Remove redundant sleep
Based on the learnings from efectn, the time.Sleep is redundant since we already wait for the clientDone channel.
<-clientDone
-// Sleep to ensure the server has started processing the request
-time.Sleep(100 * time.Millisecond)Likely invalid or redundant comment.
hooks.go
Outdated
| func (h *Hooks) executeOnPostShutdownHooks(err error) { | ||
| for _, v := range h.onPostShutdown { | ||
| if err := v(err); err != nil { | ||
| log.Errorf("failed to call pre shutdown hook: %v", err) |
There was a problem hiding this comment.
Fix error message in log
The error message incorrectly states "pre shutdown hook" when it should be "post shutdown hook".
- log.Errorf("failed to call pre shutdown hook: %v", err)
+ log.Errorf("failed to call post shutdown hook: %v", err)Also applies to: 234-234
app_test.go
Outdated
| var shutdownHookCalled atomic.Int32 | ||
| // TODO: add test | ||
| // app.Hooks().OnShutdown(func() error { | ||
| // shutdownHookCalled.Store(1) | ||
| // return nil | ||
| // }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment and update shutdown hook test
The TODO comment and commented-out shutdown hook test should be uncommented and updated to use the new pre/post shutdown hooks.
var shutdownHookCalled atomic.Int32
-// TODO: add test
-// app.Hooks().OnShutdown(func() error {
-// shutdownHookCalled.Store(1)
-// return nil
-// })
+app.Hooks().OnPreShutdown(func() error {
+ shutdownHookCalled.Store(1)
+ return nil
+})Committable suggestion skipped: line range outside the PR's diff.
JIeJaitt
changed the title
…e/improve-shutdown-with-context
|
@efectn @ReneWerner87 @gaby I have completed the general revision, now can review my revision ideas, the rest of the lint problem and the content of the new document to add, and so on the big brothers review after I write, to prevent me from writing problems need to repeatedly modify the document! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
hooks_test.go (3)
238-238: Rename Unused Parametererrto Enhance ReadabilityThe parameter
errin the closure is not used, which may cause confusion. Renaming it to_clarifies that it is intentionally unused.Apply this diff:
- app.Hooks().OnPostShutdown(func(err error) error { + app.Hooks().OnPostShutdown(func(_ error) error { execution = append(execution, 1) return nil })🧰 Tools
🪛 GitHub Check: lint
[failure] 238-238:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)🪛 golangci-lint (1.62.2)
[warning] 238-238: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
243-243: Rename Unused Parametererrto Enhance ReadabilityThe parameter
errin the closure is not used. Consider renaming it to_to indicate that it is intentionally unused.Apply this diff:
- app.Hooks().OnPostShutdown(func(err error) error { + app.Hooks().OnPostShutdown(func(_ error) error { execution = append(execution, 2) return nil })🧰 Tools
🪛 GitHub Check: lint
[failure] 243-243:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)🪛 golangci-lint (1.62.2)
[warning] 243-243: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
263-263: Rename Unused Parametererrin ClosureThe parameter
erris not used within this closure. Renaming it to_will make it clear that the error parameter is intentionally unused.Apply this diff:
- app.Hooks().OnPostShutdown(func(err error) error { + app.Hooks().OnPostShutdown(func(_ error) error { return hookErr })🧰 Tools
🪛 GitHub Check: lint
[failure] 263-263:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)🪛 golangci-lint (1.62.2)
[warning] 263-263: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
listen_test.go (6)
50-50: Addt.Helper()to Helper FunctionThe function
testGracefulShutdownis a test helper function. Addingt.Helper()at the beginning of the function improves test diagnostics by marking it as a helper.Apply this diff:
func testGracefulShutdown(t *testing.T, shutdownTimeout time.Duration) { + t.Helper() var mu sync.Mutex var shutdown bool🧰 Tools
🪛 GitHub Check: lint
[failure] 50-50:
test helper function should start from t.Helper() (thelper)🪛 golangci-lint (1.62.2)
50-50: test helper function should start from t.Helper()
(thelper)
62-62: Rename Unused Parametererrin ClosureThe parameter
errin the closure is not used. Renaming it to_clarifies that it's intentionally unused.Apply this diff:
-app.hooks.OnPostShutdown(func(err error) error { +app.hooks.OnPostShutdown(func(_ error) error { mu.Lock() defer mu.Unlock() shutdown = true return nil })🧰 Tools
🪛 GitHub Check: lint
[failure] 62-62:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)🪛 golangci-lint (1.62.2)
[warning] 62-62: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
83-83: Check Error Return Value ofconn.Close()The error returned by
conn.Close()is not checked. Failing to handle potential errors might lead to resource leaks or unnoticed issues.Apply this diff:
conn, err := ln.Dial() if err == nil { - conn.Close() + err := conn.Close() + if err != nil { + t.Errorf("Error closing connection: %v", err) + } return true }🧰 Tools
🪛 GitHub Check: lint
[failure] 83-83:
Error return value ofconn.Closeis not checked (errcheck)🪛 golangci-lint (1.62.2)
83-83: Error return value of
conn.Closeis not checked(errcheck)
120-120: Avoid Unnecessary Copy of Loop VariableIn Go 1.22 and later, it is unnecessary to create a new variable
tcinside the loop. The loop variable is no longer reused across iterations, making the copy redundant.Apply this diff:
for _, tc := range testCases { - tc := tc t.Run(tc.name, func(t *testing.T) { time.Sleep(tc.waitTime)🧰 Tools
🪛 golangci-lint (1.62.2)
120-120: The copy of the 'for' variable "tc" can be deleted (Go 1.22+)
(copyloopvar)
134-134: Userequire.NoErrorInstead ofassert.NoErrorIn test code, using
require.NoErroris preferred overassert.NoErrorwhen the test cannot proceed if there's an error. It fails the test immediately, preventing further assertions on invalid data.Apply this diff:
- assert.NoError(t, err) + require.NoError(t, err)🧰 Tools
🪛 golangci-lint (1.62.2)
134-134: require-error: for error assertions use require
(testifylint)
145-145: Userequire.NoErrorInstead ofassert.NoErrorTo ensure the test fails immediately if there is an error, use
require.NoErrorinstead ofassert.NoError.Apply this diff:
- assert.NoError(t, <-errs) + require.NoError(t, <-errs)🧰 Tools
🪛 golangci-lint (1.62.2)
145-145: require-error: for error assertions use require
(testifylint)
listen.go (1)
485-489: Architectural improvement: Centralized shutdown notification via hooksThe change from direct callback functions to using the hooks system is a good architectural improvement. This centralization makes the shutdown process more maintainable and consistent.
Consider documenting the hook execution order in the package documentation to help users understand when their hooks will be called during shutdown.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 485-485: listen.go#L485
Added line #L485 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app.go(1 hunks)app_test.go(2 hunks)hooks.go(5 hunks)hooks_test.go(1 hunks)listen.go(1 hunks)listen_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.go
🧰 Additional context used
📓 Learnings (1)
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.
🪛 GitHub Check: codecov/patch
listen.go
[warning] 485-485: listen.go#L485
Added line #L485 was not covered by tests
hooks.go
[warning] 207-208: hooks.go#L207-L208
Added lines #L207 - L208 were not covered by tests
🪛 GitHub Check: lint
hooks_test.go
[failure] 217-217:
Error return value of app.Listen is not checked (errcheck)
[failure] 228-228:
comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
[failure] 238-238:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 243-243:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 259-259:
unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 263-263:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)
listen_test.go
[failure] 50-50:
test helper function should start from t.Helper() (thelper)
[failure] 62-62:
unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _ (revive)
[failure] 73-73:
Function Listener should pass the context parameter (contextcheck)
[failure] 83-83:
Error return value of conn.Close is not checked (errcheck)
🪛 golangci-lint (1.62.2)
hooks_test.go
217-217: Error return value of app.Listen is not checked
(errcheck)
228-228: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
[warning] 238-238: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 243-243: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 259-259: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _
(revive)
[warning] 263-263: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
listen_test.go
50-50: test helper function should start from t.Helper()
(thelper)
[warning] 62-62: unused-parameter: parameter 'err' seems to be unused, consider removing or renaming it as _
(revive)
73-73: Function Listener should pass the context parameter
(contextcheck)
83-83: Error return value of conn.Close is not checked
(errcheck)
93-93: fieldalignment: struct with 64 pointer bytes could be 40
(govet)
120-120: The copy of the 'for' variable "tc" can be deleted (Go 1.22+)
(copyloopvar)
134-134: require-error: for error assertions use require
(testifylint)
145-145: require-error: for error assertions use require
(testifylint)
🔇 Additional comments (4)
app_test.go (1)
895-897: Consider Removing Unnecessary Sleep to Improve Test Reliability
Using time.Sleep(100 * time.Millisecond) after receiving clientDone might lead to flaky tests or unnecessary delays. Since the clientDone channel ensures that the client has sent the request, consider implementing a synchronization mechanism to confirm that the server has started processing the request instead of relying on time.Sleep.
hooks.go (1)
207-208: 🛠️ Refactor suggestion
Increase Test Coverage for Error Handling in executeOnPreShutdownHooks
The error handling code within executeOnPreShutdownHooks is not currently covered by tests. To ensure that errors returned by pre-shutdown hooks are properly handled and logged, consider adding test cases that include pre-shutdown hooks which return errors. This will improve the robustness and reliability of the shutdown process.
Would you like assistance in generating the necessary tests to cover this error handling code?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 207-208: hooks.go#L207-L208
Added lines #L207 - L208 were not covered by tests
listen.go (2)
485-489: Verify hook execution order
The hooks system needs to guarantee consistent execution order during shutdown.
Let's verify the hook execution implementation:
✅ Verification successful
Hook execution order is properly maintained
The implementation guarantees consistent execution order during shutdown:
- Hooks are stored in a slice (
onPostShutdown) that maintains insertion order - Registration is thread-safe using app-level mutex lock/unlock
- Hooks are executed sequentially in the order they were registered
- Error handling is properly implemented without breaking execution order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check hooks implementation
ast-grep --pattern 'func (h *Hooks) executeOnPostShutdownHooks($_) {
$$$
}'
# Check for potential race conditions in hooks execution
rg -A 5 "sync\.Mutex.*executeOnPostShutdownHooks"
Length of output: 418
Script:
#!/bin/bash
# Check how hooks are registered and stored
ast-grep --pattern 'type Hooks struct {
$$$
}'
# Check hook registration methods
ast-grep --pattern 'func (h *Hooks) OnPostShutdown($_) {
$$$
}'
# Look for hook registration calls
rg -A 2 "OnPostShutdown"
Length of output: 3150
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 485-485: listen.go#L485
Added line #L485 was not covered by tests
485-485: Add test coverage for error path
The static analysis indicates that line 485 (error path) lacks test coverage.
Let's verify the test coverage and existing tests:
Would you like me to help create a test case that covers the error path during shutdown?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 485-485: listen.go#L485
Added line #L485 was not covered by tests
| t.Fatal("hook was not called") | ||
| } | ||
|
|
||
| if receivedErr != expectedErr { |
There was a problem hiding this comment.
Use errors.Is for Error Comparison
Comparing errors using != may fail when errors are wrapped. To accurately check for specific errors, use errors.Is instead.
Apply this diff to improve error comparison:
- if receivedErr != expectedErr {
+ if !errors.Is(receivedErr, expectedErr) {
t.Fatalf("hook received wrong error: want %v, got %v", expectedErr, receivedErr)
}📝 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.
| if receivedErr != expectedErr { | |
| if !errors.Is(receivedErr, expectedErr) { |
🧰 Tools
🪛 GitHub Check: lint
[failure] 228-228:
comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
🪛 golangci-lint (1.62.2)
228-228: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error
(errorlint)
There was a problem hiding this comment.
You can use require methods instead
There was a problem hiding this comment.
@efectn, you're right, using the require methods simplifies the test assertions. You can modify the test code to:
require.ErrorIs(t, receivedErr, expectedErr)or
require.Equal(t, expectedErr, receivedErr)This leverages the testify package's assertion functions for cleaner and more expressive tests.
✏️ Learnings added
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.698Z
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`.
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.
| }) | ||
|
|
||
| go func() { | ||
| _ = app.Listen(":0") |
There was a problem hiding this comment.
Check Error Return Value of app.Listen
The error returned by app.Listen(":0") is not checked. Ignoring errors can lead to unexpected behaviors or missed errors during testing. Please handle the error appropriately to ensure that any issues during the listening process are captured.
Apply this diff to handle the error:
- _ = app.Listen(":0")
+ err := app.Listen(":0")
+ require.NoError(t, err, "app.Listen failed")📝 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.
| _ = app.Listen(":0") | |
| err := app.Listen(":0") | |
| require.NoError(t, err, "app.Listen failed") |
🧰 Tools
🪛 GitHub Check: lint
[failure] 217-217:
Error return value of app.Listen is not checked (errcheck)
🪛 golangci-lint (1.62.2)
217-217: Error return value of app.Listen is not checked
(errcheck)
| @@ -900,16 +900,18 @@ func (app *App) ShutdownWithTimeout(timeout time.Duration) error { | |||
| // | |||
| // ShutdownWithContext does not close keepalive connections so its recommended to set ReadTimeout to something else than 0. | |||
| func (app *App) ShutdownWithContext(ctx context.Context) error { | |||
| if app.hooks != nil { | |||
There was a problem hiding this comment.
Here we can define executeOnPostShutdownHooks hook with defer as well as PreShutdownHook
There was a problem hiding this comment.
Here we can define executeOnPostShutdownHooks hook with defer as well as PreShutdownHook
Is that what you mean?I think PreShutdownHook and PostShutdownHook set outside the function and then executed at ShutdownWithContext
func (app *App) ShutdownWithContext(ctx context.Context) error {
app.mutex.Lock()
defer app.mutex.Unlock()
if app.server == nil {
return ErrNotRunning
}
// Execute the Shutdown hook
if app.hooks != nil {
app.hooks.executeOnPreShutdownHooks()
}
defer app.hooks.executeOnPostShutdownHooks(nil)
return app.server.ShutdownWithContext(ctx)
}
Description
feat: Optimize ShutdownWithContext method in app.go
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.
feat: Enhance ShutdownWithContext test for improved reliability
This commit improves the comprehensiveness and reliability of the
ShutdownWithContexttest, ensuring proper verification of shutdown hooks, timeout behavior, and server state during long-running requests.Changes 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.Commit formatting
Sorry, I just initiated the PR and saw this regulation. I will definitely add it in the next commit