-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Timeout middleware implementation #1743
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1743 +/- ##
==========================================
+ Coverage 86.75% 86.88% +0.13%
==========================================
Files 29 30 +1
Lines 2069 2090 +21
==========================================
+ Hits 1795 1816 +21
Misses 175 175
Partials 99 99
Continue to review full report at Codecov.
|
Currently Go 1.12 is skipped due to missing Request.Clone, is this something that should really be added or can we live without it? I was considering adding a separate file for go 1.12 using Request.WithContext but that only creates a shallow clone, and I don't know what are the implications of it with other usecases. |
We just need to clarify if a timeout middleware should be part of echo core. As we are also preparing a rate limiter in #1724 I guess this may fit into core too. |
5393d80
to
d69960f
Compare
76752db
to
c69860a
Compare
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.
LGTM!
Please improve the TestTimeoutTestRequestClone test and I'll approve the PR
middleware/timeout_test.go
Outdated
|
||
m := TimeoutWithConfig(TimeoutConfig{ | ||
Timeout: time.Second, | ||
ErrorHandler: func(err error, c echo.Context) error { |
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.
Instead of checking if the Context was cloned properly on the ErrorHandler function, do it directly on the HandlerFunc that you pass to the Timeout middleware. It'll be more straightforward and probably the test will run faster because is not needed to wait for the timeout
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.
Moved to the handler
… from go1.13 only
5f78d62
to
4e2e46b
Compare
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.
LGTM!
Thanks for the new Middleware!!!
In your futures PRs, please add new commits instead of updating the original one, that helps a lot during review (I can focus on the new changes and ignore the already reviewed code)
Great! So what is missing now is documentation. |
Will do.
…On Tue, Jan 5, 2021, 11:15 Roland Lammel ***@***.***> wrote:
Merged #1743 <#1743> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1743 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIWFY26ENIAURV6CKLPNNDSYLRCXANCNFSM4VOGTKAQ>
.
|
I added a MR labstack/echox#173 |
There are occasions when you want to add a timeout to your handlers, so they get finished within a specified period of time.