-
Notifications
You must be signed in to change notification settings - Fork 288
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
update timeout middleware with warnings #218
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,48 +8,62 @@ description = "Timeout middleware for Echo" | |
|
||
Timeout middleware is used to timeout at a long running operation within a predefined period. | ||
|
||
When timeout occurs, and the client receives timeout response the handler keeps running its code and keeps using resources until it finishes and returns! | ||
|
||
> Timeout middleware is a serious performance hit as it buffers all responses from wrapped handler. Do not set it in front of file downloads or responses you want to stream to the client. | ||
|
||
Timeout middleware is not a magic wand to hide slow handlers from clients. Consider designing/implementing asynchronous | ||
request/response API if (extremely) fast responses are to be expected and actual work can be done in background | ||
Prefer handling timeouts in handler functions explicitly | ||
|
||
*Usage* | ||
|
||
`e.Use(middleware.Timeout())` | ||
`e.GET("/", handlerFunc, middleware.Timeout())` | ||
|
||
## Custom Configuration | ||
|
||
*Usage* | ||
|
||
```go | ||
// Echo instance | ||
e := echo.New() | ||
e.Use(middleware.TimeoutWithConfig(middleware.TimeoutConfig{ | ||
Skipper: Skipper, | ||
ErrorHandler: func(err error, e echo.Context) error { | ||
// you can handle your error here, the returning error will be | ||
// passed down the middleware chain | ||
return err | ||
}, | ||
Timeout: 30*time.Second, | ||
})) | ||
|
||
handlerFunc := func(c echo.Context) error { | ||
time.Sleep(10 * time.Second) | ||
return c.String(http.StatusOK, "Hello, World!\n") | ||
} | ||
middlewareFunc := middleware.TimeoutWithConfig(middleware.TimeoutConfig{ | ||
Timeout: 30 * time.Second, | ||
ErrorMessage: "my custom error message", | ||
}) | ||
// Handler with timeout middleware | ||
e.GET("/", handlerFunc, middlewareFunc) | ||
``` | ||
|
||
## Configuration | ||
|
||
```go | ||
// TimeoutConfig defines the config for Timeout middleware. | ||
TimeoutConfig struct { | ||
// Skipper defines a function to skip middleware. | ||
Skipper Skipper | ||
// ErrorHandler defines a function which is executed for a timeout | ||
// It can be used to define a custom timeout error | ||
ErrorHandler TimeoutErrorHandlerWithContext | ||
// Timeout configures a timeout for the middleware, defaults to 0 for no timeout | ||
Timeout time.Duration | ||
} | ||
``` | ||
type TimeoutConfig struct { | ||
// Skipper defines a function to skip middleware. | ||
Skipper Skipper | ||
|
||
*TimeoutErrorHandlerWithContext* is responsible for handling the errors when a timeout happens | ||
```go | ||
// TimeoutErrorHandlerWithContext is an error handler that is used | ||
// with the timeout middleware so we can handle the error | ||
// as we see fit | ||
TimeoutErrorHandlerWithContext func(error, echo.Context) error | ||
// ErrorMessage is written to response on timeout in addition to http.StatusServiceUnavailable (503) status code | ||
// It can be used to define a custom timeout error message | ||
ErrorMessage string | ||
|
||
// OnTimeoutRouteErrorHandler is an error handler that is executed for error that was returned from wrapped route after | ||
// request timeouted and we already had sent the error code (503) and message response to the client. | ||
// NB: do not write headers/body inside this handler. The response has already been sent to the client and response writer | ||
// will not accept anything no more. If you want to know what actual route middleware timeouted use `c.Path()` | ||
OnTimeoutRouteErrorHandler func(err error, c echo.Context) | ||
|
||
// Timeout configures a timeout for the middleware, defaults to 0 for no timeout | ||
// NOTE: when difference between timeout duration and handler execution time is almost the same (in range of 100microseconds) | ||
// the result of timeout does not seem to be reliable - could respond timeout, could respond handler output | ||
// difference over 500microseconds (0.5millisecond) response seems to be reliable | ||
Timeout time.Duration | ||
} | ||
``` | ||
|
||
*Default Configuration* | ||
|
@@ -58,6 +72,64 @@ TimeoutErrorHandlerWithContext func(error, echo.Context) error | |
DefaultTimeoutConfig = TimeoutConfig{ | ||
Skipper: DefaultSkipper, | ||
Timeout: 0, | ||
ErrorHandler: nil, | ||
ErrorMessage: "", | ||
} | ||
``` | ||
|
||
## Alternatively handle timeouts in handlers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sure I'm missing something, because for me this is what the Timeout middleware is actually doing. Could you please help me to understand why this is better vs using the Timeout middleware? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion handling work in background is not webserver responsibility and should be implemented explicitly in business domain (i.e. by developer). This ensures that people will not set |
||
|
||
```go | ||
func main() { | ||
e := echo.New() | ||
|
||
doBusinessLogic := func(ctx context.Context, UID string) error { | ||
// NB: Do not use echo.JSON() or any other method that writes data/headers to client here. This function is executed | ||
// in different coroutine that should not access echo.Context and response writer | ||
|
||
log.Printf("uid: %v\n", UID) | ||
//res, err := slowDatabaseCon.ExecContext(ctx, query, args) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add note here. Using methods with contexts has maybe sideeffects that are not wanted - i.e. when timeout we set to request is reached, |
||
time.Sleep(10 * time.Second) // simulate slow execution | ||
log.Print("doBusinessLogic done\n") | ||
return nil | ||
} | ||
|
||
handlerFunc := func(c echo.Context) error { | ||
defer log.Print("handlerFunc done\n") | ||
|
||
// extract and validate needed data from request and pass it to business function | ||
UID := c.QueryParam("uid") | ||
|
||
ctx, cancel := context.WithTimeout(c.Request().Context(), 5 * time.Second) | ||
defer cancel() | ||
result := make(chan error) | ||
go func() { // run actual business logic in separate coroutine | ||
defer func() { // unhandled panic in coroutine will crash the whole application | ||
if err := recover(); err != nil { | ||
result <- fmt.Errorf("panic: %v", err) | ||
} | ||
}() | ||
result <- doBusinessLogic(ctx, UID) | ||
}() | ||
|
||
select { // wait until doBusinessLogic finishes or we timeout while waiting for the result | ||
case <-ctx.Done(): | ||
err := ctx.Err() | ||
if err == context.DeadlineExceeded { | ||
return echo.NewHTTPError(http.StatusServiceUnavailable, "doBusinessLogic timeout") | ||
} | ||
return err // probably client closed the connection | ||
case err := <-result: // doBusinessLogic finishes | ||
if err != nil { | ||
return err | ||
} | ||
} | ||
return c.NoContent(http.StatusAccepted) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I got it correctly, this is returning a 202 even when doBusinessLogic finished. At this point the code should return 200 instead. |
||
} | ||
e.GET("/", handlerFunc) | ||
|
||
s := http.Server{Addr: ":8080", Handler: e} | ||
if err := s.ListenAndServe(); err != http.ErrServerClosed { | ||
log.Fatal(err) | ||
} | ||
} | ||
``` |
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.
Sorry for posting a comment too late. When I read this the first time I was not sure if it's true, so after a while playing around with the Timeout middleware a found a bug in it. Please see labstack/echo#1909
So, I think this should say that the handler must handle the Request Context properly in order to finish when the middleware timeouts.
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.
no, only "reasonable" usecase for timeout middleware is to let handler execute until it normally finishes/exists regardless what happens with coroutine serving request. If this usecase is not needed - then timeout middleware is not needed. you could just create new context with timeout and be fine.
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.
"reasonable" i.e. if client disconnects prematurely the db transaction would still get finished and commited.
if we start cancelling contexts it would mean that
conn.ExecContext(ctx, query, args)
would get cancelled and work would not get commited.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.
I should probably indicate that also in example that
doBusinessLogic
takes in will get deadlined.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.
I think
timeout
has 2 properties that user may seek/need: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.
Well, for me the timeout middleware was to ensure that your handler do the work you want in a particular time period, and if that isn't true you get a response that indicates that. And for that reason all the work should be stop. But your descriptions are more like a middleware to do async work, and of course you are right, if that is the case the work must continue until it is finished. Maybe we need to decide which of this two behaviors will be supported by this middleware an rename it to clarify our intention if it is needed.