fiber
fiber copied to clipboard
feat: Improve and Optimize ShutdownWithContext Func
Description
feat: Optimize ShutdownWithContext method in app.go
- 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.
feat: Enhance ShutdownWithContext test for improved reliability
- 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.
Changes introduced
- By holding the mutex lock throughout the operation, any modifications to the app's state are protected. defer ensures that the hook function is executed only after it is unlocked. At this time, no other goroutine will access the shared resource at the same time, ensuring concurrency safety.
- defer ensures that the hook function will be executed regardless of whether an error occurs in the function. It also avoids execution order issues and further ensures concurrency safety.
Type of change
Please delete options that are not relevant.
- [x] New feature (non-breaking change which adds functionality)
Checklist
Before you submit your pull request, please make sure you meet these requirements:
- [x] Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
- [x] Conducted a self-review of the code and provided comments for complex or critical parts.
- [x] Updated the documentation in the
/docs/directory for Fiber's documentation. - [x] Added or updated unit tests to validate the effectiveness of the changes or new features.
- [x] Ensured that new and existing unit tests pass locally with the changes.
- [x] Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
- [x] Aimed for optimal performance with minimal allocations in the new code.
- [x] Provided benchmarks for the new code to analyze and improve upon.
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]( https://github.com/gofiber/fiber/blob/master/.github/CONTRIBUTING.md#pull -requests-or-commits)
Sorry, I just initiated the PR and saw this regulation. I will definitely add it in the next commit
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
Codecov Report
Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.
Project coverage is 84.14%. Comparing base (
283ef32) to head (cda210b). Report is 4 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| hooks.go | 91.66% | 2 Missing :warning: |
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
| Flag | Coverage Δ | |
|---|---|---|
| unittests | 84.14% <94.11%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
[!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
@coderabbitai reviewcommand as a PR comment. Alternatively, push new commits to this PR.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.
📥 Commits
Reviewing files that changed from the base of the PR and between 05e14f58ccfece592d6b941b37080a348bd20990 and cda210b359afedff13c50bef31df78aa0d9a35d7.
📒 Files selected for processing (1)
docs/api/hooks.md(3 hunks)
Walkthrough
The changes enhance the Fiber web framework by introducing a new method New for creating an instance of App with customizable configuration options. The Listen method now accepts an optional ListenConfig parameter for server customization. Additionally, the ShutdownWithContext method has been updated to clarify the execution of shutdown hooks and improve control flow during shutdown. Documentation has been expanded to provide detailed descriptions and examples for these new features, ensuring clarity for users.
Changes
| File(s) | Change Summary |
|---|---|
| docs/api/fiber.md | Updated documentation to include new New method, detailed Config structure, and examples for app instantiation and server listening. |
| app.go | Updated ShutdownWithContext method to check server state before shutdown and reorder hook execution. |
| app_test.go | Enhanced tests for ShutdownWithContext, added atomic operations for shutdown hook verification, and refined error handling during shutdown. |
| hooks.go | Restructured shutdown hook handling by introducing OnPreShutdownHandler and OnPostShutdownHandler, replacing the previous OnShutdownHandler. Updated execution methods for new hooks. |
| hooks_test.go | Renamed Test_Hook_OnShutdown to Test_Hook_OnPreShutdown, added Test_Hook_OnPostShutdown, and updated assertions for shutdown hooks. |
| listen.go | Removed OnShutdownError and OnShutdownSuccess fields from ListenConfig, centralizing error handling in hooks. |
| listen_test.go | Restructured Test_Listen_Graceful_Shutdown into a new helper function testGracefulShutdown for better organization and clarity. |
| docs/api/hooks.md | Introduced OnPreShutdown and OnPostShutdown hooks, updated related documentation to reflect the changes. |
| docs/whats_new.md | Added a new section detailing significant updates in version 3, including new hooks and method changes. |
Possibly related PRs
- gofiber/fiber#3306: The changes in the main PR regarding the
ShutdownWithContextmethod inapp.goare related to the modifications in the same method in the retrieved PR, which also involves error handling and control flow during shutdown processes. - gofiber/fiber#3220: The changes in the main PR regarding the
ShutdownWithContextmethod's error handling and execution order are directly related to the modifications in the retrieved PR, which also focuses on enhancing shutdown processes, including the introduction of aShutdownTimeoutfeature.
Suggested reviewers
- sixcolors
- gaby
- ReneWerner87
- efectn
Poem
In the garden of code, changes bloom bright,
With hooks that now dance, in the soft moonlight.
Shutdowns are smoother, no more race to the end,
Documentation's richer, our framework's new friend.
So hop with delight, let the updates take flight! 🐇✨
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?
🪧 Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
I pushed a fix in commit <commit_id>, please review it.Generate unit testing code for this file.Open a follow-up GitHub issue for this discussion.
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:@coderabbitai generate unit testing code for this file.@coderabbitai modularize this function.
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.@coderabbitai read src/utils.ts and generate unit testing code.@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.@coderabbitai help me debug CodeRabbit configuration file.
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)
@coderabbitai pauseto pause the reviews on a PR.@coderabbitai resumeto resume the paused reviews.@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository.@coderabbitai full reviewto do a full review from scratch and review all the files again.@coderabbitai summaryto regenerate the summary of the PR.@coderabbitai generate docstringsto generate docstrings for this PR. (Beta)@coderabbitai resolveresolve all the CodeRabbit review comments.@coderabbitai configurationto show the current CodeRabbit configuration for the repository.@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
@JIeJaitt The Makefile in the root of the repo, has a the commands to run tests/lint, etc
@JIeJaitt The Makefile in the root of the repo, has a the commands to run tests/lint, etc
Thanks, I'll try to fix any go-lint issues I have
@gaby Hello, I have completed the repair of lint related issues
false positive for the benchmark because of the same naming https://github.com/benchmark-action/github-action-benchmark/issues/264
@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?
@JIeJaitt can you make another comment in the whats new markdown?
No problem, I will add it in what new later
@efectn @gaby pls check this PR
@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!
@JIeJaitt can you update the code with the last ideas
@ReneWerner87 Hi, I have updated the code as per the latest discussion. But I'm having a little go lint glitch. The Listener entry doesn't pass ctx, but go-lint lets me pass ctx, which is weird!
To solve this problem, you may have to change the signature of the Listener method so that it receives the context parameter directly. This seems to be a long-standing problem.
@JIeJaitt
problem is that the context is created outside in another golang function
previously both were within the same scope
@efectn @ReneWerner87 @gaby Please review the latest code
@JIeJaitt pls adjust the markdown documentation
- [ ] https://github.com/gofiber/fiber/blob/main/docs/api/hooks.md
@efectn pls re-review
@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
we will have a look
very thankful
@ReneWerner87 @efectn @gaby all github action check succeed. Please review the latest code
@JIeJaitt last two doc changes and then i will merge
@JIeJaitt last two doc changes and then i will merge
I have modified the document according to the discussion.