-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: [#504] Route supports configure timeout #97
Conversation
goravel/goravel#504 add withcontext
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #97 +/- ##
==========================================
- Coverage 79.29% 78.41% -0.89%
==========================================
Files 12 16 +4
Lines 874 1019 +145
==========================================
+ Hits 693 799 +106
- Misses 151 183 +32
- Partials 30 37 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new method is good, please add the global configuration logic.
Please merge your two PRs. |
I merged two PCs. Please explain what "add the global configuration logic" means |
I added a timeout to the config, how to use them in gin and fiber? |
Do I understand correctly what needs to be added to goravel/gin/route.go in the function NewRoute(config config.Config, parameters map[string]any) (*Route, error) add |
CI failed. |
gin doesn't have the |
do I need to implement TimeoutMiddleware? |
Yes, don't use gin-contrib/timeout, it has a lot of dependencies, we can implement it ourselves. |
where to place middleware timeout? |
update NewRoute goravel#97
I added timeout middleware following https://github.com/goravel/gin/blob/master/tls.go and rewrote the use of this middleware in NewRoute. Please check my changes and give feedback |
please check my changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, according to the change in this PR, I think it's time to merge goravel/framework#675, please fix the CI.
After the PR above is merged, please execute go get github.com/goravel/framework@master
in this PR, to make sure the CI of this PR passes.
#97 (comment) #97 (comment) already implemented |
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
with this version, everything works for me |
I constantly get a pointer to nil in the request, that is, when ctx.Request() is called.Next() gives an error, the pointer leads to nil |
Could you add me to this repo? https://github.com/KlassnayaAfrodita/gin |
yes, I invited you. patch-1 current branch |
feat: optimize timeout middleware
Please solve the conflict. |
528e9dc
to
b4c16c6
Compare
middleware_timeout_test.go
Outdated
require.NoError(t, err) | ||
|
||
route.Middleware(Timeout(1*time.Second)).Get("/timeout", func(ctx contractshttp.Context) contractshttp.Response { | ||
time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case failed in Windows: https://github.com/goravel/gin/actions/runs/11534959849/job/32113680328?pr=97
We may try to change this to 2 second:
time.Sleep(1 * time.Second) | |
time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR, thanks!
We can optimize fiber next. |
goravel/goravel#504
add withcontext
📑 Description
Closes goravel/goravel#504
@coderabbitai summary
✅ Checks