🔥 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3162 +/- ##
==========================================
- Coverage 82.79% 82.72% -0.08%
==========================================
Files 114 114
Lines 11151 11151
==========================================
- Hits 9233 9225 -8
- Misses 1519 1525 +6
- Partials 399 401 +2
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 new methods and updating existing ones in the documentation. A new method Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (2)
app_test.go (2)
893-894: Explain the purpose of the sleepThe sleep after closing the
clientDonechannel might be necessary to ensure the server has started processing the request. Consider adding a comment to explain its purpose for better clarity.<-clientDone +// Sleep to ensure the server has started processing the request time.Sleep(100 * time.Millisecond)
913-918: Consider removing or explaining the commented-out codeThe commented-out default case in the select statement at the end of the function might be confusing. Consider either removing it or adding a comment explaining why it's kept for future reference.
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 +// Note: The default case is commented out because we expect the server to shut down. +// If uncommented, it would handle the case where the server is still running due to a long-running request. }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app_test.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
app_test.go (5)
864-868: Improved shutdown hook verificationThe addition of the
shutdownHookCalledvariable and its associated hook is a good improvement. It allows the test to verify that the shutdown hook is actually called during the shutdown process.
877-891: Good use of channels for concurrency managementThe use of channels (
serverErr,clientDone) for managing concurrent operations in the test is a good practice. It helps in synchronizing the different parts of the test and ensures proper sequencing of events.
896-901: Proper error handling for shutdown timeoutThe use of
context.WithTimeoutfor the shutdown process is good. It ensures that the shutdown doesn't hang indefinitely. The error is properly captured and checked in the subsequent assertions.
903-909: Appropriate assertions for shutdown behaviorThe assertions checking for the timeout error and the
DeadlineExceedederror are appropriate. They verify that the shutdown process behaves correctly when it times out.
911-911: Verify shutdown hook executionGood addition of an assertion to check if the shutdown hook was called. This ensures that the shutdown process is working as expected.
app_test.go
Outdated
| var shutdownHookCalled int32 | ||
| app.Hooks().OnShutdown(func() error { | ||
| atomic.StoreInt32(&shutdownHookCalled, 1) | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Potential data race on shutdownHookCalled
The shutdownHookCalled boolean is accessed by multiple goroutines without proper synchronization, which could lead to a data race. Consider using sync/atomic package for thread-safe operations on this variable.
Here's a suggested fix:
+import "sync/atomic"
-var shutdownHookCalled int32
+var shutdownHookCalled atomic.Int32
app.Hooks().OnShutdown(func() error {
- atomic.StoreInt32(&shutdownHookCalled, 1)
+ shutdownHookCalled.Store(1)
return nil
})
// Later in the test
-assert.Equal(t, int32(1), atomic.LoadInt32(&shutdownHookCalled), "Shutdown hook was not called")
+assert.Equal(t, int32(1), shutdownHookCalled.Load(), "Shutdown hook was not called")Committable suggestion was skipped due to low confidence.
|
@gaby Hello, I have completed the repair of lint related issues |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app_test.go (1)
864-917: Improved shutdown test implementationThe changes to the
Test_App_ShutdownWithContextfunction look good. The test now properly checks for the execution of the shutdown hook using an atomic integer, and the timing issues have been addressed with appropriate sleep durations.Consider adding a more specific error check for the shutdown error:
- require.Error(t, err, "Expected shutdown to return an error due to timeout") + require.Error(t, err, "Expected shutdown to return an error due to timeout") + require.ErrorIs(t, err, context.DeadlineExceeded, "Expected DeadlineExceeded error")This will ensure that the specific error type (context.DeadlineExceeded) is returned, providing a more precise test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
- app_test.go (2 hunks)
🧰 Additional context used
…main' of github.com:JIeJaitt/fiber into jiejaitt-feature/improve-shutdown-with-context
go.mod
Outdated
| @@ -21,6 +21,7 @@ require ( | |||
| github.com/philhofer/fwd v1.1.3-0.20240612014219-fbbf4953d986 // indirect | |||
| github.com/pmezard/go-difflib v1.0.0 // indirect | |||
| github.com/valyala/tcplisten v1.0.0 // indirect | |||
| golang.org/dl v0.0.0-20241001165935-bedb0f791d00 // indirect | |||
There was a problem hiding this comment.
This is an official multi-version control function of the go language, because the Go version I use locally is higher than the go version in fiber's go mod. In order to keep the version consistent, I rely on this method to use it locally in the same way as the fiber framework. Go language version, you can see this to get more information: https://pkg.go.dev/golang.org/dl and https://pkg.go.dev/golang.org/[email protected]/go1.22.6 . It will not have any impact on the function of the fiber
go.sum
Outdated
| @@ -29,6 +29,8 @@ github.com/valyala/fasthttp v1.56.0 h1:bEZdJev/6LCBlpdORfrLu/WOZXXxvrUQSiyniuaoW | |||
| github.com/valyala/fasthttp v1.56.0/go.mod h1:sReBt3XZVnudxuLOx4J/fMrJVorWRiWY2koQKgABiVI= | |||
| github.com/valyala/tcplisten v1.0.0 h1:rBHj/Xf+E1tRGZyWIWwJDiRY0zc1Js+CV5DqwacVSA8= | |||
| github.com/valyala/tcplisten v1.0.0/go.mod h1:T0xQ8SeCZGxckz9qRXTfG43PvQ/mcWh7FwZEA7Ioqkc= | |||
| golang.org/dl v0.0.0-20241001165935-bedb0f791d00 h1:OX0WPBB1pQPZy1SL0+q5C/VuuM6e1wv6uEuB9iyBi/I= | |||
There was a problem hiding this comment.
Same as my answer above
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/api/fiber.md (1)
208-208: Enhance documentation clarity and completenessThe current documentation could be improved in both grammar and completeness. Consider the following suggestions:
- Add a comma for better readability
- Improve the phrasing of the force shutdown description
- Include details about the mutex lock optimization
Apply this change to improve the documentation:
-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, forcefully terminating 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. The method is optimized to acquire locks early, allowing for immediate return if the server is not running.🧰 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 (1)
- 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)
…e/improve-shutdown-with-context
- 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.
…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.
| <-clientDone | ||
| // Sleep to ensure the server has started processing the request | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Replace sleep with synchronization for test reliability
Using time.Sleep(100 * time.Millisecond) to wait for the server to start processing the request might lead to flaky tests. Consider using synchronization primitives like channels or sync.WaitGroup to ensure the server has begun processing before proceeding.
…/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
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