-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Custom timeout middleware changes behavior of code #2763
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
Comments
Well I see why it works for the first one, it essentially is just skipping the timeout middleware. Still curious why it is sending a 200 for /do2 |
You are probably better off with context timeout middleware. Timeout middleware runs handler actually in separate goroutine and if that ends up in endless loop there is nothing that will stop it. |
So I tried that one and it does cancel the context but it doesn't automatically respond with a timeout like the regular timeout middleware does. It also doesn't seem to run that error function that it has in its config. |
For func main() {
e := echo.New()
ctmw := middleware.ContextTimeoutWithConfig(middleware.ContextTimeoutConfig{
Skipper: nil,
ErrorHandler: func(err error, c echo.Context) error {
return err
},
Timeout: 10 * time.Second,
})
e.GET("/do2", hello, ctmw, authenticate)
e.Start(":8080")
} echo/middleware/context_timeout.go Lines 58 to 74 in c44f628
line 70 is not executed when handler retuns an error? |
Sorry, my mistake. I got two things switched around in my code I was testing with. So the error is handling correctly, however the timeout is only cancelling the context and not responding with the 503 as I expected and does in the other middleware. Which I know isn't programmed in. Is this just not a feature that is supported? |
this current example does not run long enough to context to be deadlined. Without auth headers you will be served with 401 as this is the error that is returned.
also this code is problematic - c.JSON will send the response and error that is returned is ignored by global error handler because the response was already sent with c.Json |
your handler has to be func authenticate(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
authHeader := c.Request().Header.Get("Authorization")
if authHeader != "secret" {
return echo.ErrUnauthorized
}
return next(c)
}
} with this - timeout middleware will be also ok |
but I still recommend context timeout. |
If I change my hello handler to be: func hello(c echo.Context) error {
go func() {
fmt.Println("deadline started")
<-c.Request().Context().Done()
fmt.Println("deadline done")
}()
time.Sleep(10 * time.Second)
return c.JSON(http.StatusOK, "hello")
} The deadline will stop after the 3 seconds (good), but it will wait the full 10 seconds then respond to my client with a 200. |
this is because context aware is something like that select {
case <-time.After(10 * time.Second):
case <-c.Request().Context().Done():
} |
Right. But what I’m mostly looking for is for, after a given timeout,
cancel the context and immediately respond back to the client with a 503.
…On Thu, Apr 3, 2025 at 3:57 PM Martti T. ***@***.***> wrote:
this is because time.Sleep(10 * time.Second) is not "context aware"
—
Reply to this email directly, view it on GitHub
<#2763 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABZFTXGGM2ZXXLULTFSGKF32XWHEBAVCNFSM6AAAAAB2M543TCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZWG44DQNBWGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
[image: aldas]*aldas* left a comment (labstack/echo#2763)
<#2763 (comment)>
this is because time.Sleep(10 * time.Second) is not "context aware"
—
Reply to this email directly, view it on GitHub
<#2763 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABZFTXGGM2ZXXLULTFSGKF32XWHEBAVCNFSM6AAAAAB2M543TCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONZWG44DQNBWGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
you really should not choose this kind of middleware just base of response code. Context timeout will do that same, assuming you are using context aware methods echo/middleware/context_timeout.go Lines 49 to 56 in c44f628
|
Please read #2745 (comment) and there are older posts that discuss differencens in these middlewares if you search in issues history |
It's more of just convenience. It is nice because we have some endpoints that do things that aren't necessarily context away and even though we may have a rogue go routine, it ends up being a better DX (in our situation) to have this cancel and respond with a simple middleware. We also are moving from the base http library to Echo, and are using this from the standard library. Having the same functionality would be nice but if it isn't achievable with Echo then it ain't the end of the world. And thanks for the link, in there you say "Timeout MW will just send response to client that request timeoutted" which is exactly what I want, its more of the weird behavior around another middleware sending a response but then the client receiving a 200. You did mention earlier that "c.JSON will send the response and error that is returned is ignored by global error handler because the response was already sent with c.Json", which seems like it relates to what problem I'm seeing? But I would expect a call to c.JSON to start writing immediately then if the global error handler tried to write again, it would really just be a superfluous write cause the middleware already sent the response. |
generally with Echo - do not write/send errors to client with c.JSON/.String. Errors have to be returned from handlers as |
Ok that makes sense. So this is really me not using things right. I should send an "unauthorized" error back up from my middleware and have a global error handler right? Like should I be using the |
yes, nb: check the comment on default error handler about errors and if you implement your version then do not forget to add that Lines 411 to 422 in de44c53
|
I guess that is what is confusing me. If This is a little out of my realm of expertise, but if that is saying that it Also, I know you've basically solved my issue and I appreciate you taking time to help me understand the why here. So if this is getting to be too much then just let me know. I just am trying to understand why this is happening. |
@thestephenstanton did you figure out the source of the cryptic 200? |
@obataku this is because authentication middleware in example is sending the response for error itself and not letting timeout middleware to handle it. So this func authenticate(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
authHeader := c.Request().Header.Get("Authorization")
if authHeader != "secret" {
c.JSON(http.StatusUnauthorized, customError{Message: "unauthorized"})
return echo.ErrUnauthorized
}
return next(c)
}
} should be this func authenticate(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
authHeader := c.Request().Header.Get("Authorization")
if authHeader != "secret" {
return echo.ErrUnauthorized
}
return next(c)
}
} |
@aldas I follow but it's not obvious to me why |
Probably if you try this enough time you will see sometimes that as you see - this is not trivial. far from it. If you want to run your business logic asynchnously then create/run goroutine in your business layer and DO NOT try to delegate this to webframe work. Timeout middleware is actually about running response asynchnously and Context timeout middleware is about ending handlers after certain time or client disconnects. timeout middleware is black magic around multiple goroutines and how/when Go standard library decides to write data back to client. |
I could totally be thinking about this thing wrong and have the wrong mental model but in this simplified configuration, I'm getting weird behavior:
/do1 will return as expected but /do2 will respond with a 200 and have no body
Maybe where I'm wrong in my thoughts is responding in the
authenticate
middleware. Essentially I want to have that middleware respond but I also want to return up the error so that other middlewares above can get that error (e.g. logging middleware). Is this not the right way to go about this?To me it seems more like a problem with that custom timeout middleware. I don't understand why having the custom one would act any different than the
middleware.Timeout()
one.The text was updated successfully, but these errors were encountered: